86b7ejgm6/add properties to draco

[vnoves]

Summary by CodeRabbit

• Bug Fixes
  • Improved robustness of glTF/GLB file export with enhanced handling of extensions, extras, and embedded resources.
  • Better image inlining into exported binary files.
  • Safer file operations during export with improved cleanup and overwrite behavior.
  • Enhanced path and file naming handling for more reliable exports.

[coderabbitai]

Walkthrough

The Draco.cs file is refactored to introduce new internal GLTF/GLB patching infrastructure. A new GltfExtrasPatcher class patches GLTF and GLB files post-encoding, handling extras merging, extension processing, and image inlining. The Draco.Compress method is updated to invoke this patcher and improve file handling with safer overwrite mechanics.

Changes

Cohort / File(s)	Summary
GLTF/GLB Patching Infrastructure Common_glTF_Exporter/Export/Draco.cs	Introduces internal GlbData and GltfExtrasPatcher classes with extensive logic to patch GLTF/GLB files after encoding. Includes PatchExtras entry point, GLTF/GLB-specific patching paths, node-level extras copying, extension merging, image inlining with MIME type handling, and GLB chunk manipulation utilities. Adds System.Linq, Newtonsoft.Json, and Newtonsoft.Json.Linq using directives.
Draco.Compress Refactor Common_glTF_Exporter/Export/Draco.cs	Refactors Draco.Compress to replace string.Concat with + operator for path assembly, introduces File_MoveOverwrite for safer file handling, calls GltfExtrasPatcher.PatchExtras post-encoding, manages GLTF bin file renaming, and cleans up intermediate files while preserving REVIT2025/2026 compatibility.

Sequence Diagram(s)

sequenceDiagram
    participant Draco as Draco.Compress
    participant Patcher as GltfExtrasPatcher
    participant JSON as JSON Handler
    participant GLB as GLB Reader/Writer
    participant FileOps as File Operations

    Draco->>Draco: Encode with Draco compression
    Draco->>Patcher: PatchExtras(encoded_file_path)
    
    alt File is GLB
        Patcher->>GLB: Read GLB structure (JSON + BIN chunks)
        GLB-->>Patcher: JSON chunk data
        Patcher->>JSON: Patch JSON (merge extensions, extras)
        JSON-->>Patcher: Updated JSON
        Patcher->>Patcher: Inline external images into BIN
        Patcher->>GLB: Write updated GLB with new chunks
    else File is GLTF
        Patcher->>JSON: Parse GLTF JSON
        JSON-->>Patcher: JSON data
        Patcher->>JSON: Copy node extras & extensions
        JSON-->>Patcher: Patched JSON
        Patcher->>Patcher: Inline external images, update refs
        Patcher->>FileOps: Write updated GLTF & BIN
    end
    
    Draco->>FileOps: Rename/move final BIN file
    Draco->>FileOps: Clean up intermediate files
    Draco->>FileOps: File_MoveOverwrite to destination

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

• GltfExtrasPatcher implementation: Substantial new class with multiple patching methods, JSON manipulation via Newtonsoft.Json, and GLB binary chunk handling—requires careful verification of correctness for both GLTF and GLB paths.
• Image inlining logic: Complex byte-level operations including 4-byte padding alignment, bufferView creation, MIME type resolution, and proper URI updates—error-prone and needs detailed review.
• Draco.Compress refactor scope: Changes touch file I/O, path handling, bin renaming, and cleanup logic; ensure REVIT2025/2026 compatibility is preserved and no edge cases in file overwriting are introduced.
• JSON parsing and mutation: Newtonsoft.Json usage for extension and extras merging requires verification that serialization/deserialization preserves data integrity.

Poem

🐰 Draco now patches with finesse and care,
Merging extras through GLB's binary lair,
Images inlined, extensions aligned,
From GLB to GLTF, no stone left behind,
Files dance and rename in proper array,
A hoppy refactor to brighten the day! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'add properties to draco' is vague and does not accurately reflect the substantial changes made. The PR introduces GLTF/GLB patching infrastructure, refactors Draco.Compress flow, and adds file handling logic—not just simple property additions.	Revise the title to better describe the main change, such as 'Add GLTF/GLB patching and refactor Draco compression flow' or 'Enhance Draco encoder with extras and image inlining support'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch 86b7ejgm6/AddPropertiesToDraco

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example:

"Create a concise high-level summary as a bullet-point list. Then include a Markdown table showing lines added and removed by each contributing author."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

Common_glTF_Exporter/Export/Draco.cs (4)

15-24: GLB read/write logic looks correct but could be more defensive

The GlbData DTO and the ReadGlb / WriteGlb implementations look structurally correct and match the GLB v2 layout (header + JSON chunk + optional BIN chunk). Padding with spaces for JSON and zeros for BIN is also appropriate.

One thing you may want to tighten up (non‑blocking):

• Validate the GLB header when reading:
  • Check magic == 0x46546C67 and version == 2 before proceeding.
  • Optionally ensure length is consistent with fs.Length and that chunkLen0, chunkLen1 don’t exceed file length.
• Optionally verify chunkType0 == 'JSON' and chunkType1 == 'BIN\0' before trusting the payload.

