glm migration and backward-compatibility strategies / testing

[kylemcdonald]

It seems like glm is happy converting vec3 to vec2 via truncation, but it won't automatically convert vec2 to vec3 by adding a zero. I know this has been a contentious topic in the past (e.g., vec3 to vec4 might require a one instead of a zero) but I'm running into a lot of code that will break because of this. For example, adding ofMesh::addVertex(ofVec2f) now fails.

If there are no plans to change this behavior, it would be great to add it as another note in the http://openframeworks.cc/learning/02_graphics/how_to_use_glm/ tutorial!

[bakercp]

Agreed. I'm 101% excited about glm, but some extra documentation here would be good. I've run into this a lot with ofPolyline in the last days. In the past we have been very non-strict with our use of 2d and 3d points. For instance, our ofRectangle and ofPolyline are built on 3d vectors, but many of their member operations only are only valid for 2d operations

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofPolyline.h#L370-L378

What does bool ofPolyline::inside(const glm::vec3 & p) const; mean :) ?

And ofPolyline::fromRectangle(const ofRectangle& rect) doesn't actually take the dubious ofRectangle::position.z component into account.

... and lots of warnings may not be a good substitute for stricter compile-time errors

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/types/ofRectangle.h#L14-L15

Anyway, I'd argue that we should make ofRectangle a 2D rectangle and we should make an ofPolyline2f (or something), to make all of this clearer, but admittedly this does make oF less readable. But with the right combination of enable_if, etc and the newly-templated ofPolyline may make this pretty easy to implement.

[roymacdonald]

Hi, @kylemcdonald +1 on this. I'm not sure if I'm excited as you are @bakercp with glm as it gave me a huge head ache on a recently made project. As for what you mention of the polyline, although it is not correct in one sense it is super handy on the other. I really like ofRectangle, I use it a lot and I have never faced a problem because its position is a 3d vec . I'm not sure if making it 2d will bring problems but it will certainly break code that expects a 3d vec.

bool ofPolyline::inside(const glm::vec3 & p) const; this checks if the passed vec falls inside the polyline, just like in ofRectangle. :)

[bakercp]

@roymacdonald I'm not excited about the transition, I too have spent hours updating code -- but I think it's a better more flexible codebase and is worth the effort. Agreed regarding the ease of use with ofRectangle and ofPolyline. Perhaps my (over) engineering brain just needs to chill out :)

[roymacdonald]

@bakercp sure, I completely understand you and agree with you. I think that your over engineering brain comes out with some really great things, which have helped me more times that I can remember. So, I thank your brain for that (and send my regards to it). ;)

[kylemcdonald]

Working with this a bit more recently, the best solution I've found is to switch lines like mesh.addVertex(ofVec2f(x, y)); to mesh.addVertex(ofVec3f(x, y)); and avoid referring to glm or manually adding the trailing zero.

[arturoc]

the shortest is mesh.addVertex({x, y, 0}); and would also work for both ofVec and glm

[bakercp]

So just an updated on this after more experience and time --

Currently the switch to glm (which I believe is the best decision and support it 100%) means that 90% of the addons my students and I have encountered this past semester (we are using the develop branch ⚡️ 😨 in class) require a huge amount of work to update. The number #1 issue is that ofPolyline and ofMesh no longer have methods for implicitly accepting 2D vectors. ofVec2f has no problem converting to glm::vec2 via its operator, but the newly templated ofPolyline and ofMesh can't quietly accept glm::vec2 vertices because they are more strict (ofVec3f was quietly and implicitly constructed from an ofVec2f).

I've been experimenting with a few options and the one that seems to work the best is adding (and simultaneously marking as deprecated?) functions like this to ofPolyline_ in each location where a 3d vertex is expected.

To add glm::vec2 to an ofMesh of ofPolyline templated on

template<class U, typename std::enable_if<std::is_same<U, glm::vec2>::value, int>::type = 0>
void addVertex( const U& p ) { addVertex(p.x, p.y, 0); }

Alternatively, leaving things as they are would provide a very nice filter for automatically selecting up-to-date addons from ofxaddons and marking them as incompatible. :)

[arturoc]

isn't that the same as:

void addVertex( const glm::vec2& p ) { addVertex(p.x, p.y, 0); }

but any of those will fail to compile if you try to define an ofPolyline_<glm::vec2> no? i think what we would need is something that only enables that method if T==glm3

typename std::enable_if<std::is_same<T, glm::vec3>::value, void>::type
addVertex(const glm::vec2 & p){...}

something that would be also useful is to allow the renderers to draw ofPolyline_<glm::vec2> which we could typedef to ofPolyline2d or similar. that also would made the internal math 2d right away which could make some things faster

[bakercp]

Yes, effectively it is the same. That said, it isn't the first thing to fail because you can't currently template on 2d vectors like ofPolyline_<ofVec2f> or ofPolyline_<glm::vec2> because ofPolyline_ has so many 3-parameter constructors in the implementation (i.e. it assumes that T is a vec3 or some kind). It's basically hard-coded to be 3D or expect the constructor T(x,y,z) to exist. (btw I built out and taste your solution and FWIW, the c++11 compiler didn't like your solution, but I don't want to paste the errors here).

So, at this point, without changing ofPolyline_ to handle 2D vectors, the simplest solution would just be to add

/// \brief Adds a point using floats at the end of the ofPolyline.
void addVertex(const glm::vec2& p);
/// \brief Adds a point using floats at the end of the ofPolyline.
void addVertex(const ofVec2f& p);

... but because ofVec2f / ofVec3f now have operator overloads for glm::vec types, adding these methods becomes ambiguous. The only solution I've found that is unambiguous and will also be compatible with ofPolyline_<ofVec2f> or ofPolyline_<glm::vec2> in the future is:

