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

Minor refactor to allow for saving AnimLayers #2307

Merged
merged 5 commits into from
Sep 1, 2024

Conversation

neph1
Copy link
Contributor

@neph1 neph1 commented Aug 22, 2024

While working on this jMonkeyEngine/sdk#612 I found out that AnimLayers couldn't be saved.

Tested in local SDK.

I'll look it over once more before turning into a proper PR.

The tests don't explicitly test the changes, just general AnimComposer behavior.

* The composer that owns this layer. Were it not for cloning, this field
* would be final.
*/
private AnimComposer composer;
Copy link
Contributor Author

@neph1 neph1 Aug 22, 2024

Choose a reason for hiding this comment

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

redundant field

Copy link
Contributor Author

@neph1 neph1 Aug 22, 2024

Choose a reason for hiding this comment

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

I decided to only save name and mask. The other fields felt like they're only interesting for runtime.

@pspeed42
Copy link
Contributor

Note that in my case, it's actions that give me heartburn for not being persisted. But my memory is that there are more demons down that rabbit hole.

But it does mean that for every model rig I load, I have to do a loop like this with my own external fix-up config:

            for( BlendInfo blend : blends ) {
                BlendAction ba = anim.actionBlended(blend.getName(), new LinearBlendSpace(0, 1),
                                                    blend.getBlendedActions());
                ba.getBlendSpace().setValue((float)blend.getDefaultBlend());
            }

Actions enter this weird area where I can see defining them one-off in code to do some particular thing... while also wanting to preconfigure some with masks and blending and so on. But also, every other thing in JME operates this way... if I tweak a "runtime to me" material parameter and then save that j3o then it gets saved. So I'm not sure I buy arguments against saving them.

@neph1
Copy link
Contributor Author

neph1 commented Aug 25, 2024

If I can find a use case from the SDK's perspective, I don't mind making Actions Savable too (I actually started it). But atm I don't see exactly how you would use them. In any case, that's for another PR.

@neph1 neph1 marked this pull request as ready for review August 25, 2024 17:53
@capdevon
Copy link
Contributor

capdevon commented Aug 25, 2024

To enhance clarity and facilitate review, please submit the optimizations as a separate pull request (PR). This will prevent confusion and simplify potential rollbacks. By separating these changes, you can maintain the flexibility to revert optimizations without affecting the core functionality.

Edit:
Please excuse me if I seem overly formal. I'm not trying to be harsh. After years of reviewing software, I'm simply applying the guidelines outlined in the JME code of conduct.
Thank you for your work and understanding

@capdevon
Copy link
Contributor

Ok, everything looks good to me. Thank you.

@neph1
Copy link
Contributor Author

neph1 commented Aug 31, 2024

Does anyone have to officially approve it? Or do I just merge it?

@stephengold
Copy link
Member

Does anyone have to officially approve it? Or do I just merge it?

The project isn't very formal these days. Merge when ready.

@neph1 neph1 merged commit db63530 into jMonkeyEngine:master Sep 1, 2024
14 checks passed
@stephengold stephengold added this to the Future Release milestone Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants