Skip to content

Classify TOC and geometry streams before GUID tables - #149

Open
menes97 wants to merge 1 commit into
IOExtend:mainfrom
menes97:fix/geometry-guid-misclassification
Open

menes97 wants to merge 1 commit into
IOExtend:mainfrom
menes97:fix/geometry-guid-misclassification

Conversation

@menes97

@menes97 menes97 commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #24

StreamClassifier.Classify checked the GUID-table shape before the TOC and geometry signatures. A TOC with recCount == 3 * chunkCount, or a geometry stream of a suitable length, has that shape too. So NwcIO-written files with three meshes (and similar sizes) gained spurious GUIDs on reopen.

Following the issue's first suggestion, the stricter signatures now run before the GUID heuristic:

  • Geometry: a vertex buffer record ([id][94|95], then 60 at offset 12) is recognised whatever its record id, when its vertex count fits in the stream.
  • TOC: the existing check (8 + 4*chunkCount + 4*recCount bytes) moved up unchanged.

One trade-off: a real GUID table whose first int is exactly three times its GUID count would now be read as a TOC. I don't know what that first int holds in real files, and I had no real fixtures to check against. The multiple-TOC handling the issue mentions is unchanged.

Tests

  • StreamClassifierTests.Classify_GuidShapedToc_ReturnsToc: one and two chunks.
  • WriterTests.Document_GuidShapedGeometryAndToc_RoundtripPreservesGuids: three meshes and a single 124-vertex mesh, with and without GUIDs. Checks the reopened GUIDs, vertices and stream kinds.

All 6 cases fail without the src change. dotnet build NwcIO.slnx succeeds, and NwcIO.Tests passes locally: 245 passed, and 2 skipped because they need private Navisworks files (.NET 10.0.400 on Windows, with global.json pinned down to it temporarily; not committed).

The change was drafted with OpenAI Codex, then reviewed and verified with Claude Code.

🤖 Generated with Claude Code

A TOC with three records per chunk, or a geometry stream of a suitable
size, also has the shape of a GUID table, which Classify checked first,
so spurious GUIDs were added to the document. Geometry vertex buffers
(any record id) and TOCs are now recognised before the GUID heuristic.

Fixes IOExtend#24

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@mahach666 mahach666 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes requested: the intended fix is useful, but the inline comment below describes a regression reproduced against the reviewed head and the unchanged base. Please address it and add the indicated regression test before merging.

Finding: Validate TOC contents before overriding a GUID-table match.

if (recCount > 0 && recCount < 1_000_000 && chunkCount > 0 && chunkCount < 5000)
{
var expected = 8 + 4 * chunkCount + 4 * recCount;
if (data.Length == expected || data.Length == expected + 4)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Validate TOC contents before overriding a GUID-table match

The size formula alone also matches a GUID table with header [a=6, count=2]: both formats have 40 bytes. A synthetic container containing that header followed by GUIDs 00112233-4455-6677-8899-aabbccddeeff and ffeeddcc-bbaa-9988-7766-554433221100 reopens with two GUIDs on main/base, but zero on this PR because this branch labels it Toc. GuidTableParser.TryParse accepts that table; its first header word currently has no constrained value. Interpreting these GUID bytes as TOC entries produces negative lengths, so this example is not a valid TOC.

Please validate the catalog entries or use container context before preferring Toc over Guids, and add this regression case. This was reproduced synthetically; it did not occur in the two real models checked (their GUID header has a=1).

This branch has not been deployed

No deployments
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.

StreamClassifier misreads TOC/geometry streams as GUID tables, adding spurious GUIDs (e.g. NwcIO-written files with 3 meshes)

2 participants