OBJFileLoader: Fix AssetContainer handling and add opt-in texture loading waits by noname0310 · Pull Request #18895 · BabylonJS/Babylon.js

noname0310 · 2026-09-09T08:41:19Z

Summary

This PR fixes resource collection and ownership in the OBJ loader and adds an option to wait for referenced textures before completing a load.

Changes

Return created geometries from importMeshAsync and include them in AssetContainer.geometries.
Prevent container-owned geometries, textures, and generated point/line materials from being registered in the scene prematurely, and assign their _parentContainer.
Skip unused MTL materials and their textures instead of creating and subsequently disposing unused materials.
Reuse existing textures when cloning materials for line rendering.
Preserve line markers until all line materials have been processed to avoid unnecessary material cloning.
Honor the per-load invertTextureY option.

Optional texture loading waits

Adds OBJLoadingOptions.waitForTextures defaulting to false to preserve the existing non-waiting behavior.

When enabled:

Referenced texture loading promises are awaited.
Delayed textures are started without requiring the container to be rendered.
Texture loading failures follow materialLoadingFailsSilently.

const container = await LoadAssetContainerAsync("model.obj", scene, {
    pluginOptions: {
        obj: {
            waitForTextures: true,
            materialLoadingFailsSilently: false,
        },
    },
});

Validation

29 OBJ unit tests passed, covering resource ownership, shared textures, optional waiting, delayed loading, and error handling.

Note: The tests were AI-generated, but I have not reviewed their code quality.

…mises

Copilot

🟡 Changes recommended

It introduces incorrect _blockEntityCollection restoration in new/modified code paths, which can break scene collection blocking state and cause entities to register into the wrong collections.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the OBJ/MTL loading pipeline to improve AssetContainer ownership/collection correctness (meshes/geometries/materials/textures) and adds an opt-in waitForTextures loading mode so callers can await referenced texture loads before completing an OBJ load.

Changes:

Include created Geometry instances in OBJFileLoader.importMeshAsync results and propagate them into AssetContainer.geometries.
Add OBJLoadingOptions.waitForTextures (default false) and implement optional awaiting of referenced texture loads, including support for delayed textures.
Improve material/texture handling (skip unused MTL materials, reuse textures for line material cloning, and preserve container ownership).

File summaries

File	Description
packages/dev/loaders/test/unit/OBJ/assetContainer.test.ts	Adds unit tests covering container ownership, geometry returns, material/texture reuse, and optional texture-waiting behavior.
packages/dev/loaders/src/OBJ/solidParser.ts	Adjusts geometry/container ownership behavior and blocks scene collection during container loads.
packages/dev/loaders/src/OBJ/objLoadingOptions.ts	Adds the `waitForTextures?: boolean` option to OBJ loading options.
packages/dev/loaders/src/OBJ/objFileLoader.pure.ts	Returns geometries from `importMeshAsync`, pushes them into `AssetContainer`, and optionally awaits texture load promises from MTL parsing.
packages/dev/loaders/src/OBJ/mtlFileLoader.ts	Extends `parseMTL` to optionally track/await texture loading, filter loaded materials, and honor per-load `invertTextureY`.

Review details

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+                if (materialNames && !materialNames.has(value)) {
+                    material = null;
+                    continue;
+                }


noname0310 · 2026-09-09T08:48:51Z

+                    scene._blockEntityCollection = !!assetContainer;
                    newMaterial = new StandardMaterial(Geometry.RandomId(), scene);
+                    newMaterial._parentContainer = assetContainer;
+                    scene._blockEntityCollection = false;


I'm tired of seeing these pointless AI reviews...

this is actually valid. In SolidParser lines 924–927, _blockEntityCollection is unconditionally reset to false. This is only safe if its previous value was false and material construction cannot throw - neither is guaranteed. The similar glTF implementation means glTF has the same pre-existing weakness; it does not justify introducing it in new OBJ code. The new line-material path at objFileLoader.pure.ts lines 397–401 also restores on success but lacks try/finally.

RaananW

🤖 This review comment was created by an AI agent.

Requesting changes for one new correctness blocker:

