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

ofVec and ofMatrix wrapping glm #5449

Closed
roymacdonald opened this issue Feb 14, 2017 · 6 comments
Closed

ofVec and ofMatrix wrapping glm #5449

roymacdonald opened this issue Feb 14, 2017 · 6 comments

Comments

@roymacdonald
Copy link
Contributor

Hi,
so as glm is now part of the OF core I have gotten into some really nasty issues dealing with code compatibility, in particular with addons.
So I was thinking about writing a sort of wrapper for glm that would not break older code.
The idea is to make ofVec and ofMatrix to inherit from their respective glm counterparts and make their methods to simply wrap the glm functions. This way, any code that used ofVec or ofMatrix would "see" no difference although all the inner work is done by glm.

How does this sound?
Is it worth the effort?
I already have some of it done, so I wouldn't have any problem on making this for all the math classes that now use glm.

cheers!

@arturoc
Copy link
Member

arturoc commented Feb 14, 2017

i don't think this makes sense for older projects compatibility the best is to enable ofVec* as default in ofConstants and keep using ofVec/ofMatrix for new projects just use glm everywhere. trying to wrap glm with ofVec seems like it would make maintenance in the future even harder and make the transition to glm longer

@roymacdonald
Copy link
Contributor Author

the thing is that by enabling the legacy vec in ofConstants.h the only thing that changes is ofMesh and ofPolyline, but there are a lot of other parts where ofVec was being used that are now glm::vec, which are actually where the conflict arises. But even if all these parts that now use glm::vec are changed to ofDefaultVec there would be problems with the functions related to ofVec, and this is why I'm proposing a wrapper. This wrapper could even have a deprecation flag so in the future we can finally get rid of ofVec.
further more, the ones using the master branch know from a while now about the switch to glm although the users that just download the release versions will have to face a really annoying scenario once glm becomes part of the release version of OF. as of now the transition to glm is not smooth at all, I'm just proposing to make it smoother than it is now, which will save us from a lot of headaches.
cheers!

@arturoc
Copy link
Member

arturoc commented Feb 14, 2017

the main problem with transitioning to glm was returning or accepting as parameter vector<glm::vec?> vs vector<ofVec?> since those can't be autoconverted. everything else autoconverts from ofVec? to glm::vec? and viceversa so passing or getting a vector or matrix to/from OF core should work right away. there's still a couple of cases that will fail, trying to call an ofVec? method directly on a return method like:

camera.getLensOffset().getRotated(...);

that's the only line of code in all the OF examples that needed to be changed when compiling in compatibility mode and can be easily solved like:

ofVec3f offset = camera.getLensOffset();
offset.getRotated(...);

the other case is when a function accepts a glm::vec3 and you try to pass a vec2 but for all such cases in the core we've replicated every function to accept both vec2 and vec3 so it shouldn't be a problem either and if there's any missing case we can just add the corresponding overload.

what are the specific trouble you found? also i'm not very clear on how an ofVec wrapper around glm would make things easier as opposed to just keep using ofVec, can you put a concrete example?

@roymacdonald
Copy link
Contributor Author

Hi,
I was dealing with all this glm stuff like 2 weeks ago so I cant recall clearly now. Although, I remember that it was not autoconverting both ways, only one. I had a ton of errors regarding glm.
Let me dig a little bit to find out those problems, isolate them and post.
cheers

@bakercp
Copy link
Member

bakercp commented May 20, 2017

I'm going to close this issue in and move the discussion here: #5440

@bakercp bakercp closed this as completed May 20, 2017
@roymacdonald
Copy link
Contributor Author

roymacdonald commented May 20, 2017

Sure! thanks

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