Add TypeScript FBX loader#18483
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PatrickRyanMS
requested review from
Popov72,
RaananW,
bghgary,
deltakosh and
sebavan
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
I added two initial test models for visual testing to the assets repo. I need to build out all of the visual test variations, but wanted to get this in draft PR for review since it's large while I build out the visual test assets. |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18483/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/18483/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/18483/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
⚡ Performance Test Results🟢 All performance tests passed — no regressions detected. |
🟢 Memory Leak Test Results13 passed, 0 leaked out of 13 scenarios 🟢 All memory leak tests passed — no leaks detected. Passed Scenarios (13)
|
|
Just explaining the reason it fails on native (before fully reviewing this) - This PR uses BigInt syntax ( |
|
@RaananW I'll take care of it. We dont need bigint |
Remove bigint usage from the FBX loader and harden parser validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document SceneLoader callback parameters for FBX public loader methods. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18483/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/18483/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/18483/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
⚡ Performance Test Results🟢 All performance tests passed — no regressions detected. |
🟢 Memory Leak Test Results13 passed, 0 leaked out of 13 scenarios 🟢 All memory leak tests passed — no leaks detected. Passed Scenarios (13)
|
There was a problem hiding this comment.
Is the specs folder a thing now? I only see one file in this folder right now for tree shaking.
Popov72
left a comment
Popov72
left a comment
There was a problem hiding this comment.
Code review comments from Copilot CLI. I left out the quality-gate notes as requested.
|
|
||
| if (mapping === "AllSame") { | ||
| // All polygons use material index 0 | ||
| const indices = new Int32Array(polygonCount); |
There was a problem hiding this comment.
For MappingInformationType === "AllSame", this always assigns material slot 0. FBX files can store a non-zero single slot in Materials[0]; with multiple connected materials that means the mesh renders with the wrong material. Please read the Materials array for the all-same case and preserve a non-zero slot through the later all-same optimization (or assign the resolved material directly).
|
|
||
| // Convert indices | ||
| let indices: Uint32Array; | ||
| if (rawIndices instanceof Int32Array) { |
There was a problem hiding this comment.
ASCII FBX shorthand arrays are parsed as Float64Array, so shape Indexes from ASCII files will not pass this Int32Array/Uint32Array check and the morph target is silently dropped. Please accept/convert numeric arrays here the same way geometry and skeleton index arrays do.
| return []; | ||
| } | ||
|
|
||
| const keyTimes = toInt64Array(keyTimeNode.properties[0]?.value); |
There was a problem hiding this comment.
For ASCII FBX files, KeyAttrFlags and KeyAttrRefCount are parsed as Float64Array, but the helpers below only accept Int32Array. That causes constant/cubic interpolation flags to be ignored and curves default to linear. Please convert the ASCII numeric arrays before interpreting key attributes.
| if (model.geometry && model.subType === "Mesh") { | ||
| // Create mesh | ||
| if (nameFilter && !nameFilter(model.name)) { | ||
| return; |
There was a problem hiding this comment.
Returning here skips the entire subtree when a parent mesh does not match meshesNames. If a requested mesh is nested under an unrequested mesh, importMeshAsync will never recurse far enough to import it. Please continue traversing children (likely with an uncreated/placeholder parent or adjusted hierarchy handling) instead of returning immediately.
| */ | ||
|
|
||
| /** Individual property value within an FBX node */ | ||
| export type FBXPropertyValue = boolean | number | string | Float32Array | Float64Array | Int32Array | Uint8Array; |
There was a problem hiding this comment.
FBX object IDs are 64-bit values, but the shared IR stores all int64 values as JS number. IDs above Number.MAX_SAFE_INTEGER can lose precision and collide in connection maps, which can miswire materials, skins, animations, and model hierarchy. Please preserve 64-bit IDs exactly (for example as bigint/string for IDs) or reject unsafe IDs with a clear parse error.
| throw new Error("Not a valid binary FBX file"); | ||
| } | ||
|
|
||
| const version = view.getUint32(23, true); |
There was a problem hiding this comment.
A truncated binary file that contains the 21-byte magic but is shorter than the 27-byte header reaches getUint32(23, true) and throws a raw RangeError. Please validate buffer.byteLength >= HEADER_SIZE before decoding the version so malformed input produces a controlled FBX parse error.
| @@ -0,0 +1,236 @@ | |||
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | |||
| import { NullEngine } from "core/Engines"; | |||
There was a problem hiding this comment.
This import trips the repo lint rule babylonjs/no-directory-barrel-imports because core/Engines resolves through a directory index. Please import NullEngine from core/Engines/nullEngine instead.
6 participants
Summary
specs/fbx-loaderdocumentation covering goals, requirements, architecture, normal-map handling, texture loading, and deferred grayscale bump-map conversion.Notes
BumpandBumpFactorslots are treated as normal-map-like inputs for compatibility until true grayscale height-to-normal conversion is implemented.Validation
npm run compile -w @dev/loadersnpm run test -- packages/dev/loaders/test/unitnpm run format:checknpm run lint:checkcurrently fails on pre-existing core tree-shaking manifest/side-effect-stub drift outside the FBX changes.