OBJFileLoader._parseSolidAsync (objFileLoader.pure.ts:356-363): with waitForTextures: true and the default materialLoadingFailsSilently: true, Promise.all rejects as soon as any texture fails; the catch immediately resolves the outer MTL promise, so the OBJ load can complete while other referenced textures are still pending. This violates the option’s wait contract. In silent mode, catch each texture rejection (or use Promise.allSettled) and still await every texture; add a multi-texture regression test where one fails before another settles.

Existing review threads: Copilot’s solidParser.ts restoration concern remains valid and unresolved, so I did not duplicate it inline. Its mtlFileLoader.ts comment describes behavior already present on the base commit and is not counted as PR-introduced.

CI: repository build/test/lint/typecheck checks have not run; only GitGuardian passed. This review requests changes for the code issue above, not unfinished CI.

noname0310 · 2026-09-09T10:34:29Z

@RaananW

The _blockEntityCollection restoration case mentioned here is also not handled by GLTFFileLoader. Its _createDefaultMaterial method follows the same pattern

see: https://github.com/BabylonJS/Babylon.js/blob/master/packages/dev/loaders/src/glTF/2.0/glTFLoader.pure.ts#L2415-L2432

OBJ does the same as glTF, so I’d leave this for a separate PR.

The waitForTextures issue has been fixed, with regression tests

RaananW · 2026-09-09T14:33:49Z

Commented about the copilot comment. The older Copilot comment on mtlFileLoader.ts describes pre-existing code and should not block this PR

RaananW

🤖 This review comment was created by an AI agent.

Re-review verdict: 🔴 Request changes

The follow-up commit fixes the previously reported waitForTextures contract: Promise.allSettled now waits for every referenced texture before propagating or silently handling failures, and the new two-texture tests cover both completion and failure of the remaining request.

One previously raised correctness blocker remains on the current head: _blockEntityCollection is still not restored with try/finally around the newly container-aware material constructors in SolidParser.parse (solidParser.ts:924-927) and OBJFileLoader._parseSolidAsync (objFileLoader.pure.ts:397-401). A constructor failure leaves the scene-wide flag altered, while a pre-existing blocked state can be clobbered. The analogous MTL path predates this PR and is not counted as a new blocker. Please preserve the prior value and restore it in finally for these new paths.

CI: ⚠️ Missing coverage — only GitGuardian is reported; build, tests, lint/format, and type-check checks are absent.
CI safety: ✅ No credential-exposure risk found in the changed source/tests.

Popov72 · 2026-09-10T09:14:30Z

/azp run

azure-pipelines · 2026-09-10T09:14:45Z

Azure Pipelines successfully started running 1 pipeline(s).

bjsplat · 2026-09-10T09:15:59Z

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

bjsplat · 2026-09-10T09:16:00Z

Reviewer - this PR has made changes to the build configuration file.

This build will release a new package on npm

If that was unintentional please make sure to revert those changes or close this PR.

bjsplat · 2026-09-10T09:37:03Z

Snapshot stored with reference name:
refs/pull/18895/merge

Test environment:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18895/merge/index.html

To test a playground add it to the URL, for example:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18895/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/18895/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/18895/merge
https://gui.babylonjs.com/?snapshot=refs/pull/18895/merge
https://nme.babylonjs.com/?snapshot=refs/pull/18895/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/18895/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.

bjsplat · 2026-09-10T10:01:25Z

Interaction tests

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18895/merge/testResults/interactionplaywright/index.html

bjsplat · 2026-09-10T10:09:14Z

🟢 Memory Leak Test Results

4 passed, 0 leaked out of 4 scenarios

🟢 All memory leak tests passed — no leaks detected.

Passed Scenarios (4)

Scenario	Package
Core Playground #2FDQT5#1508	@babylonjs/core
Core Playground #T90MQ4#14	@babylonjs/core
Core Playground #8EDB5N#2	@babylonjs/core
Core Playground #LL5BIQ#636	@babylonjs/core

bjsplat · 2026-09-10T10:16:34Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18895/merge/testResults/webgpuplaywright/index.html

bjsplat · 2026-09-10T10:16:51Z

⚡ Performance Test Results

🟢 All performance tests passed — no regressions detected.

bjsplat · 2026-09-10T10:26:59Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18895/merge/testResults/webgl2playwright/index.html

