SingleLayerInfluenceMask: fix serialization issues

[capdevon]

see #2317

[yaRnMcDonuts]

Is there an inherent reason that the serialization issue cannot be fixed?

I am not an expert in serialization, so I apologize if I'm overlooking something obvious here

But wouldn't it resolve the issue if AnimComposer and SkinningControl get removed as class variables, and instead get passed in as paramters in the methods that use them?

That would of course still break a few lines of code for anyone currently using this, but that may be less intrusive than eventually deleting it altogether.

[codex128]

The issue would indeed be resolved if AnimComposer and SkinningControl were removed. The problem is that AnimComposer is being used to check if any higher layers are using a joint. Without it, SingleLayerInfluenceMask is useless.

SkinningControl I think could be removed. A lot of neat utility methods would have to go with it, but it can probably be removed anyway.

[yaRnMcDonuts]

A lot of neat utility methods would have to go with it, but it can probably be removed anyway.

What if you change those methods to take the AnimComposer or SkinningControl as a method paramater? This would allow that method to use AnimComposer without needing AnimComposer as a class variable:

public void makeLayer(AnimComposer animComposer) { animComposer.makeLayer(layer, this); }

You would then obviously have to change some of your own code that calls these methods to pass in the new AnimComposer param. But this way these non-serializable variables would no longer need stored in this class that needs serialized

[yaRnMcDonuts]

Hi @codex128 , I'm curious if you have considered my suggestion? I'd be willing to keep this class in the engine if you can make the changes I suggested to resolve the serialization issues. Or is there something more to this issue I am possibly missing that makes my suggestion invalid?

Removing it would be the last resort, and there's a good case for why this may not have been a good addition in the first place. But removing an existing class altogether isn't typically a great decision once users may be relying on it as you mentioned, so I think the optimal solution would be to fix the serialization issues so it doesn't cause any future issues.

[codex128]

Ah, yes, thanks for reminding me. It slipped my mind and I've been rather busy lately. I've submitted #2376 that incorporates your suggestion.

[capdevon]

Hi guys,
I've edited the PR to not deprecate the class, you can consider it a revision of @codex's PR #2372 to fix the serialization issues already discussed.

• I have rewritten the SingleLayerInfluenceMask class to adhere to SOLID programming principles and removed all problematic methods.
• I have optimized the variable names, the code and the javadoc to make it clearer and simpler.
• I removed all methods for adding Joints to the mask because they add complexity to future maintenance and are already present in the ArmatureMask superclass.
• I rewrote the TestSingleLayerInfluenceMask class more clearly, adding the missing SingleLayerInfluenceMask serialization test.

Please let me know your thoughts.

@yaRnMcDonuts @codex128

[codex128]

It looks fine to me.

I removed all methods for adding Joints to the mask because they add complexity to future maintenance and are already present in the ArmatureMask superclass.

I would only suggest considering keeping addAll or moving it to ArmatureMask, because it is new and useful functionality. If I remember correctly the alternative is to use addFromJoint on the root joint, which is little bit of a hassle. I'm ok with not keeping it, though.

[capdevon]

I would only suggest considering keeping addAll or moving it to ArmatureMask, because it is new and useful functionality. If I remember correctly the alternative is to use addFromJoint on the root joint, which is little bit of a hassle. I'm ok with not keeping it, though.

The addAll method is not necessary. The constructor of the ArmatureMask class with the Armature parameter does exactly the same thing. I've updated the test class to highlight this detail.

@yaRnMcDonuts if you also agree, I propose to move forward with this solution.

[yaRnMcDonuts]

That sounds good to me!

[yaRnMcDonuts]

If everything looks good to both @capdevon and @codex128 and if there is nothing else you plan to change, then I will merge this in about 48 hours. Thanks to both of you for collaborating on this to find a good solution!