Add a native Draco encoder to the NativeDraco plugin

[bkaradzic-microsoft]

Adds a native Draco encoder to the NativeDraco plugin as _native.DracoCodec.Encode. The plugin already ships a native decoder, so this completes the pair and lets Babylon.js's DracoEncoder use a synchronous native path instead of fetching and instantiating the draco_encoder WebAssembly module at runtime.

Implementation. Deliberately mirrors Draco's own emscripten glue so the native and WASM paths agree: AddAttributeToMesh<T> mirrors PointCloudBuilder::AddAttribute<T>, AddTypedAttributeToMesh mirrors the WASM encoder's addAttributeMap, and ReadIndices handles Uint16Array/Uint32Array index upload. The returned { data, attributeIds } shape matches what the existing WASM worker and module paths produce, so the JavaScript side needs no special-casing beyond its feature probe. TypedArrayData<T> honors the view's ByteOffset rather than assuming offset 0.

DRACO_GLTF_BITSTREAM is switched off. That subset is not sufficient once the encoder is in play: it drops pre-glTF backwards compatibility (rejecting older streams with "Unsupported major version") and compiles out the attribute deduplication passes the encoder relies on. Building the full library costs roughly 1.2 MB.

Risk. Purely additive — DracoCodec.Decode and Version are untouched and nothing existing changes shape, so no Babylon.js change is required and the encoder sits unused until a JS consumer probes for it. BABYLON_NATIVE_PLUGIN_NATIVEDRACO=ON is already set by every CI workflow (win32, uwp, linux, macos, ios, android), so this code is compiled and linked on all platforms by this PR's own run.

Blocked on JsRuntimeHost#223. The sole red job, Ubuntu_Clang_QuickJS, is not a defect here: the new tests make a native module throw, which walks Napi::Error::what() into a use-after-free in QuickJS's napi_escape_handle. With only that dependency changed, this branch goes from exit 139 to a clean 16/16 across five runs. Needs a JsRuntimeHost pin bump once #223 lands.

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds a native Draco mesh encoder to the NativeDraco plugin so Babylon.js can synchronously encode via _native.DracoCodec.Encode instead of loading the WASM encoder at runtime.

Changes:

• Implement native mesh encoding path in NativeDraco.cpp, including attribute upload, index handling, and option mapping (quantization/speed/method).
• Expose DracoCodec.Encode alongside existing Decode/Version.
• Build full Draco (disable DRACO_GLTF_BITSTREAM) to support encoding and required deduplication/back-compat behaviors.

Reviewed changes

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

File	Description
Plugins/NativeDraco/Source/NativeDraco.cpp	Adds encoder implementation and exports Encode on DracoCodec.
Dependencies/CMakeLists.txt	Disables DRACO_GLTF_BITSTREAM so full Draco features needed for encoding are built.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bkaradzic-microsoft]

The macOS failures were the JavaScript.All unit test, not a compile error. The NativeDraco suite contained an assertion that the plugin does not expose an encoder:

it("does not expose an encoder", function () {
    expect(_native.DracoCodec.Encode).to.equal(undefined);
});

That was an accurate description of the old behavior, so this PR necessarily invalidates it. I replaced it with actual encoder coverage rather than just deleting it:

• round trips an indexed mesh through Encode and back through Decode
• encodes an unindexed mesh
• accepts 32 bit indices
• rejects a non 16/32 bit index buffer, an index count that is not a multiple of 3, an attribute length that is not a multiple of its component count, and a mesh with no position attribute

The last four exercise the validation added in response to the review comments above.

dist/tests.javaScript.all.js is committed and consumed directly by Apps/UnitTests/CMakeLists.txt, so it has to be regenerated. I rebuilt it with the lockfile-pinned toolchain (npm ci, webpack 5.105.2) so the bundle diff is exactly the 74 lines of the source change with no incidental churn.

[bkaradzic-microsoft]

CI caught two real problems in the new tests — thanks, both now fixed in b4bdc65.

1. Encode returned an Int8Array. The result buffer was built with Napi::Int8Array, so the encoded bytes came back signed:

expected Int8Array[ 68, 82, 65, 67, 79, 2, ... ] to be an instance of Uint8Array

Draco output is a byte stream, so Uint8Array is the correct type, and it matches what Decode accepts as input. This was a genuine API bug that only surfaced because the new round-trip test asserts the returned type.

2. The unindexed encode test was wrong, not the code. Without an index buffer the vertices are taken as a flat triangle list, so the vertex count itself has to be a multiple of three. The shared fixture is a four-vertex quad, which correctly tripped the new %3 validation:

Draco: Index count 4 is not a multiple of 3

The test now uses a single triangle. The validation behaved exactly as intended here.

Verified locally on Win32 (JavaScript.All): 49 passing, 0 failing, exit 0.

[bkaradzic-microsoft]

The Ubuntu_Clang_QuickJS segfault turned out to be a real bug in the encode path, not a test problem. Fixed in 8f7252a.

Finding it. CI stdout is block buffered, so the trailing output was lost with the crash. Diffing the printed test list against the passing Ubuntu_Clang_JSC job narrowed it to rejects an index count that is not a multiple of 3 — which is the first test that reads typed array data and then throws. The rejection test just before it throws before touching the data pointer, which is why it passed.

Two fixes:

1. Indices were never range checked. Nothing validated index values against the vertex count, so an out of range index was written into a face and later dereferenced by DeduplicateAttributeValues / DeduplicatePointIds and the encoder, reading past the end of the attribute buffers. This is reachable from script in one line:

   _native.DracoCodec.Encode([{ kind: "position", dracoName: "POSITION", size: 3, data: positions }],
                             new Uint16Array([0, 1, 9999]));

   It now throws, and there is a test for it. Note this hole only became reachable in this PR, since DRACO_GLTF_BITSTREAM=OFF enables the deduplication passes. I also rejected a non-positive position component count, which otherwise divided by zero when computing the vertex count.

2. Typed array reads now go through TypedArrayOf<T>::Data() instead of ArrayBuffer().Data() plus the byte offset. napi_get_typedarray_info already returns a pointer to the first element, so the manual offset arithmetic was redundant, and this avoids materializing a temporary ArrayBuffer handle just to read a pointer.

Verified locally on Win32 against both engines: default 50 passing, QuickJS 40 passing, exit 0 in both cases. I built a local QuickJS configuration specifically to reproduce this.

[bkaradzic-microsoft]

Update on the Ubuntu_Clang_QuickJS failure: it is not caused by this PR. It is a latent heap-use-after-free in JsRuntimeHost's QuickJS Node-API shim that the new tests happen to expose.

napi_escape_handle stores the escaped handle at the scope start index so it outlives the scope, but napi_close_escapable_handle_scope then resizes the handle stack back to that same index and frees it. Napi::ObjectReference::Get uses an EscapableHandleScope, and Napi::Error::Message() / what() are built on it, so reading the message of a native error on QuickJS reads freed memory.

The tests added here make a native module throw, and ExternalCallback::Callback calls e.what() when there is no pending QuickJS exception, which walks into the freed handle. ASan on Ubuntu:

==ERROR: AddressSanitizer: heap-use-after-free
    #0 ToJSValue                     js_native_api_quickjs.cc:302
    #3 Napi::Error::Message
    #4 Napi::Error::what
    #5 ExternalCallback::Callback    js_native_api_quickjs.cc:164
freed by:
    #1 napi_close_escapable_handle_scope  js_native_api_quickjs.cc:1939
    #2 Napi::ObjectReference::Get

Fix is up as BabylonJS/JsRuntimeHost#223. With that branch patched in, this PR's JavaScript.All is 50 passing / exit 0 / 0 ASan errors in the exact clang + QuickJS config that is red here.

So this PR is blocked on JsRuntimeHost#223 landing plus a submodule bump. Every other job is green. Happy to either wait for the bump, or land this with the known-external red job if you would rather not serialize them.

[bkaradzic-microsoft]

CI status update: re-ran the failed jobs, and the Win32_x64_D3D11 failure was an unrelated infrastructure flake (403 Forbidden from snippet.babylonjs.com) which now passes. That leaves 31/32 green, with Ubuntu_Clang_QuickJS the only red job.

That one job is the external JsRuntimeHost use-after-free described above, not anything in this PR. The fix, BabylonJS/JsRuntimeHost#223, is now 24/24 green and ready for review. Once it lands and the JsRuntimeHost dependency here is bumped, this PR should be fully green — verified locally by building this branch against the fix with clang + QuickJS on Ubuntu, which gives 50 passing, exit 0, and 0 ASan errors.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Plugins/NativeDraco/Source/NativeDraco.cpp:484

• The native result uses Uint8Array, but Babylon.js's IDracoEncodedMeshData contract and existing WASM/module encoder path return Int8Array. Because this entry point is intended as a drop-in native path, this changes observable behavior (instanceof and the declared return type), and the new test currently locks in the mismatch. Return an Int8Array here and update the source/generated test expectation accordingly.

            auto encodedData = Napi::Uint8Array::New(env, buffer.size());

Plugins/NativeDraco/Source/NativeDraco.cpp:514

• Exposing Encode while building full Draco leaves Plugins/NativeDraco/README.md:5-13 and its API declaration at lines 23-41 materially incorrect: they still describe a decode-only, glTF-bitstream-only plugin and omit Encode. Update that documentation as part of this API change so consumers are not told the new capability is unavailable.

        codec.Set("Encode", Napi::Function::New(env, EncodeDracoMesh, "Encode"));

Plugins/NativeDraco/Source/NativeDraco.cpp:462

• This comment says the decoder is built with DRACO_GLTF_BITSTREAM, but this PR explicitly sets that option to OFF. Keep the macro guards for externally supplied Draco targets, but describe the conditional case rather than the configuration used by this build.

            // Mirror Encoder::EncodeMeshToDracoBuffer. The deduplication passes are compiled out by
            // DRACO_GLTF_BITSTREAM (the glTF bitstream subset the decoder is built with does not need
            // them), so guard them on the feature macros draco publishes. They only shrink the encoded
            // output; skipping them still produces a valid stream.

Plugins/NativeDraco/Source/NativeDraco.cpp:245

• The added tests only pass typed arrays whose views start at byte offset zero, so the byte-offset behavior this helper specifically introduces is unverified. Add an encode/decode test using position and/or index subviews with non-zero byteOffset; otherwise a backend-specific TypedArrayOf<T>::Data() regression could silently encode preceding buffer data.

This issue also appears in the following locations of the same file:

• line 459
• line 484
• line 514

        const T* TypedArrayData(const Napi::TypedArray& array)
        {
            return array.As<Napi::TypedArrayOf<T>>().Data();

[bkaradzic-microsoft]

Addressed the four comments in the collapsed Suppressed comments block of the Copilot review in 8c53b98b. Those don't create review threads, so they weren't in the resolved/unresolved list and I'd missed them until now. All four were correct.

Encode should return Int8Array, not Uint8Array. Agreed, and I had this backwards. I originally moved it to Uint8Array because Int8Array surfaces the high bytes as negative numbers, which reads wrong in a debugger — but that's cosmetic, the bytes are identical, and it's the wrong thing to optimize for. IDracoEncodedMeshData in packages/dev/core/src/Meshes/Compression/dracoEncoder.types.ts declares data: Int8Array, because the WASM encoder returns a view onto emscripten's signed HEAP8. Since this entry point exists to be a drop-in for that path, the view type has to match what callers instanceof-check and declare against. Now Napi::Int8Array, with the reason in a comment so it doesn't get "fixed" again. The round-trip test still passes: Decode takes any Napi::TypedArray and reads raw bytes off the backing buffer, so it doesn't care about the view type.

README was materially wrong. Correct — it still described a decode-only, glTF-bitstream-only plugin, omitted Encode from the interface declaration, and stated DRACO_GLTF_BITSTREAM=ON when this PR sets it OFF. Rewritten: the encoder is in the summary and the TypeScript declaration, and the bitstream bullet now gives the real reason for OFF (the subset drops pre-glTF backwards compatibility, so it rejects streams from older or full encoders with "Unsupported major version", and it compiles out the deduplication passes the encoder needs) rather than the inverted one.

Stale DRACO_GLTF_BITSTREAM comment at the deduplication guards. Correct. The #ifdefs are there for externally supplied draco targets that may enable the subset, not for this build's configuration, so the comment now describes the conditional case.

No test with a non-zero byteOffset. This was the valuable one. TypedArrayData<T>() exists precisely to read through the typed view rather than off the start of the backing ArrayBuffer, and nothing exercised it. Added a test that encodes position and index subviews sitting partway into larger buffers, with the padding filled with deliberately wrong values, then decodes and compares corners — so a regression that ignored byteOffset would encode the padding and fail the comparison instead of passing quietly. Both use sites are covered (NativeDraco.cpp:294 for attributes, :325 for 16-bit indices).

41/41 unit tests pass locally.

On the remaining CI failure: Ubuntu_Clang_QuickJS still segfaults at teardown after all Draco tests report passing, which matches the pre-existing use-after-free in the QuickJS Node-API shim I described earlier in this PR — it isn't specific to this change.

[bkaradzic-microsoft]

The Ubuntu_Clang_QuickJS failure is not coming from this PR's code. I reproduced it locally and tracked it down: it's a use-after-free in the QuickJS Node-API shim that JsRuntimeHost#223 fixes. This PR is just the first thing to exercise the affected path.

What happens. Every test passes, then the process segfaults on the way out:

✅ encodes typed array views with a non-zero byteOffset
Segmentation fault (core dumped)

The backtrace points somewhere quite specific:

#0  js_force_tostring                       quickjs.c:4813
#3  napi_get_value_string_utf8              js_native_api_quickjs.cc:696
#4  Napi::String::Utf8Value
#6  Napi::Error::Message                    napi-inl.h:3087
#7  Napi::Error::what
#8  ExternalCallback::Callback              js_native_api_quickjs.cc:164

Frame 8 is the shim's catch-all, which calls e.what() when a C++ exception escapes without a pending JS exception. Napi::Error::what() reads the error's message, and ObjectReference::Get does that through an escapable handle scope:

inline MaybeOrValue<Napi::Value> ObjectReference::Get(const char* utf8name) const {
  EscapableHandleScope scope(_env);
  ...
  return scope.Escape(result);
}

The escaped handle is freed when that scope closes, so reading it afterwards dereferences freed memory. At the crash the JSValue has tag = -7 (string) but ptr = 0x7ff8dec9a216, which isn't even pointer-aligned — reused memory.

This PR triggers it because the encoder tests (rejects malformed input, rejects truncated input, rejects an empty buffer) are what throw Napi::Error from a native callback. Nothing on master does, which is why master is green.

Verified by A/B. Same tree, same flags as CI (clang, QuickJS, RelWithDebInfo, no sanitizers), only the JsRuntimeHost dependency changed:

JsRuntimeHost	Result
current master	exit 139, 1, 139 — segfault
with #223	exit 0 × 5 — clean, 16/16

So this job should go green once #223 lands; nothing to change here. Happy to rebase once it does.

[bkaradzic-microsoft]

CI is green now (34/34). Rebased on master and added one commit that is not Draco work, so flagging it explicitly:

Ubuntu_Clang_QuickJS was segfaulting the moment it entered the NativeDraco tests. The bug was not in this PR -- QuickJS is refcounted, and napi_escape_handle was dropping the escaped value's last reference when its escapable scope closed, leaving the caller holding a freed JSValue. This PR just happens to be the thing that trips it, because the encoder returns a typed array out of an escapable scope.

Fixed upstream in BabylonJS/JsRuntimeHost#223, now merged, so this bumps the GIT_TAG pin from 9271f13b to dbd4620f. That is the only commit between the two, so the bump is a fast-forward carrying nothing else.

Verified locally on Linux/Clang/QuickJS rather than relying on CI alone. With the old pin:

[log]   NativeDraco
Segmentation fault (core dumped)

and with the new one the same build runs 16/16 suites, 51 assertions, no crash.

Happy to split the pin bump into its own PR if you would rather keep this one purely Draco, but on its own it would have nothing to demonstrate.