-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Matrix3f: refactoring and minor API improvements #2527
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
base: master
Are you sure you want to change the base?
Conversation
Similar to the quaternion class I dont think we should be exposing internal implementation details unless we have a really good use case |
Regarding the proposal to directly expose fields in Core math classes like All these classes are declared as Bringing The primary benefit of this adjustment is the ability to have direct variable access in high-computation areas such as I understand that strict adherence to getters/setters is generally considered good programming practice. However, in the context of |
My biggest concern is what @riccardobl mentioned about thinking the engine should go in the opposite direction, and that even the vector fields should not be public in the first place. I already question if there is any benefit of these changes, but his comment also raises the question that there could be downfalls as well. This type of design choice seems to ultimately be negligible and can vary from dev to dev based on coding preferences. Someone else may just come along and say they want to change this back again in the future if they have different preferences. For me, this falls int of the category of refactoring that seems very theoretical and has no clear benefit that I can see from my own real experiences using jme. (I'd have to see a real example of an app that could substantially benefit from these changes to be convinced) So I'd say this PR should be closed, especially due to the high volume of PRs right now that offer more objective benefits. Even if the debate continues and you prove that there is indeed a proven benefit, I still worry that it would be so minimal that is not worth the time debating over. If you still really disagree then you can keep this PR (or any similar ones that get closed/denied) open and it can be revisited in the future. But right now, my opinion would be to reject this PR. |
The |
This PR refactors and improves the
Matrix3f
class in several ways:These changes enhance code readability, consistency, and maintainability without altering core functionality.