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

[Idea] - Remove legacy math from Core #8178

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

dimitre
Copy link
Member

@dimitre dimitre commented Nov 3, 2024

This is a proposition for discussion.
I was testing removing all legacy math (ofPoint, ofVecXf, ofMatrixXxX, ofQuaternion) from core, addons and examples. Now I have this updated PR where we can opt out from legacy math if desired.

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);
}

Thoughts? Ideas? cc @ofTheo @NickHardeman

@dimitre
Copy link
Member Author

dimitre commented Nov 3, 2024

@dimitre dimitre changed the title [Idea] - Test removing legacy math from core [Idea] - Remove legacy math from Core Nov 3, 2024
@roymacdonald
Copy link
Contributor

roymacdonald commented Nov 4, 2024

As far as I can recall, there was a lot of work and though put into how to fully go into using glm only but not breaking projects using OF vec classes. The solution at which we got is quite nice and it works (thus, I dont see the need to remove at all if it is not bothering). Having #ifdef OF_USE_LEGACY_MATH is nice, but I would leave it enabled by default and only if someone really knew what they are doing to disable. I tend to work with old projects and it really becomes painful and annoying when things dont work as they used to.

As for the examples, all those should have been converted to use glm long ago and I dont see any issue on doing so. (I suggest making a different PR just with the updated examples). But you need to test these and compare, visually that the behavior is the same. I remember seeing odd stuff, like with matrix decomposition.

I would keep using the toOf and toGlm functions within the core in order to enforce their usage and still keep compatibility, which essentially implies dont change much.

I think we should not use implicit conversion in OF Core,

Implicit conversion is actually nice and useful, what problems do you see with it?

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.

if it is possible to remove this, and make it direct it would be great.

As of the addons. I think it is ok to update their internal use but I would not change their signatures (public and protected members and methods) as that can potentially break projects. I would change those only using OF_USE_LEGACY_MATH is undefined.

This is a BIG change which I really think it needs to be taken with calm. there is no need to rush. there is no need to break what is already working.

Last but not least:
Please, make a new branch for this, it makes it a lot easier for other to check. And dont merge to master until agreed to do so.

@dimitre
Copy link
Member Author

dimitre commented Nov 4, 2024

@roymacdonald This PR should not change any functionality. (it needs testing)
what I did was finish porting all Core functions and examples to glm and having the option of not using "legacy math"

@roymacdonald
Copy link
Contributor

I know what you did, I checked your commits, and while seems like a tempting idea, it really needs to be checked and tested.

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

Successfully merging this pull request may close these issues.

2 participants