This would fail fast on corrupted or mis‑labeled files instead of parsing arbitrary data as JSON.

Also applies to: 296-322, 324-359

---

26-52: Extras and extensions merge behavior is sensible; consider future extension interactions

The GltfExtrasPatcher flow (PatchExtras → PatchExtrasGltf / PatchExtrasGlb → PatchNodeLike) plus MergeExtensionsUsedAndRequired and EnsureExtInArray is coherent:

• Node extras from the original are cleanly copied onto the Draco‑encoded file.
• Node extensions are merged so that KHR_draco_mesh_compression from the Draco output wins, and other extensions from the original are preserved.
• Root‐level extensionsUsed / extensionsRequired are unioned, and Draco is ensured in extensionsUsed.

Two small considerations (optional):

1. Other “known” structural extensions
   Right now CopyUnknownExtensions only treats "KHR_draco_mesh_compression" as “known”. If you later introduce other structural extensions that Draco also rewrites (e.g., mesh/material related), you may need to add them to the keepKnown set to avoid overwriting encoder‑generated data with the original JSON.

2. Non‑node extras
   The patcher only restores extras at the node level. If you ever start emitting extras on meshes/materials/scene, they won’t be restored by this pipeline. Not an issue today, but worth documenting.

As it stands, this matches the current Revit usage (node extras) and looks good.

Also applies to: 68-88, 89-177

---

179-295: Image inlining logic is solid; watch for alignment and file deletion side effects

The InlineExternalImagesIntoBin_JObject / AppendBytes helpers are generally well thought‑out:

• Ensure buffers[0] and bufferViews exist.
• Load an existing bin (GLTF) or reuse the GLB BIN chunk.
• Append image bytes, create bufferViews, and wire images[i].bufferView + mimeType.
• Pad the combined BIN to 4 bytes, while keeping bufferView.byteLength equal to the raw image size (excluding padding), which is correct.
• Update buffers[0].byteLength and optionally delete consumed image files.

A couple of subtle behaviors to be aware of:

1. Alignment assumptions
   AppendBytes assumes the incoming binBytes.Length is already 4‑byte aligned (so that the new byteOffset is also aligned). This is true for well‑formed GLTF/GLB binaries, but if you ever feed a non‑aligned BIN in here, byteOffset could end up misaligned. If you want to guard against that, you could:

   • Either pad binBytes to 4 before the first append, or
   • Assert that binBytes.Length % 4 == 0 when non‑null.

2. Deleting external image files
   removeExternalImageFiles == true will delete any successfully inlined images at Path.Combine(baseDir, uri). That’s perfectly fine for the “export & embed everything” scenario, but it means:

   • The same texture referenced by multiple GLTF/GLB files in the same folder will be removed after the first compression.
   • Re‑running compression on the same output directory may find some images missing.

If that behavior is intentional, consider a brief comment, or wiring this flag to a user preference later.

---

382-388: Compress pipeline changes mostly look good; tighten a few edge cases

The updated Compress pipeline is cleaner and the integration of GltfExtrasPatcher.PatchExtras is nice — it preserves node extras while letting Draco rewrite the geometry. The temp/final naming scheme (*.gltf vs *Temp.gltf, same for *.glb) is consistent.

Some details worth calling out:

1. Order of operations is correct

   • Draco encodes into fileToCompressTemp.
   • Patcher reads both original and temp, merges extras/extensions, inlines images, and writes back to temp.
   • You only delete files after patching completes, so originals are preserved on failure.
   • For GLTF, you then move the temp BIN to the final BIN, and patch the GLTF JSON to reference the final BIN name.

   This is all sound.

2. String.Replace‑based renames

   • binTemp = fileToCompressTemp.Replace(".gltf", ".bin"); and binFinal = fileToCompressTemp.Replace("Temp.gltf", ".bin"); will behave correctly for normal paths like .../SceneTemp.gltf.
   • As a minor robustness improvement, using Path.ChangeExtension for the extension change and a small helper for stripping "Temp" would be less brittle and more self‑documenting.

3. JSON replacement scope
   In the GLTF case you replace binTempName with binFinalName across the whole JSON text. That can also touch any string in extras or custom data that happens to contain the temp bin name. That’s unlikely in practice, but if you want to be strict you could:

   • Parse the JSON and update only buffers[0].uri (and/or other known locations) instead of raw string replacement.

4. File_MoveOverwrite behavior
   File_MoveOverwrite deletes the destination if it exists, then moves the source. If deletion fails, File.Move will throw, which is probably what you want, but note:

   • Any IO exceptions from File.Move are not caught, so the entire export will fail in that case.
   • If you ever need more robust behavior, File.Replace might be an option to consider on Windows.

Overall, the flow is correct; the above points are mostly about making the behavior a bit more explicit and resilient.

Also applies to: 392-395, 424-431, 433-463, 467-474

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e92a6fb and 8d657fc.

📒 Files selected for processing (1)

• Common_glTF_Exporter/Export/Draco.cs (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

Common_glTF_Exporter/Export/Draco.cs (1)

Common_glTF_Exporter/Export/RevitGrids.cs (1)

• Export (21-65)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: build