Conversation
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Building or testing the sandbox has failed. If the tests failed, results can be found here: |
|
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/18251/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/18251/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/18251/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. |
|
You have made possible changes to the playground. https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/18251/merge/ The snapshot playground with the CDN snapshot (only when available): Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly. |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
You have made possible changes to the playground. https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/18251/merge/ The snapshot playground with the CDN snapshot (only when available): Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly. |
|
Building or testing the sandbox has failed. If the tests failed, results can be found here: |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
There was a problem hiding this comment.
Pull request overview
Adds an OpenPBR toggle to the Babylon.js Sandbox so glTF assets can be (re)loaded using OpenPBRMaterial via the glTF loader’s useOpenPBR flag, with the preference persisted in local storage.
Changes:
- Persist OpenPBR mode in local storage and initialize
GlobalState.useOpenPBRfrom it on startup. - Add a footer toggle button (with active styling) that flips OpenPBR mode and triggers a scene reload.
- Wire
GlobalState.useOpenPBRintoGLTFFileLoader.useOpenPBRduring plugin activation.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/tools/sandbox/src/tools/localStorageHelper.ts | Adds SetUseOpenPBR / GetUseOpenPBR persistence helpers. |
| packages/tools/sandbox/src/sandbox.tsx | Initializes useOpenPBR from local storage at app startup. |
| packages/tools/sandbox/src/img/icon-openpbr.png | Adds the OpenPBR toggle icon asset. |
| packages/tools/sandbox/src/globalState.ts | Adds useOpenPBR state and onRequestSceneReload observable. |
| packages/tools/sandbox/src/custom.d.ts | Adds TS module declarations for .scss and .png imports. |
| packages/tools/sandbox/src/components/renderingZone.tsx | Adds reload helper, listens for reload requests, and sets loader.useOpenPBR. |
| packages/tools/sandbox/src/components/footerButton.tsx | Supports an active state for toggle-style buttons. |
| packages/tools/sandbox/src/components/footer.tsx | Adds the OpenPBR toggle button and persists/requests reload on change. |
| public override componentWillUnmount(): void { | ||
| if (this._onRequestSceneReloadObserver) { | ||
| this.props.globalState.onRequestSceneReload.remove(this._onRequestSceneReloadObserver); | ||
| this._onRequestSceneReloadObserver = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
componentWillUnmount cleans up the onRequestSceneReload observer, but initEngine() also registers window-level resize and keydown listeners that aren’t removed. If this component is ever unmounted/remounted (e.g., hot reload), those handlers will accumulate and can cause duplicate reload/resize behavior; consider storing the handler functions and removing them here as well.
|
Inspector has gltf loader options. Is it sufficient to just add the OpenPBR option rather than making changes to Sandbox? |
|
I added missing options (include |
Here's a quick idea of how to add a toggle to the Sandbox to load glTF files with the OpenPBRMaterial instead of the PBRMaterial.