Properly await async GUI playground setup in visual tests#18506
Properly await async GUI playground setup in visual tests#18506bghgary wants to merge 3 commits into
Conversation
The viz test runner had a special-case `guiIsReady()` gate that kept rendering an extra frame after all `AdvancedDynamicTexture`s reported ready, on top of the per-test `renderCount`. This was added to give GUI-based PGs time to finish loading, but it does not actually solve the race for PGs that fire-and-forget `parseFromSnippetAsync` / `parseFromURLAsync`: when the snippet fetch is still in flight, `guiIsReady()` walks an empty root container and returns `true` immediately. The gate also bakes GUI-specific knowledge into a generic runner. The correct contract is: a PG whose setup is async returns `Promise<Scene>` from `createScene` and the runner awaits it. The runner already does `await (createScene(engine, canvas) ?? createScene())`, so no runner change is needed beyond removing the gate. Updates the two PGs that triggered the workaround to actually follow this contract, by saving new revisions that return the parse promise resolving to the scene: - `#YS93KY#0 -> #YS93KY#1` Load GUI snippet with unicode - `#ERVGT5#0 -> #ERVGT5#1` Parse GUI json with unicode The ADT-side `guiIsReady()` / `onGuiReadyObservable` API is kept; this only removes its use in the visualization test runner. [Created by Copilot on behalf of @bghgary] 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). |
There was a problem hiding this comment.
Pull request overview
Removes the GUI-specific AdvancedDynamicTexture.guiIsReady() post-render gate from the visualization test render loops, and instead relies on async Playground scenes returning Promise<Scene> (by pinning affected tests to new Playground snippet revisions).
Changes:
- Removed the
guiIsReady()-based extra rendering logic from both visualization render-loop evaluators (test tools + Playwright runner). - Updated visualization test config to use new Playground snippet revisions (
#YS93KY#1,#ERVGT5#1) that returnPromise<Scene>for async GUI parsing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/tools/testTools/src/visualizationUtils.ts | Drops the ADT guiIsReady() render gate from the non-Playwright visualization render loop. |
| packages/tools/tests/test/playwright/visualizationPlaywright.utils.ts | Drops the ADT guiIsReady() render gate from the Playwright visualization render loop. |
| packages/tools/tests/test/visualization/config.json | Pins the two GUI-unicode visualization tests to updated Playground snippet revisions. |
Comments suppressed due to low confidence (1)
packages/tools/testTools/src/visualizationUtils.ts:219
- In the
renderCount <= 0branch whenscene.isReady()is false, the promise resolvesfalsebut the engine render loop is not stopped. This can leave a runaway render loop after a failure and interfere with subsequent steps/tests; stop the render loop before resolving false in this branch.
window.engine && window.engine.stopRenderLoop();
return resolve(true);
} else {
console.error("Scene is not ready after rendering is done");
return resolve(false);
|
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/18506/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/18506/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/18506/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. |
bghgary
changed the title
bghgary
changed the title
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
🟢 Memory Leak Test Results13 passed, 0 leaked out of 13 scenarios 🟢 All memory leak tests passed — no leaks detected. Passed Scenarios (13)
|
⚡ Performance Test Results🟢 All performance tests passed — no regressions detected. |
When `renderCount <= 0` but `scene.isReady()` is false, the code resolves `false` but did not stop the engine render loop, leaving it running after the test failure. Add `engine.stopRenderLoop()` in the failure branch in both runners. Pre-existing bug in the same code paths this PR is editing; fixing it alongside the gate removal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Combine the success and failure branches to avoid duplicating `engine.stopRenderLoop()`. The render loop is stopped whenever the scene isn't ready or when not in `continueRenderingOnDone` mode; the log + resolve value follows from the readiness check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
|
Visualization tests for WebGPU |
🟢 Memory Leak Test Results13 passed, 0 leaked out of 13 scenarios 🟢 All memory leak tests passed — no leaks detected. Passed Scenarios (13)
|
|
WebGL2 visualization test reporter: |
⚡ Performance Test Results🟢 All performance tests passed — no regressions detected. |
Context
The visualization test runner has special-case logic that, after the per-test
renderCountis exhausted, keeps rendering until everyAdvancedDynamicTexturereportsguiIsReady()and then renders one more frame. This was introduced in #13475 to give GUI-based PGs time to finish loading before the screenshot is captured.It doesn't actually solve the race for PGs that fire-and-forget
parseFromSnippetAsync/parseFromURLAsync: while the snippet fetch is still in flight, the ADT root container has zero children,Container.isReady()iterates an empty array and returnstrue, soguiIsReady()is satisfied immediately. The gate only adds a settle frame for image loads on controls that are already in the tree.It also bakes GUI-specific knowledge into a generic test runner. The correct contract is: a PG whose setup is async returns
Promise<Scene>fromcreateSceneand the runner awaits it. The runner already doesawait (createScene(engine, canvas) ?? createScene()), so no runner-side framework change is needed beyond removing the gate.Changes
guiIsReady()gate from both viz runners:packages/tools/tests/test/playwright/visualizationPlaywright.utils.tspackages/tools/testTools/src/visualizationUtils.tsPromise<Scene>contract, by saving new revisions:#YS93KY#0→#YS93KY#1(Load GUI snippet with unicode) —return adv.parseFromSnippetAsync("#KHPNS9").then(() => scene);#ERVGT5#0→#ERVGT5#1(Parse GUI json with unicode) —return adv.parseFromURLAsync(...).then(() => scene);Notes
AdvancedDynamicTexture.guiIsReady()/onGuiReadyObservableAPI itself is not removed — only its usage in the test runner. It is still used internally byFrameGraph/guiTask.tsand remains available to anyone who wants it.[Created by Copilot on behalf of @bghgary]