Skip to content

Wait for validation readiness without consuming rendered frames - #1890

Merged
bkaradzic-microsoft merged 10 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:pr/native-validation-readiness
Oct 1, 2026
Merged

bkaradzic-microsoft merged 10 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:pr/native-validation-readiness

Conversation

@bkaradzic-microsoft

@bkaradzic-microsoft bkaradzic-microsoft commented Sep 17, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Wait for scene readiness, GUI image readiness, and clean material defines with ready effects in the active camera's render pass. Include utility scenes using the main scene's camera.
  • Restore the prior render pass on every exit, including errors.
  • Refresh render IDs without rendering extra animation/particle frames; preserve renderCount, screenshot indices, and RenderDoc capture/finalization budgets.
  • Allow up to 240 non-rendering wait ticks; on the next readiness check, render if ready or fail if still unready (tick 241 for a continuously unready scene). Preserve once-only cleanup and suite continuation; reject stale callbacks after readiness timeout or failure.
  • Restore the original seeded Math.random function as well as its seed between scenes.
  • Fail fast on terminal engine effect-error notifications while preserving fallbacks and ignoring disposed effects.

The standalone simulated-host Node test and its dedicated CI job have been removed. The production runner fixes remain unchanged.

Independence

Based on upstream master. Uses existing published Babylon.js 9.21.2 APIs. No JavaScript engine changes, dependency updates, protocol changes, engine-option changes, fixture/reference changes, capture-count changes, or tolerance changes.

Validation

Previously completed native validation of the unchanged production runner:

  • Stock Windows x64 / D3D11 / Chakra / RelWithDebInfo Playground: Native Canvas, setParent, and GUI Gradient Linear with transparency passed with unchanged fixtures and gates.
  • After the utility-scene readiness correction, Near Menu, Native Canvas, setParent, and the transparent GUI gradient passed together on Linux QuickJS/OpenGL/llvmpipe and Windows Chakra/D3D11.
  • An actual invalid GLSL ShaderMaterial followed by a black-frame control on stock Windows x64 / D3D11 / Chakra / Release produced ran=2 passed=1 failed=1 missingRef=0 in 2.121 seconds; the expected process exit was -1.

These are native runtime results, not claims of retained standalone Node test coverage.

Linux CI follow-up

The initial readiness wait now includes associated utility scenes. Previously, Linux QuickJS Near Menu exhausted 240 convergence checks in 114 ms with five utility-scene loads still pending and zero rendered frames because executeWhenReady covered only the main scene.

The existing initial readiness wait and 10-minute timeout now cover associated utility scenes too. Resource loads therefore complete before bounded convergence polling begins. The 240-check limit, canonical Near Menu render count (60), references, and pixel tolerances are unchanged.

Terminal shader error follow-up

  • Fail pending scene creation or scene processing outside the compiler's notification callback.
  • Ignore remaining fallbacks, retained ready pipelines, and disposed effects.
  • Remove observers/timers and invalidate pending readiness/render/screenshot callbacks; late-loaded scenes cannot overwrite the next test.
  • Keep the existing readiness budgets, render counts, references, tolerances, dependency pins, and Babylon.js 9.21.2 unchanged.

This does not claim unsupported WGSL preprocessing now reports an error on stock Babylon.js. Early WGSL rejection and asynchronous shader-preparation fixes remain separate Babylon.js changes. This PR consumes terminal errors that the released engine already emits.

Copilot AI lite review requested due to automatic review settings September 17, 2026 21:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved moderate issues remain in render-pass handling and asynchronous framebuffer evaluation.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds readiness polling for native validation without consuming rendered frames, preserving deterministic state and adding standalone regression coverage.

Changes:

  • Waits for scene, GUI, material, effect, and utility-scene readiness.
  • Preserves render budgets, render-pass state, cleanup, and seeded randomness.
  • Adds documentation, Node tests, and CI execution.
File summaries
File Summary
Documentation/AddingNewValidationTests.md Documents readiness and deterministic capture behavior.
Apps/Playground/Tests/validation_native.test.cjs Adds standalone host-independent runner regression tests.
Apps/Playground/Scripts/validation_native.js Implements readiness polling and lifecycle handling; findings remain regarding pass ordering/restoration and stale asynchronous evaluation.
.github/workflows/ci.yml Runs the standalone validation tests in CI.
Review details

Suppressed comments (1)

