jmonkeyengine icon indicating copy to clipboard operation
jmonkeyengine copied to clipboard

Fix misuse of read and skipBytes

Open riccardobl opened this issue 3 months ago • 1 comments

Fixes https://github.com/jMonkeyEngine/jmonkeyengine/issues/2555 across the entire engine.

This PR fixes the implementation of LittleEndien.readFully() and introduces two new utility methods in ByteUtils:

  • readFully
  • skipFully

Both ensure that the requested number of bytes are reliably read or skipped, even if the underlying stream does not return the full amount in a single call.

The changes are not yet fully tested, but they are part of the codebase I am actively working with. Additional testing and verification from others would be greatly appreciated.

riccardobl avatar Sep 17 '25 21:09 riccardobl

This patch may (still) actually cause an issue with GLTF loader. Essentially requiring the "full skip" and erroring on EOF can cause failure. From an asset I'm working with:

int end = count * stride + byteOffset; //1839200, when the actual byte array backing it is only 1839188 long
        stream.skipBytes(byteOffset);
        while (index < end) {
            //Index is 1839136, reads the last value
            for (int i = 0; i < numComponents; i++) {
                buffer.put(readAsFloat(stream, format));
            }
            if (dataLength < stride) {
                //Tries to skip 52 bytes; but 1839136+52=1839200 > 1839188
                stream.skipBytes(stride - dataLength);
                //Throws EOFException here
            }
            index += stride; //Would set index to 1839200 and skip loop
        }

(I'd add the asset as example since it messes up so much, but it's Unreal licensed.)

ScottPeterJohnson avatar Sep 17 '25 22:09 ScottPeterJohnson

🖼️ Screenshot tests have failed.

The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests.

📄 Where to find the report:

  • Go to the (failed run) > Summary > Artifacts > screenshot-test-report
  • Download the zip and open jme3-screenshot-tests/build/reports/ScreenshotDiffReport.html

⚠️ If you didn't expect to change anything visual: Fix your changes so the screenshot tests pass.

If you did mean to change things: Review the replacement images in jme3-screenshot-tests/build/changed-images to make sure they really are improvements and then replace and commit the replacement images at jme3-screenshot-tests/src/test/resources.

If you are creating entirely new tests: Find the new images in jme3-screenshot-tests/build/changed-images and commit the new images at jme3-screenshot-tests/src/test/resources.

Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar".

See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information

Contact @richardTingle (aka richtea) for guidance if required

github-actions[bot] avatar Dec 22 '25 10:12 github-actions[bot]