Keep binary class IDs distinct when loading J3O files

[toaster0123]

What goes wrong

Imagine saving a scene that contains many different kinds of objects, then reopening it and finding that some objects have been loaded as the wrong Java class.

That can happen when a J3O file contains 129 or more distinct saved classes. Each class has a small numeric ID, but the loader was treating those ID bytes as ordinary text. Some different IDs then became indistinguishable. The trigger is the number of different classes, not simply the number of objects.

In the reproduction, a file with 128 classes loads correctly. At 129 classes, one object comes back as the wrong type; at 255 classes, 127 objects do.

What this changes

The loader now reads class IDs as integers and stores them in IntMap. It can tell the classes apart without converting their IDs to text. The patch is confined to loading: it does not change how scenes are saved.

Does this break old exports?

The J3O file format and exporter are unchanged. This change does not require assets to be re-exported.

I checked compatibility using existing files, not just by saving new files with the patched code:

• Seven files were written with the pre-patch exporter and then read unchanged with the patched loader. The previously working scenes still load correctly; the scenes affected by this bug now restore the correct object types. File hashes confirm that none were rewritten
• Existing repository exports in binary format versions 0, 1 and 2 (Boat, Tree and Jaime) load before and after the patch with matching object classes, vertex counts and triangle counts
• The existing Default.j3o and Console.j3o font tests pass
• Unicode class names, field names and string values are preserved in the compatibility checks

Class and field names still use their previous text-decoding rules. These checks do not cover every historical engine version or every combination of system text encodings.

Tests and scope

• Core, desktop, plugins, effects and terrain suites: 568 tests, zero failures, two existing skips
• Six committed regression cases cover low/high IDs and non-ASCII field names/string values; three ID cases fail before the fix
• Core main/test Checkstyle passes with existing warnings
• A separate smoke test uses the normal exporter with 2, 127, 128, 129, 130, 131, 255, 256, 257 and 261 distinct classes. All object types are preserved after the fix
• A separate sweep of 253 valid ID ranges, including Unicode names and values, goes from 127 failures to zero

The committed compact tests cover one-byte IDs. The larger smoke test covers the two-byte boundary; that distinction is now documented in the test class. Validation is limited to the listed modules and checks, not a full-platform build.

[jaime-jmebot]

Nice catch — the alias bytes really are raw IDs (see generateTag() / fixClassAlias() in BinaryExporter), so decoding them losslessly instead of via the default charset is the right call. Two notes, no blockers:

• The fix itself looks solid. readString(..., ISO_8859_1) is applied symmetrically in the class table and in readObject(), which is what keeps the map lookup consistent — that symmetry is the whole fix, and it's there. Class names, field names and string values keeping their existing decoding means the on-disk format and exporter are untouched, so no version bump. Import ordering also satisfies the existing checkstyle config.

• A short comment at the alias read sites would help future readers. Something like "aliases are raw numeric ID bytes, not text — decode losslessly" would stop someone from "fixing" these two lines back to the default charset later. That's the main thing I'd add to the patch.

• The committed tests only reach 1-byte aliases. The interesting boundary is IDs ≥ 255, where aliases become 2 bytes. Since the alias width is fixed per table (derived from the class count), a 3-class fixture can't carry a 2-byte alias, so the 255/256/257/261-class checks from your description can't be expressed this way. Worth either committing one boundary test that goes through the real exporter path, or noting in the test/class javadoc that the 2-byte boundary is covered externally. Otherwise the exact regression that motivated the PR stays untested in the suite.

Overall: correct, minimal, well-targeted. The compile/test evidence in the description lines up with what I see in the code — nice work.

[riccardobl]

I think it would be better to refactor this to use integer aliases directly, while keeping backward compatibility with existing exports. You can use IntMap .

[toaster0123]

Switched the class table to IntMap and read the raw class IDs directly as integers in b6559ed, as requested. Both readers use the exporter's big-endian byte order; class IDs no longer pass through a charset. Text decoding and the exporter are unchanged.

For backward compatibility, I wrote seven files with the pre-patch exporter and loaded the exact same bytes with this version. Previously working files still pass, and the affected 129/255/256/261-class files now restore the correct types. Unicode names/values are preserved. Existing repository exports in format versions 0, 1 and 2 also load with matching class and geometry counts.

I also added the requested raw-ID explanation and documented that the committed compact tests cover one-byte IDs, while the separate ordinary-exporter smoke covers the two-byte boundary. The description now explains the problem and compatibility checks more plainly.

The updated implementation passes 568 tests across core, desktop, plugins, effects and terrain, with zero failures and two existing skips, plus core Checkstyle. All ten large-scene sizes and the 253-case Unicode/ID sweep pass.