Apps/Playground/Scripts/validation_native.js:318

  • scene.isReady() runs before currentRenderPassId is switched to the active camera pass. Babylon's readiness path calls mesh.isReady(true), which prepares/checks pass-specific draw wrappers; the later materialDefines/effect reads can therefore see an uninitialized camera-pass wrapper and treat the missing values as ready, allowing the first captured frame before that pass's effect is compiled. Select the camera pass before the entire readiness check and keep it inside the existing try/finally.
        if (!scene.isReady()) {
  • Files reviewed: 4/4 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Apps/Playground/Scripts/validation_native.js Outdated
Comment thread Apps/Playground/Scripts/validation_native.js
@bkaradzic-microsoft
bkaradzic-microsoft requested a balanced review from Copilot September 17, 2026 22:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

Comment thread Apps/Playground/Scripts/validation_native.js
Comment thread Apps/Playground/Scripts/validation_native.js
Comment thread Apps/Playground/Scripts/validation_native.js
Comment thread Documentation/AddingNewValidationTests.md

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread Apps/Playground/Scripts/validation_native.js Outdated
Comment thread Apps/Playground/Scripts/validation_native.js
Comment thread Apps/Playground/Tests/validation_native.test.cjs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread Apps/Playground/Scripts/validation_native.js Outdated
Comment thread Apps/Playground/Scripts/validation_native.js
Comment thread Apps/Playground/Scripts/validation_native.js

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Initial readiness can hang indefinitely when a pending associated utility scene is disposed or detached.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread Apps/Playground/Scripts/validation_native.js Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Utility layers created before the main camera can bypass both readiness phases.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread Apps/Playground/Scripts/validation_native.js Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The asynchronous lifecycle handling and private utility-layer instrumentation warrant final human validation across supported native engines.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Native asynchronous lifecycle handling and private utility-layer instrumentation warrant final human validation.

Review effort: Balanced
Findings: None

bkaradzic-microsoft and others added 2 commits September 22, 2026 13:05
Check GUI images and active-pass material readiness, including associated
utility scenes, before counting rendered frames. Fail explicitly after
240 waiting ticks and preserve once-only cleanup and capture indices.
Stop late readiness/render callbacks after completion or failure.

Restore both the seeded random function and its seed between tests.
Add full-script host-independent regression coverage and a CI job.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Protect scene and GUI readiness with the render-pass restoring finally
block, not only the material inspection. Invalidate queued screenshot
evaluation on each failure path before cleaning up the scene.

Add regressions for scene and GUI exceptions, delayed screenshots after
render/readiness/convergence/initial-timeout failure, and delayed success.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
bkaradzic-microsoft and others added 6 commits September 22, 2026 13:05
Apply the existing initial readiness wait and timeout to associated
utility scenes, not just the main scene. Near Menu can otherwise exhaust
240 unrendered ticks while its utility scene is still loading models.

Reproduced on Linux QuickJS/OpenGL with llvmpipe: 240 checks in 114ms,
five pending utility-scene loads, and zero rendered frames. The unchanged
Near Menu fixture and three GUI controls now pass on Linux and Windows.
Add both callback-order regressions and utility-timeout/late-callback
coverage. Preserve all frame counts and the 240-check convergence limit.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Track timer identities and due times, support real cancellation, and load
synthetic playground snippets through the runner's existing Promise path.
Cover immediately and asynchronously resolved scenes, exact render counts,
and absence of stale timeout failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Match Scene.isReady's nullish pass fallback without treating pass zero
as missing. Cover missing/null/undefined camera passes and utility scenes
attached during convergence; document why that scene set is recomputed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
Reconcile associated scenes while waiting instead of relying on a fixed
callback count. Drop removed scenes and enroll new ones, with a runner-owned
deadline that remains active when scene disposal clears readiness observers.

Cancel readiness timers and ignore late callbacks after the wait ends.
Cover disposal, detachment, attachment, independent timeout, and stale
callbacks without changing rendered-frame or screenshot budgets.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
Record UtilityLayerRenderer's public scene association during its existing
constructor-time camera update. Use that association during readiness
instead of requiring the utility scene to have copied the main camera.

Keep camera matching for other virtual scenes and cover early utility-layer
creation without waiting on unrelated camera-less virtual scenes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
Use the published engine effect-error observable to reject pending scene
creation or fail scene processing outside compiler notification callbacks.
Cancel readiness and screenshot callbacks, remove observers and timers,
and dispose late-loaded scenes without changing the following test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
@bkaradzic-microsoft
bkaradzic-microsoft force-pushed the pr/native-validation-readiness branch from efa46ee to cc74dff Compare September 22, 2026 20:05
bkaradzic-microsoft added a commit to bkaradzic-microsoft/BabylonNative that referenced this pull request Sep 30, 2026
Review-Group: E6
Source: PR BabylonJS#1890
Squashed final review changes, including regressions and review follow-ups.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Comment thread Apps/Playground/Tests/validation_native.test.cjs Outdated
Keep the production readiness fixes and remove the bespoke Node test,
its dedicated CI job, and its documented invocation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e6c3c123-c355-4790-a3d4-94337ed6e052
@bkaradzic-microsoft
bkaradzic-microsoft enabled auto-merge (squash) October 1, 2026 14:34
The waits belong in the examples. They remain in the runner only because
fixing each validation test individually is a much larger task.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e6c3c123-c355-4790-a3d4-94337ed6e052

@bghgary bghgary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI has more comments, but feel free to ignore them since this will be replaced soon.

Comment thread Apps/Playground/Scripts/validation_native.js
Comment thread Apps/Playground/Scripts/validation_native.js
Comment thread Apps/Playground/Scripts/validation_native.js
Comment thread Apps/Playground/Scripts/validation_native.js
@bkaradzic-microsoft
bkaradzic-microsoft merged commit d4fed19 into BabylonJS:master Oct 1, 2026
35 checks passed
bkaradzic-microsoft added a commit to bkaradzic-microsoft/BabylonNative that referenced this pull request Oct 7, 2026
BabylonJS#1890 is being replaced, so revert the validation_native.js and
documentation changes made in response to its review.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c46f0590-2707-466b-93c7-bb74316f385a
bkaradzic-microsoft added a commit that referenced this pull request Oct 8, 2026
Follow-up for review comments on #1885 and #1897, rebased onto
`3428526f` after #1913 merged.

- Reject multisampled cube targets and zero-initialize every cube face
before its first clear.
- Clarify that `_native.decodeMeshopt` is a compatibility export unused
by stock Babylon.js.

The validation-runner follow-ups for #1890 were dropped because #1890 is
being replaced.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Branimir Karadzic <bkaradzic@users.noreply.github.com>
Copilot-Session: e6c3c123-c355-4790-a3d4-94337ed6e052
Copilot-Session: c46f0590-2707-466b-93c7-bb74316f385a
Copilot-Session: 97f1ddbf-b571-4a6c-8cb3-411b37e0d15a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants