Skip to content

Preserve buffer positions during binary export#2821

Merged
riccardobl merged 3 commits into
jMonkeyEngine:masterfrom
ittaigolde:fix-binary-exporter-buffer-position
May 28, 2026
Merged

Preserve buffer positions during binary export#2821
riccardobl merged 3 commits into
jMonkeyEngine:masterfrom
ittaigolde:fix-binary-exporter-buffer-position

Conversation

@ittaigolde
Copy link
Copy Markdown
Contributor

Summary

  • Preserve original NIO buffer positions when BinaryOutputCapsule writes FloatBuffer, IntBuffer, ByteBuffer, and ShortBuffer values
  • Add a binary exporter regression test that verifies positions are unchanged and serialized contents still round-trip

Fixes #2312.

Testing

  • ./gradlew.bat :jme3-core:test --tests com.jme3.export.binary.BinaryOutputCapsuleBufferPositionTest --no-daemon
  • ./gradlew.bat :jme3-core:test --no-daemon

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates BinaryOutputCapsule to preserve the positions of NIO buffers (FloatBuffer, IntBuffer, ByteBuffer, and ShortBuffer) during serialization by wrapping the write operations in a try-finally block that restores the original position. It also adds a comprehensive unit test suite to verify this behavior. The reviewer suggests a cleaner approach: using absolute get(int index) instead of relative get(), which avoids mutating the buffer's position entirely and eliminates the need for the try-finally blocks.

Comment thread jme3-core/src/plugins/java/com/jme3/export/binary/BinaryOutputCapsule.java Outdated
Comment thread jme3-core/src/plugins/java/com/jme3/export/binary/BinaryOutputCapsule.java Outdated
Comment thread jme3-core/src/plugins/java/com/jme3/export/binary/BinaryOutputCapsule.java Outdated
Comment thread jme3-core/src/plugins/java/com/jme3/export/binary/BinaryOutputCapsule.java Outdated
@ittaigolde
Copy link
Copy Markdown
Contributor Author

Could a maintainer please approve/run the gated GitHub Actions workflow for this fork PR? If the screenshot comment workflow fails, it appears to be the same infrastructure issue: Build jMonkeyEngine is gated for the fork PR, so the Run Screenshot Tests check is not created for the monitor workflow to wait on.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes BinaryExporter buffer serialization so writing NIO buffers no longer mutates their current positions, aligning binary export behavior with the stated regression case for issue #2312.

Changes:

  • Replaces relative buffer reads after rewind() with absolute indexed reads in BinaryOutputCapsule.
  • Adds a regression test covering ByteBuffer, FloatBuffer, IntBuffer, and ShortBuffer position preservation and round-trip contents.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
jme3-core/src/plugins/java/com/jme3/export/binary/BinaryOutputCapsule.java Preserves caller buffer positions while serializing buffer contents.
jme3-core/src/test/java/com/jme3/export/binary/BinaryOutputCapsuleBufferPositionTest.java Adds regression coverage for buffer position preservation during binary export.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@riccardobl
Copy link
Copy Markdown
Member

riccardobl commented May 28, 2026

Thank you! Nice find and patch 💯

@riccardobl riccardobl merged commit f5a37ff into jMonkeyEngine:master May 28, 2026
10 checks passed
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

Successfully merging this pull request may close these issues.

BinaryExporter resets buffer positions to 0 when writing them.

3 participants