Skip to content

Update SeparateJointModelTransform.java #2409

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 1 commit into
base: master
Choose a base branch
from

Conversation

utsavsavani01
Copy link

Avoid mutating input

@@ -30,9 +30,12 @@ public void getOffsetTransform(Matrix4f outTransform, Matrix4f inverseModelBindM

@Override
public void applyBindPose(Transform localTransform, Matrix4f inverseModelBindMatrix, Joint parent) {
localTransform.fromTransformMatrix(inverseModelBindMatrix.invert());
Matrix4f safeInverseBind = inverseModelBindMatrix.clone().invert(); // avoid mutating input
Copy link
Contributor

@capdevon capdevon Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Matrix4f.invert() method already creates a new object without altering the input object, so there is no need to clone the input matrix before inverting it.

if (parent != null) {
localTransform.combineWithParent(parent.getModelTransform().invert());
Transform safeParentTransform = parent.getModelTransform().clone().invert(); // same here
Copy link
Contributor

@capdevon capdevon Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Transform.invert() method already creates a new object without altering the input object, so there is no need to clone the parent model Transform before inverting it.

@capdevon
Copy link
Contributor

capdevon commented Apr 23, 2025

Hi @utsavsavani01 ,
You don't need to clone a Matrix4f before inverting it because the invert() method already produces a new, independent result.
https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-core/src/main/java/com/jme3/math/Matrix4f.java#L1484

You don't need to clone a Transform before inverting it because the invert() method already produces a new, independent result.
https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-core/src/main/java/com/jme3/math/Transform.java#L424

Edit:
To properly evaluate this change, please explain the problem you are trying to solve and provide details on how the results were tested. Otherwise, I suggest closing this PR.

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