Skip to content

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

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

Conversation

capdevon
Copy link
Contributor

@capdevon capdevon commented Jun 25, 2025

This PR refactors and improves the Matrix3f class in several ways:

  • Cleans up and clarifies import statements.
  • Improves variable naming for exporters/importers and capsules for better readability.
  • Updates copyright years.
  • Improves column normalization and cross product calculation comments for clarity.
  • Refactors methods to use consistent parameter names and deprecates inconsistent API methods in favor of better-named variants.
  • Replaces hardcoded float tolerances with a named variable for easier maintenance.
  • Updates exception messages and JavaDocs for accuracy.
  • Refactors equality checks and serialization/deserialization logic for clarity and maintainability.
  • Cleans up commented-out legacy code and improves code style.

These changes enhance code readability, consistency, and maintainability without altering core functionality.

@richardTingle
Copy link
Member

Similar to the quaternion class I dont think we should be exposing internal implementation details unless we have a really good use case

@capdevon
Copy link
Contributor Author

Regarding the proposal to directly expose fields in Quaternion and Matrix3f, I believe this is a fundamental step toward optimizing crucial aspects of our engine and aligning with an already established design pattern.

Core math classes like Vector2f, Vector3f, Vector4f, ColorRGBA, Ray, and Matrix4f already expose their variables via public modifiers. This wasn't an arbitrary design choice; it has proven extremely effective and is widely used throughout jme3-core (especially by Material parsers and ShadowRenderers), as well as by our entire developer community.

All these classes are declared as final. This means they cannot be extended, which makes typical arguments related to inheritance and encapsulation via getters/setters less pertinent in this context. Altering this structure now would mean breaking compatibility with all current and past jME projects—a risk none of us want to take.

Bringing Quaternion and Matrix3f into this consistent design is a logical progression. It's not just about aesthetics; it's about enabling the performance and direct usage advantages I've already detailed, such as reduced allocations and more efficient access. Even without precise measurements, any native optimization can positively contribute to overall performance, which is crucial for achieving high frame rates on less powerful hardware like smartphones or mid-range devices.

The primary benefit of this adjustment is the ability to have direct variable access in high-computation areas such as Material parsers, advanced development tools, and demanding shadow computation functions, where direct field access in our existing math classes is already instrumental and widely exploited.

I understand that strict adherence to getters/setters is generally considered good programming practice. However, in the context of final math classes within a game engine like jME, where the priority is efficiency and the consistency of an already established API, opposing this change purely for formal reasons becomes counterproductive. The consistency with the other six final classes already following this pattern, combined with the tangible benefits in performance and usability, makes this proposal not just valid, but necessary.

@yaRnMcDonuts
Copy link
Member

yaRnMcDonuts commented Jun 27, 2025

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.

@capdevon capdevon closed this Jun 27, 2025
@capdevon capdevon reopened this Jun 30, 2025
@capdevon capdevon changed the title Matrix3f: making mXX fields public Matrix3f: refactoring and minor API improvements Jun 30, 2025
@capdevon
Copy link
Contributor Author

capdevon commented Jun 30, 2025

The protected class variables have been restored. If you notice any technical errors or potential bugs in the proposed changes, please let me know.

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.

3 participants