Remove the four previous-value snapshots introduced by this PR and reset _blockEntityCollection to false, matching the existing glTF and OBJ loader pattern. Keep the existing finally blocks and material texture cleanup.

noname0310 · 2026-09-14T13:42:42Z

OBJ's _blockEntityCollection handling is intended to follow the existing glTF loader convention:
set it to !!assetContainer while creating or applying a resource, assign _parentContainer, and then reset it to false.

for instance:

Babylon.js/packages/dev/loaders/src/glTF/2.0/glTFLoader.pure.ts

Lines 2415 to 2419 in deab6a2

    
           private _createDefaultMaterial(name: string, babylonDrawMode: number, impl: Readonly<PBRMaterialImplementation>): Material { 
        
               this._babylonScene._blockEntityCollection = !!this._assetContainer; 
        
               const babylonMaterial = new impl.materialClass(name, this._babylonScene); 
        
               babylonMaterial._parentContainer = this._assetContainer; 
        
               this._babylonScene._blockEntityCollection = false;

If anything, the code elsewhere that saves and restores the previous state appears to be the result of a loss of context.

deltakosh

Hi! Thanks for the detailed loader work. I found two correctness issues in the new texture-waiting path:\n\n1. A second waitForTextures load can hang forever when it reuses an already-failed cached texture. The new wrapper subscribes after the cached texture's error notification has fired, so its deferred promise never settles. Please retry or replay the cached failure state and cover a second load of the same failed URL.\n\n2. With waitForTextures: true and materialLoadingFailsSilently: false, a texture failure rejects before the created meshes, geometries, materials, and textures are copied into the AssetContainer. Because scene collection was blocked and the catch only clears _assetContainer, those resources are orphaned and cannot be disposed by the caller. Please track or clean up all resources on this rejection path and add a reference-count regression test.\n\nThe earlier _blockEntityCollection restoration concern is not part of this review. The changelog label and full required CI are also still missing. Thanks for taking another pass at these edge cases!

RaananW

🤖 This review comment was created by an AI agent.

Re-review verdict: 🔴 Request changes

The prior _blockEntityCollection blocker is still open on this head. The latest commit removes the previous-value snapshots and resets the scene-wide flag to false. That can still clobber a caller's existing blocked state, and the new point-cloud and line-material constructor paths still do not restore the flag if construction throws:

Please preserve the previous value and restore it in finally for the new paths. The two texture-waiting issues raised in the current team review are also blocking and are not repeated here.

CI: ⚠️ Missing coverage — only GitGuardian is reported on this head; build, tests, lint/format, and type-check checks are absent.
CI safety: ✅ No credential-exposure risk found in the changed source and tests.

noname0310 · 2026-09-15T02:02:35Z

@deltakosh I've fixed the cleanup when loading throws.

The missing onError callback when reusing a cached InternalTexture was identified as a bug and addressed in #11217 in 2021, but it appears to still persist.

Would it be okay if I opened a separate PR to fix it?

Popov72 · 2026-09-15T08:50:33Z

/azp run

azure-pipelines · 2026-09-15T08:50:48Z

Azure Pipelines successfully started running 1 pipeline(s).

bjsplat · 2026-09-15T09:27:23Z

Interaction tests

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18895/merge/testResults/interactionplaywright/index.html

bjsplat · 2026-09-15T09:29:41Z

🟢 Memory Leak Test Results

4 passed, 0 leaked out of 4 scenarios

🟢 All memory leak tests passed — no leaks detected.

Passed Scenarios (4)

Scenario	Package
Core Playground #2FDQT5#1508	@babylonjs/core
Core Playground #T90MQ4#14	@babylonjs/core
Core Playground #8EDB5N#2	@babylonjs/core
Core Playground #LL5BIQ#636	@babylonjs/core

bjsplat · 2026-09-15T09:39:57Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18895/merge/testResults/webgl2playwright/index.html

bjsplat · 2026-09-15T09:46:07Z

⚡ Performance Test Results

🟢 All performance tests passed — no regressions detected.

RaananW

🤖 This review comment was created by an AI agent.

Thanks for the cleanup update. Resources are now tracked as they are created, and a failed container is disposed. This fixes the earlier orphaned-resource concern.

I still need these changes:

The earlier _blockEntityCollection blocker is still open in the new point and line material paths. They replace the caller's prior value with false. A constructor error can also leave the value changed. Please save the old value and restore it in finally: solidParser.ts:924-928 and objFileLoader.pure.ts:373-398.
waitForTextures: true can still wait forever on a second load of an already failed cached texture. _GetTexture only receives future callbacks, after the cached error was already sent. Please retry the failed texture or replay its error, and add a second-load regression test: mtlFileLoader.ts:267-276.
Required CI is not green. Monorepo CI (Memory leak tests (packages suite)) and Monorepo CI (Visualization tests - WebGL 2) are failing. Performance and WebGPU checks are still pending.

CI safety: I found no credential exposure risk in the changed source and tests.

bjsplat · 2026-09-15T09:55:22Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18895/merge/testResults/webgpuplaywright/index.html

RaananW · 2026-09-17T16:30:38Z

any update here?

noname0310 added 4 commits September 9, 2026 17:12

fix assetContainer resource collection, introduce texture loading pro…

1b2a103

…mises

return texture loading promises from parseMTL

1460af7

add waitForTextures option

133f9fb

add test

b483c23

Copilot AI lite review requested due to automatic review settings September 9, 2026 08:41

Copilot started reviewing on behalf of noname0310 September 9, 2026 08:41 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

RaananW self-requested a review September 9, 2026 09:17

RaananW requested changes Sep 9, 2026

View reviewed changes

handle silent texture failures

6c47b07

RaananW requested changes Sep 10, 2026

View reviewed changes

Align OBJ entity collection resets with glTF

6493880

Remove the four previous-value snapshots introduced by this PR and reset _blockEntityCollection to false, matching the existing glTF and OBJ loader pattern. Keep the existing finally blocks and material texture cleanup.

deltakosh self-requested a review September 14, 2026 14:33

deltakosh requested changes Sep 14, 2026

View reviewed changes

RaananW requested changes Sep 14, 2026

View reviewed changes

handle disposal on load failure

efdd76b

RaananW requested changes Sep 15, 2026

View reviewed changes

Conversation

noname0310 commented Sep 9, 2026

Summary

Changes

Optional texture loading waits

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

noname0310 Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

RaananW Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

RaananW left a comment

Choose a reason for hiding this comment

Uh oh!

noname0310 commented Sep 9, 2026

Uh oh!

RaananW commented Sep 9, 2026

Uh oh!

RaananW left a comment

Choose a reason for hiding this comment

Uh oh!

Popov72 commented Sep 10, 2026

Uh oh!

azure-pipelines Bot commented Sep 10, 2026

Uh oh!

bjsplat commented Sep 10, 2026

Uh oh!

bjsplat commented Sep 10, 2026

Uh oh!

bjsplat commented Sep 10, 2026

Uh oh!

bjsplat commented Sep 10, 2026

Uh oh!

bjsplat commented Sep 10, 2026

🟢 Memory Leak Test Results

Uh oh!

bjsplat commented Sep 10, 2026

Uh oh!

bjsplat commented Sep 10, 2026

⚡ Performance Test Results

Uh oh!

bjsplat commented Sep 10, 2026

Uh oh!

noname0310 commented Sep 14, 2026

Uh oh!

deltakosh left a comment

Choose a reason for hiding this comment

Uh oh!

RaananW left a comment

Choose a reason for hiding this comment

Uh oh!

noname0310 commented Sep 15, 2026

Uh oh!

Popov72 commented Sep 15, 2026

Uh oh!

azure-pipelines Bot commented Sep 15, 2026

Uh oh!

bjsplat commented Sep 15, 2026

Uh oh!

bjsplat commented Sep 15, 2026

🟢 Memory Leak Test Results

Uh oh!

bjsplat commented Sep 15, 2026

Uh oh!

bjsplat commented Sep 15, 2026

⚡ Performance Test Results

Uh oh!

RaananW left a comment

Choose a reason for hiding this comment

Uh oh!

bjsplat commented Sep 15, 2026

Uh oh!

RaananW commented Sep 17, 2026

Uh oh!