Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature request] ofPolyline addVertex accepting glm::vec2 #7831

Open
dimitre opened this issue Dec 20, 2023 · 10 comments
Open

[feature request] ofPolyline addVertex accepting glm::vec2 #7831

dimitre opened this issue Dec 20, 2023 · 10 comments

Comments

@dimitre
Copy link
Member

dimitre commented Dec 20, 2023

it should just work, as ofPolyline is usually a 2d drawing.
some related discussion here:

@artificiel
Copy link
Contributor

definitely. I've skimmed through #5440 and TLDR is that ofPolyline is templated and things are not simple because T may == glm::vec2. this branch has work to support compile-time enabling by checking the types:

https://github.com/arturoc/openFrameworks/blob/1a2c94bfa52abf4f1f42ce934e0249746332a1f4/libs/openFrameworks/graphics/ofPolyline.h#L65-L69

then enabling the correct interpretation on the different glm::vec2 inputs such as:

https://github.com/arturoc/openFrameworks/blob/1a2c94bfa52abf4f1f42ce934e0249746332a1f4/libs/openFrameworks/graphics/ofPolyline.h#L90C1-L94C1

(but the branch has many other things going; perhaps the relevant bits can be pulled?)

@dimitre
Copy link
Member Author

dimitre commented Dec 23, 2023

a new pr with relevant changes would be great!

@dimitre
Copy link
Member Author

dimitre commented Feb 7, 2025

I've stumbled in this again and there are some things in the way of having a ofPolyline_<glm::vec2>

this initialization

	void setRightVector(T v = T(0, 0, -1));

and this doesn't work

ofGetCurrentRenderer()->draw(*this);

because the template is already initialized with ofDefaultVertexType

using ofPolyline = ofPolyline_<ofDefaultVertexType>;
using ofDefaultVertexType = ofDefaultVec3;

@artificiel
Copy link
Contributor

yes as mentionned above #5739 addresses all these problems but the work is not "done" — the PR is actually closed unfinished and it's scope is wider than 2d vs 3d (or maybe solving that pulls other things in). not clear if extracting the relevant bits is more productive than just redoing it (if constexpr seems more fluid for most of the cases, which is C++17, which was not allowed in OF at the moment of that PR. also _v).

@ofTheo
Copy link
Member

ofTheo commented Feb 7, 2025

I'd love easier glm::vec3 to glm::vec2 conversion in general!!!

It is often a hangup with functions arguments and it often triggers really hard to decipher error messages.
I'm not sure if there is a way to make a helper class or something to do this or a preprocessor macro, but I am guessing not.

Wish there was a glm define to allow for automatic downgrading of glm::vec3 to vec2 and vice versa.

@artificiel
Copy link
Contributor

the problem with ofPolyLine is not strictly conversion per se but that the code design of the class makes assumptions that the data is 3D. it should currently be named of3DPolyLine where the template stuff is private implementation. making it more flexible is what #5739 is after, where it can publicy become ofPolyLine<glm::vec3> or 2.

otherwise glm::vec3 -> 2 already works; 2 -> 3 does not because there is no unique correct way to default the value for 3rd member (often 0 but no necessarily). the old ofVec3f did it by assuming 0 for missing z, which is fine for 3D position in a (-1,1) space, but glm::vec3 gets used for other things too... hence glm deciding not supporting that conversion/construction.

/// \brief Construt a 3D vector with `x`, `y` and `z` specified
ofVec3f( float x, float y, float z=0 );

we could patch glm/detail/type_vec2.hpp for opt-in support:

#ifdef GLM_ALLOW_IMPLICIT_VEC2_TO_VEC3_CONVERSION_DEFAULTING_Z_TO_ZERO
    operator glm::vec3() const {
        return glm::vec3(x, y, 0.0f); 
    }
#endif

but while that might help, in itself it will not solve ofPolyLine<glm::vec2>.

also, if we focus on this specific issue as defined by it's title it's more of an user interface issue: it's simple to overload ofPolyLine::addVertex(glm::vec2 vec) and set the zero in the method (ofPolyLine::addVertex( float x, float y, float z=0 ) already does something similar). there is a big diff in allowing ofPolyLine to ingest 2D, and having the whole structure optimized in 2D.

@dimitre
Copy link
Member Author

dimitre commented Feb 14, 2025

@ofTheo I'm going in the opposite direction :D
by removing implicit conversion and enforcing correct data types I think it is better to know what is going on (at least on OF Core) and sometimes a lot of implicit conversion is going on just to accept some int or double multiplication on GLM. like

vert = (vertices[i]+vertices[i+1]+vertices[i+2]) / 3;

it is probably converting glm::vec3 to ofVec3f to be able to multiply by int.

@artificiel
Copy link
Contributor

@dimitre if you don't know "what is going on" FYI it has nothing to do with implicit conversion — glm::vec3 implements operator + and * and / on scalars (and a large number of other operators) so I'm not sure where the idea that "probably converting glm::vec3 to ofVec3f" comes from.

that being said I don't understand why implicit conversion is problematic. glm supports implicit conversion from glm::vec3 to ::vec2 — are you suggesting to remove that from glm?

@dimitre
Copy link
Member Author

dimitre commented Feb 14, 2025

@artificiel I actually found this issues while removing implicit conversion, it is not from float, it is from double and int.
just changing / 3 to /3.0f fixes that.

and glm is more strict with types conversion.

edit:
there are extensions from glm to implicitly convert like
#include <glm/gtx/scalar_multiplication.hpp>
but they are unnecessary if we add the correct types, so we can include less includes in other includes

@artificiel
Copy link
Contributor

@dimitre i am getting more confused

  1. can you post a complete compilable snippet about your /3 vs /3.0 case?

  2. what do you mean by "removing implicit conversion"?

  3. "OF is in fact extending glm to allow some more interoperability there" — can you link to the source that extends glm?

  4. as it is stated above that glm does not implement vec2 -> 3 because there is no clear default that makes sense. however it implements 3->2 because slicing is evident:

	glm::vec3 v3 {1,1,1};
	glm::vec2 v2 = v3;
	glm::vec3 v3b = v2;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants