Add native Draco and meshopt codec plugins - #1797
Conversation
The WASM Draco decoder fails to instantiate in the full validation suite
(order-dependent LinkError after ~139 prior tests). Port the decoder to a
native C++ plugin per the project's no-WASM policy.
NativeDraco exposes _native.decodeDracoMesh(dataView, attributes?), a
synchronous decoder built on google/draco 1.5.7 (CMake FetchContent). It
emits the same { indices, attributes[], totalVertices } shape as the WASM
worker path (de-interleaved, tightly packed per-point values).
draco marks its include paths PRIVATE, so re-export them SYSTEM PUBLIC on
the combined target (draco/draco_static), including CMAKE_BINARY_DIR for the
generated draco/draco_features.h.
Un-excludes 7 Draco decode tests (202-206, 232, 233), all pass. Full
regression 373/373. Encoder-dependent tests (207, 217) stay WASM/excluded.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Extends the NativeDraco plugin with _native.encodeDracoMesh(attributes,
indices, options), a synchronous native replacement for the WASM Draco
encoder. Draco is now fully native (decode + encode); no Draco test
depends on WASM.
Faithfully replicates draco's emscripten encoder wrapper: builds a
draco::Mesh (SetNumFaces/SetFace), adds attributes via a templated
AddAttributeToMesh<T> (PointAttribute::Init + per-point SetAttributeValue),
applies quantization/method/speed options, then DeduplicateAttributeValues
+ DeduplicatePointIds + Encoder::EncodeMeshToBuffer. Returns { data,
attributeIds }. The returned attribute id equals draco's unique_id
(PointCloud::SetAttribute -> set_unique_id), so the decoder's
GetAttributeByUniqueId round-trips.
Un-excludes idx 207 (Decoder/Encoder roundtrip) and 217 (glTF serializer
KHR draco), both pass. Full regression 375/375.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Native replacement for the WASM meshopt decoder (zeux/meshoptimizer), exposing _native.decodeMeshopt(source, count, stride, mode, filter?). Mirrors the reference meshopt_decoder.js decode() helper: rounds count up to a multiple of 4, decodes via meshopt_decodeVertexBuffer/IndexBuffer/ IndexSequence, applies the optional Oct/Quat/Exp filter in-place, and returns the first count*stride bytes. FetchContent meshoptimizer v0.22. Un-excludes idx 234 (GLTF Buggy with Meshopt Compression). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
There was a problem hiding this comment.
Pull request overview
Adds two new native codec plugins (NativeDraco, NativeMeshopt) to replace the current WASM-based Draco/meshopt mesh decoding path in Babylon Native, wiring them into the Embedding runtime and build system via new FetchContent dependencies.
Changes:
- Introduces C++ N-API implementations for
_native.decodeDracoMesh,_native.encodeDracoMesh, and_native.decodeMeshopt. - Adds CMake targets for the new plugins and wires them into
Embeddinginitialization. - Adds
dracoandmeshoptimizeras FetchContent dependencies and configures their builds for use as libraries.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Plugins/NativeMeshopt/Source/NativeMeshopt.cpp | Implements native meshopt decode entry point exposed to JS. |
| Plugins/NativeMeshopt/Include/Babylon/Plugins/NativeMeshopt.h | Declares NativeMeshopt initialization API. |
| Plugins/NativeMeshopt/CMakeLists.txt | Builds/link-wires the NativeMeshopt plugin. |
| Plugins/NativeDraco/Source/NativeDraco.cpp | Implements native Draco decode + encode entry points exposed to JS. |
| Plugins/NativeDraco/Include/Babylon/Plugins/NativeDraco.h | Declares NativeDraco initialization API. |
| Plugins/NativeDraco/CMakeLists.txt | Builds/link-wires the NativeDraco plugin. |
| Plugins/CMakeLists.txt | Registers the new plugin subdirectories in the plugin build. |
| Embedding/Source/Runtime.cpp | Initializes the new plugins during runtime setup. |
| Embedding/CMakeLists.txt | Links the new plugins into the Embedding target. |
| Dependencies/CMakeLists.txt | Fetches/configures draco + meshoptimizer dependency targets. |
| CMakeLists.txt | Declares new FetchContent sources for draco and meshoptimizer. |
…rrowing Fixes the macOS/iOS build break and the review comments on BabylonJS#1797. Build fix: NativeDraco.cpp used `long` for vertex and face counts, which is 64-bit on the Apple toolchains and tripped -Werror,-Wshorten-64-to-32 when narrowed to draco's uint32_t index types. All three sites now use uint32_t directly. MSVC did not catch this because `long` is 32-bit there. Plugin gating: NativeDraco and NativeMeshopt were added unconditionally, unlike every other plugin. Both now follow the established convention with BABYLON_NATIVE_PLUGIN_NATIVEDRACO / _NATIVEMESHOPT options (default ON), wired through Plugins/CMakeLists.txt, Embedding/CMakeLists.txt and the Runtime.cpp initialization. The draco and meshoptimizer FetchContent dependencies are gated on the same options, so turning a plugin off also drops its dependency instead of paying the download and binary size. Verified: with both OFF, neither dependency is fetched and Embedding builds. Input validation: the entry points took caller-supplied sizes on trust. - Draco attribute `size` is validated to [1, 127] and checked to divide the attribute data length evenly. A size of 0 previously divided by zero. - Draco indices must be Uint16Array or Uint32Array, a multiple of 3 in length, and every index must address a real vertex. Out-of-range indices previously reached draco's corner table builder and read out of bounds. - Unindexed meshes must supply a vertex count that is a multiple of 3 rather than silently dropping the remainder. - meshopt `count` and `stride` are validated before use: count non-negative, stride in [1, 256], a multiple of 4 for ATTRIBUTES, exactly 2 or 4 for TRIANGLES/INDICES, TRIANGLES count a multiple of 3, and the decoded size bounded to 2 GB so count4 * stride cannot wrap size_t. meshoptimizer itself checks these with assert(), which compiles out in release builds. Range guards compare through uint64_t so they stay well-defined on 32-bit ABIs rather than becoming tautological. Verified all twelve malformed inputs now raise a JS exception instead of crashing, and the encode/decode round trip is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
|
Pushed 5d89056 addressing all five review comments plus the macOS/iOS build break. Build break (macOS / iOS)
Plugin gatingYou're right, and it mattered more than just convention. Both plugins now have I also gated the Input validationAll four findings were real. Fixed and each one verified to now raise a JS exception rather than misbehave:
Worth calling out for the meshopt ones: meshoptimizer validates Range guards compare through Unindexed meshes now also require a vertex count that's a multiple of 3 rather than silently dropping the remainder. Verification
|
|
CI is green across all 34 checks. The four macOS/iOS jobs that were failing ( One note for the record:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
Plugins/NativeMeshopt/Source/NativeMeshopt.cpp:84
- The decoded-size guard can be bypassed for very large
countInvalues becausecount4(asize_t) is narrowed toint64_tin the check. Ifcount4 > INT64_MAX, the cast becomes negative and the limit check can fail, potentially leading to a huge allocation (or wrap/DoS). Avoid narrowing casts and validate the roundedcount4using unsigned arithmetic before allocating.
const size_t count = static_cast<size_t>(countIn);
const size_t stride = static_cast<size_t>(strideIn);
// Round count up to a multiple of 4 (the reference decoder over-allocates
// and runs the vertex filter over count4 elements).
const size_t count4 = (count + 3) & ~static_cast<size_t>(3);
// Guard the allocation size so a huge count cannot wrap size_t.
constexpr int64_t maxDecodedBytes = 1LL << 31;
if (static_cast<int64_t>(count4) * strideIn > maxDecodedBytes)
{
Plugins/CMakeLists.txt:25
- This block’s indentation is inconsistent with the surrounding plugin
if()/endif()sections (and currently makes it look likeNativeInputis nested underNativeEngine). Align theif/endifindentation to match the rest of the file for readability/maintenance.
if(BABYLON_NATIVE_PLUGIN_NATIVEENGINE)
add_subdirectory(NativeEngine)
endif()
if(BABYLON_NATIVE_PLUGIN_NATIVEINPUT)
draco declares the combined library plus ~25 internal object libraries, but only `draco` / `draco_static` were given a FOLDER property. The remaining 25 landed at the top level of the generated Visual Studio solution, next to the Babylon Native projects. Rather than hand-listing them, which would go stale the next time draco reorganizes its build, this adds a small `group_directory_targets` helper that walks a directory's BUILDSYSTEM_TARGETS (and nested SUBDIRECTORIES) and assigns the folder to each non-INTERFACE target. It is applied to draco and to meshoptimizer. Verified on the generated solution: all 26 draco targets are now under Dependencies/draco, meshoptimizer under Dependencies/meshoptimizer, and no target anywhere in the solution is left ungrouped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
|
Good catch — fixed in d784a88. draco declares the combined library plus ~25 internal object libraries ( Rather than hand-listing them — which would go stale the next time draco reorganizes its build — I added a small Verified against the regenerated solution:
and no target anywhere in the solution is left ungrouped — which wasn't true before this PR either, so this cleans up the tree slightly beyond just the draco additions. |
Addresses review feedback on the plugin's public surface, footprint and
lack of automated coverage.
API shape. The three entry points were free functions on `_native`
(`decodeDracoMesh`, `encodeDracoMesh`, `decodeMeshopt`), following
`NativeEncoding.EncodeImageAsync`. Every other plugin exposes an object,
and Babylon.js would otherwise need one feature probe per function. They
are now `_native.DracoCodec.Decode` / `.Encode` and
`_native.MeshoptCodec.Decode`, PascalCase to match the surrounding
convention, and each object carries a `Version` string. Version is the
part a bare function name cannot express: meshoptimizer stores its codec
version in the first header byte and a decoder refuses streams newer than
it understands, so the JavaScript side needs to see the version to fall
back before it tries. Settling this now avoids fixing the names in two
repositories at once.
`DracoCodec.Encode` returned its payload as an `Int8Array`, which
surfaces every byte above 127 as a negative number and disagrees with
`MeshoptCodec.Decode`. It now returns a `Uint8Array`. The new round-trip
test is what caught this.
Footprint. Both plugins defaulted to `ON`, so every consumer linked
draco before anything could reach the entry points. Both now default to
`OFF`. Measured on Playground, Win32 x64, RelWithDebInfo:
both OFF (new default) 11,100,672 bytes
NativeMeshopt only 11,132,928 bytes +31.5 KB
both ON 12,857,344 bytes +1.68 MB
so meshoptimizer is nearly free and draco accounts for +1.64 MB of the
+15.8% total.
Coverage. `Apps/UnitTests` can reach these entry points even though the
validation suite cannot yet, so both plugins are now linked and
initialized there behind the same options, and `tests.javaScript.all.ts`
grows a describe block for each: an encode/decode round trip for draco,
and for meshopt a byte-for-byte decode of a stream produced by the
reference meshoptimizer 0.22 JavaScript encoder, which pins the native
decoder against the upstream bitstream rather than against itself. Both
cover malformed, truncated and out-of-range input, which matters more now
that the codecs run in-process rather than inside the WASM sandbox. The
blocks report as skipped when the build did not opt in, rather than
passing vacuously. CI enables both options for the three jobs that run
UnitTests.
Finally, bgfx vendors its own copy of meshoptimizer, and the guard in
Dependencies/CMakeLists.txt skips our FetchContent when a target of that
name already exists. The two are reached through different include paths
today, but if that ever resolved the other way we would decode with a
different codec version than this file was written against, which fails
as silent data corruption rather than a build break, so it is now pinned
with a static_assert.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
The glTF decode path enumerated the caller's attributeIds map with Napi::Object::GetPropertyNames. JsRuntimeHost's JavaScriptCore backend implements napi_get_property_names by calling Object.getOwnPropertyNames with an argument count of zero, so the object under inspection is never passed and the call evaluates getOwnPropertyNames(undefined), which throws. JavaScriptCore is the default engine on macOS and iOS, so that decode path could not have worked there. The new round trip test caught this: it is the first coverage to reach the entry point, and it failed on exactly the four JavaScriptCore jobs while every V8, Hermes and Chakra job passed. Enumerate with Object.keys through the global object instead, which goes through napi_call_function with the argument actually supplied and behaves the same on every engine for the plain data objects this map is built from. Fixing napi_get_property_names itself belongs in JsRuntimeHost and is filed separately; doing it here keeps this change from depending on a dependency bump. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
The QuickJS N-API backend in JsRuntimeHost does not surface Napi::Error messages: the seven negative tests that matched on message text failed on MacOS_QuickJS with "Uncaught C++ exception: " and an empty message, while passing on V8, Hermes, Chakra and JSC. Every pre-existing test in this file uses a bare to.throw() for exactly this reason, so follow that convention. Each assertion is still pinned to a specific invalid input, so what is under test is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Draco was 1.64 MB of the plugin's 1.68 MB cost, which is the size concern raised in review. Two changes cut that to 587 KB, a 65% reduction, without losing any capability the JavaScript path has: Drop the encoder. Babylon.js does not bundle one either: its default configuration loads draco_decoder_gltf.wasm, and DracoEncoder fetches a separate draco_encoder.wasm on demand. Exposing DracoCodec.Encode linked the whole encoder into every binary to serve an authoring path Babylon Native does not exercise. Set DRACO_GLTF_BITSTREAM, which builds only mesh compression, normal encoding and the standard edgebreaker. This is the same subset Babylon.js decodes with, since draco_decoder_gltf.wasm is itself the glTF-bitstream build, so no stream the JavaScript path accepts becomes undecodable. It drops point cloud compression, the predictive edgebreaker and pre-glTF backwards compatibility. Point cloud streams now fail with a Draco error instead of decoding, matching Babylon.js. Playground, Win32 x64, RelWithDebInfo: both plugins OFF 11,100,672 meshopt only 11,132,928 +31.5 KB both ON (before) 12,857,344 +1.68 MB (+15.8%) both ON (after) 11,734,528 +619 KB (+5.7%) The encoder tests are replaced by fixtures from the reference draco3dgltf 1.5.7 encoder, which pins the decoder to the upstream bitstream rather than to our own encoder. A 63-vertex sphere fixture covers multi-attribute decoding and a real edgebreaker traversal, which the two-triangle fixture cannot. Its expected counts are what the reference decoder reports for the same buffer: Draco merges points whose attributes all match and drops the degenerate pole triangles, so 63 vertices / 96 triangles encodes to 62 vertices / 91 triangles, and this decoder reproduces that exactly. UnitTests 42 passing, validation suite 295/295. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
…ssue Addresses review feedback on BabylonJS#1797. CI coverage: only build-linux, build-macos and build-win32 passed the plugin options, so build-android, build-ios and build-uwp stopped compiling either plugin once they were defaulted to OFF - dropping exactly the NDK, iOS and WindowsStore toolchains where a new third-party C++ dependency is most likely to break. Turn both on in those three workflows. Android goes through Gradle, so BabylonNative/build.gradle now forwards the two options to CMake when the caller supplies them, leaving the default for local Android builds unchanged. READMEs: add Plugins/NativeDraco/README.md and Plugins/NativeMeshopt/README.md modelled on Plugins/NativeEncoding/README.md, including its experimental banner. Both record that the plugins are off by default, that NativeDraco is decode-only and restricted to the glTF bitstream (no point clouds, no predictive edgebreaker), the JS interface, and that nothing in the pinned babylonjs package calls _native.DracoCodec or _native.MeshoptCodec yet, so the grouping and names have not faced a real consumer. GetPropertyNames workaround: filed as BabylonJS/JsRuntimeHost#216 and referenced from the comment so the helper can be removed when it is fixed. The issue records that the fix is more than JavaScriptCore's missing argument: Node-API specifies the enumerable properties including the prototype chain, which is what V8 returns, while getOwnPropertyNames is own-only and includes non-enumerables. Verified against all four backends in JsRuntimeHost - Chakra (JsGetOwnPropertyNames) matches neither axis, QuickJS (JS_GPN_ENUM_ONLY) filters enumerability but omits the prototype chain - so it is a conformance gap across all three non-V8 backends. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Adds native C++ implementations of the Draco and meshopt mesh decoders as two
new optional plugins,
NativeDracoandNativeMeshopt.Today Babylon Native runs these codecs through the same WASM modules the web
uses. That pulls a WASM runtime into a native app purely to decompress meshes,
costs a module instantiation per session, and copies buffers across the JS/WASM
boundary. These plugins let the native host do the work directly.
What's here
NativeDraco_native.DracoCodec.Decode(data, attributeIds?)draco_decoder_gltf.wasmNativeDraco_native.DracoCodec.VersionNativeMeshopt_native.MeshoptCodec.Decode(source, count, stride, mode, filter?)meshopt_decoder.wasmNativeMeshopt_native.MeshoptCodec.VersionEach codec is a single object rather than a set of free functions, so the
JavaScript side needs one feature probe per codec instead of one per entry
point, and so the object can publish the codec version compiled into the
binary. Version matters because meshoptimizer records its codec version in the
first header byte and a decoder refuses streams newer than it understands; a
bare function name gives a caller no way to find that out except by failing.
DracoCodec.Decodesupports both call shapes the Babylon loaders use: the glTFpath, where the caller passes the
EXT/KHR_draco_mesh_compressionmap ofBabylon vertex-buffer kind to Draco unique id, and the standalone
.drcpath,where no map is supplied and the plugin probes the standard named attributes.
Attribute data is de-interleaved and tightly packed per point, mirroring
emscripten's
GetAttributeDataArrayForAllPointsthat the WASM decoder relieson, so the returned buffers are drop-in equivalents.
Dependencies
Two new
FetchContentdependencies, both pinned to release tags:google/draco@1.5.7zeux/meshoptimizer@v0.22Both are declared
EXCLUDE_FROM_ALLand built with tests, install, executablesand JS glue disabled. draco adds its include directories as
PRIVATE, soDependencies/CMakeLists.txtre-exports them asPUBLICon the combinedtarget; the target is named
dracounder MSVC anddraco_staticelsewhere andboth spellings are handled.
draco is additionally built with
DRACO_GLTF_BITSTREAM=ON, which compiles onlymesh compression, normal encoding and the standard edgebreaker. That is the
same subset
draco_decoder_gltf.wasmis built with, and that module is whatBabylon.js's default configuration loads, so this costs no capability the
JavaScript path has. It drops point cloud compression, the predictive
edgebreaker and pre-glTF backwards compatibility; point cloud streams return a
Draco error instead of decoding, matching Babylon.js.
Footprint, and why both plugins default to
OFFBoth options default to
OFF. Nothing routes to these entry points until theBabylon.js side lands, so until then no consumer should be paying for them.
Measured on Playground, Win32 x64, RelWithDebInfo, same tree, only the two
options changed:
Playground.exeOFF(default)NativeMeshoptonlyONmeshoptimizer is nearly free; draco is +587 KB.
Draco started at +1.64 MB. Two things brought it down, both of which track
Babylon.js more closely rather than less:
DracoDecoderloadsdraco_decoder_gltf.wasmandDracoEncoderfetches a separatedraco_encoder.wasmon demand. Linking the encoder here would cost everybinary ~1.2 MB to serve an authoring path Babylon Native does not exercise.
DRACO_GLTF_BITSTREAM, as described above.Together that is a 65% reduction in draco's contribution, which is close enough
to what a minidraco fork would buy that staying on the upstream bitstream looks
like the better trade.
Trust boundary
Worth stating plainly, because it cuts both ways:
out of the WASM sandbox and into the host process.
net-new capability rather than a relocation of existing capability.
.wasmships in this tree.
That is the reason the input-validation and malformed/truncated-input tests
below exist, and why the codecs validate every caller-supplied extent rather
than trusting the calling loader.
Scope
The change is additive. Nothing existing is modified except the CMake
wiring that registers the new plugins, four lines in
Embedding/Source/Runtime.cppthat initialize them, theUnitTestswiring forthe new coverage, and the three CI workflows that enable the plugins for the
jobs which run
UnitTests. No existing plugin, engine path, or testconfiguration is touched, and
Apps/Playground/Scripts/config.jsonisdeliberately left alone.
On validation
These entry points are not reachable from the currently pinned
babylonjsnpm package (9.15.0), so the plugins are inert until the corresponding
Babylon.js side lands and the loaders start feature-detecting them. The
validation suite therefore cannot exercise this code, but
Apps/UnitTestscan, and now does.
Both plugins are linked and initialized in
Apps/UnitTestsbehind theiroptions, and
tests.javaScript.all.tscarries a describe block for each — 16cases covering decoding and malformed, truncated and out-of-range input for
every entry point. The blocks report as skipped when the build did not opt
in, rather than passing vacuously; CI enables both options for the Linux, macOS
and Win32 jobs that run
UnitTests, including the sanitizer configurations.Neither codec's positive test is self-referential. Since neither plugin exposes
an encoder, both fixtures come from the reference JavaScript encoders, which
pins these decoders to the upstream bitstreams rather than to themselves:
draco3dgltf1.5.7, the same packageBabylon.js takes its decoder from. One is a two-triangle mesh; the other is a
63-vertex sphere carrying POSITION, NORMAL and TEX_COORD, which covers
multi-attribute decoding and a non-degenerate edgebreaker traversal — the
parts
DRACO_GLTF_BITSTREAMactually restricts.with the test asserting the native decoder reproduces the original bytes
exactly.
The sphere fixture is worth a note. It decodes to 62 vertices and 91
triangles, not the 63 and 96 that were encoded: Draco merges points whose
attributes all match and drops the degenerate triangles at the poles. Rather
than adjust the expected numbers to whatever this decoder produced, they were
taken from the reference
draco3dgltfdecoder run over the same buffer, whichreports 62 points and 91 faces. This decoder reproduces that exactly.
meshoptimizer's vertex codec is versioned in the first header byte. Encoders
from 0.23.0 onward emit format version 1 (
0xa1), which a v0.22 decodercorrectly rejects with
-1, so the encoder used to produce the fixture wasversion-matched deliberately. That version is now also pinned at compile time
with a
static_assert, because bgfx vendors its own copy of meshoptimizer andthe dependency guard skips our
FetchContentwhen a target of that namealready exists — a mismatch there would surface as silent data corruption
rather than a build break.
ran=295 passed=295 failed=0 missingRef=0 skipped=420, identical to theupstream
masterbaseline. (Indices 53–57 are excluded from the run: theycrash in
RendererContextD3D11::submiton pristinemastertoo.)JavaScript.All— 42 passing, 0 failing.Note for the Babylon.js side
dracoEncoder.tson the Babylon.js side feature-detects_native.encodeDracoMesh.With no native encoder that probe is simply false and it falls back exactly as
it does today, so nothing breaks — but the paired Babylon.js change should drop
that hunk rather than ship a probe that can never succeed. If the capability is
wanted, the encoder can come back behind an off-by-default option.