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

BinaryExporter resets buffer positions to 0 when writing them. #2312

Open
JosiahGoeman opened this issue Sep 18, 2024 · 2 comments
Open

BinaryExporter resets buffer positions to 0 when writing them. #2312

JosiahGoeman opened this issue Sep 18, 2024 · 2 comments
Labels
defect Something that is supposed to work, but doesn't. Less severe than a "bug"

Comments

@JosiahGoeman
Copy link
Contributor

When a buffer or buffer arraylist is written using BinaryExporter, the buffer's position gets reset to zero. XMLExporter handles this by restoring the old position after writing. This seems easy enough to fix. It does however bring up another issue currently masked by the position clobbering: shouldn't the position also be serialized? If a buffer with a non-zero position is serialized and the position is preserved, the deserialized buffer will not equals() the original because it will have a zero position. I can easily fix this for XMLExporter with an additional attribute, but from my brief look at BinaryOutputCapsule, it looks like this can't be changed because only one int (length) is expected before the start of the contents.

BinaryExportBufferPositionMRE.java
package com.mygame;

import com.jme3.export.JmeExporter;
import com.jme3.export.JmeImporter;
import com.jme3.export.OutputCapsule;
import com.jme3.export.Savable;
import com.jme3.export.binary.BinaryExporter;
import java.io.IOException;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import org.lwjgl.BufferUtils;

public class BinaryExportBufferPositionMRE
{
	static class SavableWithBuffer implements Savable
	{
		@Override
		public void write(JmeExporter je) throws IOException
		{
			ByteBuffer bb = BufferUtils.createByteBuffer(16);
			bb.position(4);
			
			OutputCapsule capsule = je.getCapsule(this);
			capsule.write(bb, "buffer", null);
			
			if(bb.position() != 4)
			{
				throw new IOException("Expected buffer position 4, got position: " + bb.position());
			}
		}

		@Override
		public void read(JmeImporter ji) throws IOException
		{
			
		}
	}

	public static void main(String[] args)
	{
		BinaryExporter be = BinaryExporter.getInstance();

		try
		{
			be.save(new SavableWithBuffer(), OutputStream.nullOutputStream());
		} catch(IOException e)
		{
			System.err.println(e);
		}
	}
}
@stephengold
Copy link
Member

JMonkeyEngine is often selective about which attributes it serializes. Buffer position is ignored in many contexts, so I think it's acceptable to skip serializing it. In contexts where the position is significant, developers can serialize it explicitly.

Similar arguments could be made about serializing a buffer's mark, limit, byte order, and whether it's read-only and/or direct.

It would be good to document (in the javadoc) which attributes of a buffer are automatically serialized.

@JosiahGoeman
Copy link
Contributor Author

JosiahGoeman commented Sep 18, 2024

Gotcha. I have changed the behavior for XMLExporter to save the position in the referenced PR, so I'll need to revise that.

JosiahGoeman added a commit to JosiahGoeman/jmonkeyengine that referenced this issue Sep 19, 2024
JosiahGoeman added a commit to JosiahGoeman/jmonkeyengine that referenced this issue Sep 19, 2024
JosiahGoeman added a commit to JosiahGoeman/jmonkeyengine that referenced this issue Sep 19, 2024
stephengold pushed a commit that referenced this issue Oct 25, 2024
* #2176 Make LWJGLBufferAllocator use nmemCalloc() instead of nmemAlloc()

#2176
LWJGLBufferAllocator.allocate() now always returns zero-initialized buffers.

* Added unit tests for JmeExporter/JmeImporter implementations

Tests all write* and read* methods of OutputCapsule and InputCapsule respectively.

* Fixed XMLExporter/XMLImporter BitSets

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.

* Fixed DOMInputCapsule.readString() returning defVal for empty strings

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.

* Deprecated DOMSerializer in favor of javax.xml.transform.Transformer

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.

* Fixed NullPointerException when XMLExporter writes a String[] with null

Also refactored all primitive array write and read methods to be more readable and reduce duplicate code.

* Made DOM capsules reuse write/read primitive array methods for buffers

Further reduces duplicate code

* Fixed DOMOutputCapsule.write(Savable[][]) NullPointerException

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.

* Added Savable reference consistency test to InputOutputCapsuleTest

* Fixed DOMInputCapsule throwing NullPointerException when reading list

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.

* Made XMLExporter save and load buffer positions properly.

* Cleanup and formatting for XMLExporter related classes

* Undid XMLExporter saving buffer positions.

Not saving positions is intentional #2312 (comment)

* Fixed infinite recursion with XMLExporter

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.
@stephengold stephengold added the defect Something that is supposed to work, but doesn't. Less severe than a "bug" label Oct 28, 2024
@stephengold stephengold added this to the Future Release milestone Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Something that is supposed to work, but doesn't. Less severe than a "bug"
Projects
None yet
Development

No branches or pull requests

2 participants