Fix for XMLExporter issues in #2310 #2313
Conversation
… of nmemAlloc() jMonkeyEngine#2176 LWJGLBufferAllocator.allocate() now always returns zero-initialized buffers.
Tests all write* and read* methods of OutputCapsule and InputCapsule respectively.
Previously DOMOutputCapsule was writing indices of set bits and DOMInputCapsule was reading values of each bit. Changed DOMOutputCapsule to match the expected behavior of DOMInputCapsule.
org.w3c.dom.Element.getAttribute() returns an empty string for attributes that are not found. It looks like DOMInputCapsule.readString() was interpreting an empty string as the attribute not existing, and returning defVal even when the attribute did exist and was an empty string. Now it checks explicitly whether the attribute exists.
DOMSerializer contains several edge-case issues that were only partially worked around with the encodeString() and decodeString() helper methods. Java has a robust built-in solution to serializing Document objects, and using that instead fixes several bugs.
Also refactored all primitive array write and read methods to be more readable and reduce duplicate code.
Further reduces duplicate code
Refactored write and read methods for Savables and 1 and 2 dimensional Savable arrays. Fixed NullPointerException when writing a 2d Savable array containing a null element in the outer array.
DOMInputCapsule used to throw a NullPointerException when reading an Arraylist containing a null element. Also refactored list write and read methods to clean up a bit and accidentally also fixed an unrelated bug where reading ArrayList<ByteBuffer> would return a list containing all null elements.
|
Wait, why did GitHub include the BufferAllocator commit from my previous pr? That commit is already in the target branch. |
|
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? |
|
Yes, the files changed are correct. |
stephengold
added
the
defect
JosiahGoeman
force-pushed
the
master
branch
from
45a0cf6
to
9500567
Compare
Not saving positions is intentional jMonkeyEngine#2312 (comment)
JosiahGoeman
force-pushed
the
master
branch
from
9500567
to
0629af5
Compare
|
Alright, I think this should be ready for review. |
|
If there's no substantive review or discussion, I'll run some simple tests of my own and then integrate this PR. |
|
My tests turned up an issue that's probably related to this change. I'll troubleshoot and post my findings here. |
|
The issue I'm seeing is an infinite recursion that occurs while writing a Minie 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: There's only one The same test app works fine if I replace |
|
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. |
The test succeeds with |
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 |
Writing a Savable containing a reference loop caused infinite recursion due to bookkeeping being performed after the recursive call instead of before. Also added a unit test for this to InputOutputCapsuleTest.
|
Thank you for your quick response. |
|
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. |
|
Thanks, @JosiahGoeman . |
Includes fixes for all issues listed in #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 #2312.