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

Remove SingleLayerInfluenceMask due to Serialization Issues and Dependency Conflicts. A Custom Solution Not Suited for Core #2317

Open
capdevon opened this issue Oct 23, 2024 · 6 comments

Comments

@capdevon
Copy link
Contributor

capdevon commented Oct 23, 2024

The recent change to ArmatureMask implementing Savable has caused an issue with SingleLayerInfluenceMask (which extends ArmatureMask) serialization.
Due to its dependencies on AnimComposer and SkinningControl, serialization is now impossible and the class exhibits inconsistent behavior.

Given its reliance on these external controls and potential for conflicts, I believe SingleLayerInfluenceMask is a custom solution best suited for specific needs. Its presence in the jME engine core can lead to maintenance issues and hinder the future evolution of animation base classes.

We recommend its removal from the jME engine core.
NOTE: this class was introduced in the latest 3.7.0-stable release.

https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-core/src/main/java/com/jme3/anim/SingleLayerInfluenceMask.java#L42

@codex128
Copy link
Contributor

Hmm, that's a shame. I think it is a particularly useful class. Perhaps this would work, along with some documentation?

@Override
public void write(JmeExporter ex) throws IOException {
    throw new UnsupportedOperationException("Read/write not supported.");
}
@Override
public void read(JmeImporter im) throws IOException {
    throw new UnsupportedOperationException("Read/write not supported.");
}

If not, I can move SingleLayerInfluenceMask to a utility library.

@capdevon
Copy link
Contributor Author

Personally, I do not think this is an acceptable solution. This class is not necessary for the proper functioning of the engine. It is a utility class and should be in an external library.

To maintain flexibility and avoid potential conflicts, I propose that SingleLayerInfluenceMask be moved to an external library. This would allow for more focused development and maintenance, while also preserving its accessibility for those who find it useful.

@capdevon capdevon changed the title Removal of SingleLayerInfluenceMask from jME Core due to Serialization issues Remove SingleLayerInfluenceMask due to Serialization Issues and Dependency Conflicts. A Custom Solution Not Suited for Core Nov 20, 2024
@capdevon
Copy link
Contributor Author

Alternatively, I propose moving this class to the jme3-examples module, where it can serve as a basis for users who wish to optimize or adapt it for their projects.

@codex128
Copy link
Contributor

Not moving to examples, certainly, but moving to jme3-plugins might be a good idea. I've personally been very busy lately, so I do not have the time to do this myself, but you're welcome to open a PR for this.

What I am most concerned about is that SingleLayerInfluenceMask was already released with 3.7-stable, so it can't really be moved anymore without breaking backwards compatibility.

@capdevon
Copy link
Contributor Author

Not moving to examples, certainly, but moving to jme3-plugins might be a good idea.

I strongly disagree. Including it in the core module was a mistake, and moving it to jme3-plugins would be even worse, as that module is specifically for asset import/export.

What I am most concerned about is that SingleLayerInfluenceMask was already released with 3.7-stable, so it can't really be moved anymore without breaking backwards compatibility.

While the class is now in core, I suspect its usage has been limited since release. My GitHub searches found no public projects using it outside of jME itself. While not definitive, this suggests limited adoption.

The best solution, is to remove the class from jme3-core and relocate it to jme3-examples, where it would reside with other demonstrative utility classes (see package jme3test.model.anim). If there's disagreement with this approach, I'll close the issue and leave the class as is.

If other reviewers agree with this assessment, I'll submit a PR.

I'd especially appreciate input from experienced engine maintainers. I want to be clear that this isn't a criticism of your contribution, but rather a collaborative effort to improve the engine. Constructive feedback is essential for growth, and I believe this change would benefit the project in the long run.

@codex128
Copy link
Contributor

I want to be clear that this isn't a criticism of your contribution, but rather a collaborative effort to improve the engine.

Yes, I realize this, and thank you. I think your dedication to the engine's code quality is admirable and important. 👍

Unfortunately, I do not agree that it should be moved to examples, because it isn't an example; it's implementation meant to be used. Also, just because it hasn't been visibly used yet doesn't mean someone using 3.7 won't start using it in the future, or has already started using it that we don't know about. My stance at this point is to leave it in core to maintain backwards compatibility, even though it sucks, and make double sure something like it doesn't happen again.

That's just my opinion though. It'd probably be best for you to go ahead with opening a PR with the changes you're suggesting and get an experienced maintainer to take a look at it.

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

No branches or pull requests

2 participants