template<class U, typename std::enable_if<
	(std::is_same<T, glm::vec3>::value || std::is_same<T, ofVec3f>::value) &&
	(std::is_same<U, glm::vec2>::value || std::is_same<U, ofVec2f>::value), int>::type = 0>
void addVertex( const U& p ) { addVertex(p.x, p.y); }

Shall I PR this and related functions (we need to build out similar functions for all locations where T is an input parameter ... ).

[bakercp]

Here's the bit of code I've been using for testing ...

    void test_glm()
    {
        glm::vec2 g_v_2;
        glm::vec3 g_v_3;
        ofVec2f o_v_2;
        ofVec3f o_v_3;

//        {
//            ofPolyline_<glm::vec2> poly;
//            poly.addVertex(g_v_2);
////            poly.addVertex(g_v_3); // failing, but this is not acceptable
////                                   // because it throws away data.
//            poly.addVertex(o_v_2);
////            poly.addVertex(o_v_3); // failing, but this is not acceptable
////                                   // because it throws away data.
//
//            poly.addVertex(o_v_2.x, o_v_2.y);
//            poly.addVertex(o_v_3.x, o_v_3.y, o_v_3.z);
//
//        }


        {
            ofPolyline_<glm::vec3> poly;
            poly.addVertex(g_v_2);
            poly.addVertex(g_v_3);
            poly.addVertex(o_v_2);
            poly.addVertex(o_v_3);
        }

//        {
//            ofPolyline_<ofVec2f> poly;
//            poly.addVertex(g_v_2);
//            poly.addVertex(g_v_3);
//            poly.addVertex(o_v_2);
//            poly.addVertex(o_v_3);
//        }

        {
            ofPolyline_<ofVec3f> poly;
            poly.addVertex(g_v_2);
            poly.addVertex(g_v_3);
            poly.addVertex(o_v_2);
            poly.addVertex(o_v_3);
        }
    }

[arturoc]

i think being able to use a 2d only polyline is pretty useful and the methods that accept x,y,z can just be enable_if'd to not exist if the type is glm2

by now i would just add:

/// \brief Adds a point using floats at the end of the ofPolyline.
void addVertex(const glm::vec2& p);

ofVec2f will get auto converted to glm::vec2 anyway and that's what we are using in other places like ofRectangle or all the ofGraphics functions.

if/when we fix ofPolyline to work as 2d we can enable_if'd this methods and anything else that doesn't make sense in 2d

[bakercp]

Totally agree on the polyline2d. It's a good goal. There are a lot of functions in there (as noted above) that don't make a ton of sense in 3d. Same goes for ofRectangle, etc. That said your solution

void addVertex(const glm::vec2& p);

Is ambiguous under these conditions (which is why I had to come up with that odd templated function):

glm::vec2 g_v_2;
glm::vec3 g_v_3;
ofVec2f o_v_2;
ofVec3f o_v_3;

ofPolyline_<ofVec3f> poly;
poly.addVertex(g_v_2);
poly.addVertex(g_v_3);
poly.addVertex(o_v_2); // << Ambiguous b/c ofVec3f has a ofVec3f(const ofVec2f& v) constructor.
poly.addVertex(o_v_3);

[arturoc]

mmh can you just add all of those then at the end of the class ifdef'd with

#ifndef OF_USE_LEGACY_MESH

I guess it's safe to assume that nobody is going to explicitly use an ofPolyline_<ofVec3f> only if it's enabled through the legacy mode in ofConstants

[arturoc]

mainly i would like to avoid such weird syntax if possible

[bakercp]

Totally agree. I'll do it.

[bakercp]

I submitted two PRs that significantly improve the situation. Please check them out.

[armadillu]

Trying to instantiate a ofPolyline_<glm::vec2> fails due to a default argument in setRightVector() having 3 components:

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

Seeing ofPolyline is now templated, I'm assuming it should support ofPolyline_glm::vec2? Or its just not meant to be used like this?

[tgfrerer]

The way ofPolyline is defined currently, only 3-component vector types will compile - calculations are done in 3D space. (We make use of cross-products (to calculate normals for example), and quite a few internal methods access the .z component).

[arturoc]

I have this working in this branch: https://github.com/arturoc/openFrameworks/tree/refactor-removeInls where i moved ofPolyline and ofMesh to cpp while also changing the necesary bits to make them work with 2d and let the renderers accept the 2d versions as well as the 3d ones.

The problem is that some template features i'm using on that branch don't seem to work on clang or visual studio or both (don't remember the details right now).

I started working on that as an attempt to bring down compile times by moving the inl to cpp of these two classes but since that didn't really make it much better i left it after until 0.10 is released and i'll try to make it work in the missing platforms.

[dimitre]

Today I experimented with removing all legacy math from Core, just to know what needed to be changed in Core, Examples and Addons.

• https://github.com/openframeworks/openFrameworks/pull/8178

I think today some interoperability between glm::vec2 -> glm::vec3 is achieved through some indirect mean like glm::vec2 -> ofVec2f -> ofVec3f -> glm::vec3 which is not ideal.

I think we should not use implicit conversion in OF Core, but if we need we can add operators to glm itself like this:

//--------------------------------------------------------------
inline glm::vec3 operator+(const glm::vec3 & v1, const glm::vec4 & v2){
	return v1 + glm::vec3(v2.x, v2.y, v2.z);
}

// addons/ofxGui/src/ofxButton.cpp:58:65
//--------------------------------------------------------------
inline glm::vec2 operator+(const glm::vec2 & v1, const glm::vec3 & v2){
	return v1 + glm::vec2(v2.x, v2.y);
}