Fix for XMLExporter issues in #2310

[JosiahGoeman]

trafficstars

Includes fixes for all issues listed in https://github.com/jMonkeyEngine/jmonkeyengine/issues/2310 and refactors related classes for readability, adherence to style guide, and elimination of duplicate code. Also adds unit tests for implementations of the OutputCapsule and InputCapsule interfaces. Currently one test is commented out because BinaryExporter fails it, see https://github.com/jMonkeyEngine/jmonkeyengine/issues/2312.

[JosiahGoeman]

Wait, why did GitHub include the BufferAllocator commit from my previous pr? That commit is already in the target branch.

[stephengold]

I'm unsure what happened. It might have something to do with creating a pull request in your "master" branch. Does the "files changed" tab look correct to you?

[JosiahGoeman]

Yes, the files changed are correct.

[JosiahGoeman]

Alright, I think this should be ready for review.

[stephengold]

If there's no substantive review or discussion, I'll run some simple tests of my own and then integrate this PR.

[stephengold]

My tests turned up an issue that's probably related to this change. I'll troubleshoot and post my findings here.

[stephengold]

The issue I'm seeing is an infinite recursion that occurs while writing a Minie PhysicsVehicle to an XMLExporter. However, I don't fully understand what's going wrong...

The test app looks like this:

        NativeLibraryLoader.loadNativeLibrary("bulletjme", true);
        CollisionShape shape = new SphereCollisionShape(1f);
        PhysicsVehicle body = new PhysicsVehicle(shape, 1f);

        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
        XMLExporter exporter = new XMLExporter();
        try {
            exporter.save(body, outputStream);
        } catch (IOException exception) {
            throw new RuntimeException(exception);
        }

The infinite recursion is as follows: PhysicsVehicle.write() line 846 invokes PhysicsRigidBody.write(). At line 1442, that invokes DOMOutputCapsule.write(). At line 406, that invokes RigidBodyMotionState.write(). At line 366, that invokes DOMOutputCapsule.write(). And at line 406, that invokes PhysicsVehicle.write() again.

There's only one PhysicsVehicle object in the test app, but it appears DOMOutputCapsule has forgotten it's already in the process of writing that PhysicsVehicle. The cycle repeats many times, until eventually the test app crashes with a StackOverflowError.

The same test app works fine if I replace XMLExporter with BinaryExporter.

[codex128]

I took a quick peek at it, and it looks like PhysicsVehicle contains a PhysicsRigidBody, which it attempts to save. The PhysicsRigidBody contains a RigidBodyMotionState, which it also attempts to save. Finally, the RigidBodyMotionState contains the original PhysicsVehicle, which it also attempts to save, and the process starts all over again.

I don't know why it only fails with XMLExporter, but I'd guess that BinaryExporter maybe has some sort of check for that sort of dependency issue.

[stephengold]

I'd guess that BinaryExporter maybe has some sort of check for that sort of dependency issue.

The test succeeds with XMLExporter in v3.7.0-stable, which means there was also a check in XMLExporter, and this PR disabled that check. I'm hoping @JosiahGoeman will solve this for us.

[JosiahGoeman]

I'd guess that BinaryExporter maybe has some sort of check for that sort of dependency issue.

The test succeeds with XMLExporter in v3.7.0-stable, which means there was also a check in XMLExporter, and this PR disabled that check. I'm hoping @JosiahGoeman will solve this for us.

Whoops, I think I see what the issue is. DOMOutputCapsule uses an IdendityHashMap to keep track of unique Savable instances and their corresponding DOM elements, so they only get written once and their reference network is preserved. It looks like in the DOMOutputCapsule.write(Savable value, String name, Savable defVal) method I switched the order of writing the Savable's contents and putting it in the map. So any reference loop will cause infinite recursion. I don't remember there being a reason why I did that, so it might just a matter of switching those calls back the way they were. I'll get on it.

[stephengold]

Thank you for your quick response. How about adding a reference loop to jme3-plugins/src/test ?

[JosiahGoeman]

Looks like it was indeed that simple. And here I was thinking my test suite was so thorough. -_- I added a test for the reference-loop case.

[stephengold]

Thanks, @JosiahGoeman . 7f522dc passes my tests as well, so I think this PR is ready for integration---unless someone cares to review it.