Honor instance counts without per-instance vertex attributes - #1888
Open
bkaradzic-microsoft wants to merge 5 commits into
Open
bkaradzic-microsoft wants to merge 5 commits into
bkaradzic-microsoft wants to merge 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
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
This PR fixes attribute-less instancing by explicitly passing the requested instance count to bgfx when no per-instance vertex buffer is present, and adds a native GPU regression test.
Changes:
- Calls
setInstanceCountfor supported backends usingBGFX_CAPS_VERTEX_ID. - Adds a JavaScript test covering indexed and non-indexed meshes with 0, 1, and 3 instances.
- Adds a native-rendering availability flag and includes the new test in the JavaScript suite.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
Plugins/NativeEngine/Source/VertexArray.cpp |
Supplies explicit instance counts for attribute-less draws. |
Apps/UnitTests/Source/Tests.JavaScript.cpp |
Exposes whether native rendering tests should run. |
Apps/UnitTests/JavaScript/src/tests.nativeEngine.attributeLessInstancing.ts |
Adds the GPU regression coverage. |
Apps/UnitTests/JavaScript/src/tests.javaScript.all.ts |
Registers the new test suite. |
Apps/UnitTests/JavaScript/dist/tests.javaScript.all.js |
Includes the compiled test bundle changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
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 5 changed files in this pull request and generated 4 comments.
bkaradzic-microsoft
requested review from
CedricGuillemet,
bghgary and
ryantrem
and
a balanced review from Copilot
September 18, 2026 00:56
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Vulkan location assignment incorrectly matches interface blocks by instance name, potentially breaking valid cross-stage linkage.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 8/9 changed files
- Comments generated: 1
- Review effort level: Balanced
Tell bgfx the explicit count when a draw derives its instance data from gl_InstanceID instead of an instance buffer. Preserve the existing buffered-instance path and gate the call on BGFX_CAPS_VERTEX_ID. Add indexed/non-indexed GPU controls for ordinary draws and forced counts of one and three, using distinct colors for all three instances. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Require BGFX_CAPS_VERTEX_ID at runtime in addition to the existing no-op Metal and disabled-render-test configuration guards. Wait for the render target's own pass readiness before its single draw, with an explicit timeout and no warmup renders. Report all three pixel centers together when a draw fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Mark the installation compile fixture explicitly so its shared JavaScript test source does not include private bgfx headers. Internal UnitTests keep the runtime capability check and GPU regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Assign linked varying locations by name while preserving explicit locations and reserving complete array/matrix ranges. Assign fragment output locations and translate GLSL vertex/instance IDs to Vulkan index built-ins. The Linux lavapipe regression failed even for an ordinary count-zero draw because the previous SPIR-V lacked varying locations and used OpenGL-only ID decorations. Keep ordinary GLSL parsing and stock dependency pins. Add direct emitted-SPIR-V regressions for reversed declaration order, one-sided explicit locations, matrix arrays, vertex-only outputs, fragment outputs, and vertex/instance index built-ins. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Use the linked block type name as the location key, matching glslang's getAccessName semantics while retaining ordinary varying names. Vertex and fragment stages can use different block instance names. Add a regression with different instance names, a matrix member, an ordinary varying, and an explicit location on only the fragment block. Exercise it with the existing BABYLON_NATIVE_DISABLE_WEBMIN option; the default minified parser does not support shader IO blocks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
bkaradzic-microsoft
force-pushed
the
pr/native-attribute-less-instancing
branch
from
September 18, 2026 14:30
9c024df to
d469df6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setInstanceCountwhen a draw requests instances but provides no per-instance vertex buffer, on backends supportingBGFX_CAPS_VERTEX_ID.gl_InstanceID.Mesh.forcedInstanceCountAPI: indexed and non-indexed geometry, ordinary draws, one instance, and three independently colored instances.BGFX_CAPS_VERTEX_ID, respect no-op Metal / disabled-render-test configurations, and wait for RTT-pass readiness without warmup renders. Vulkan remains enabled.Independence
Based on
masteratbb01e283, with published Babylon.js 9.21.2 and stock bgfx. No protocol, dependency-pin, fixture, tolerance, compute, or storage-buffer changes.This fixes the underlying draw behavior; it does not import or claim the separate JavaScript shader changes needed by some clustered-lighting scenes.
CI follow-up
d32b2f0efixes the installation failures: shared JavaScript test code must also compile against the public SDK, which intentionally does not export private bgfx headers. Only the installed-consumer fixture omits the private capability probe. Actual Windows/D3D11 and Linux/OpenGL installed-SDK consumers both build without source-tree include paths or installed bgfx headers.7f7b0083fixes the reproduced Linux Vulkan failure. RTT readiness alone did not fix the count-zero control: the emitted SPIR-V lacked inter-stageLocationdecorations and used OpenGL-onlyVertexId/InstanceIdbuilt-ins. Preserve ordinary GLSL parsing, assign matching varying locations by name, reserve explicit locations and complete matrix/array ranges, assign fragment output locations, and emit VulkanVertexIndex/InstanceIndex. The pinnedGLSLANG_WEBbuild omits glslang's IO mapper, so this uses a Vulkan-local AST pass without changing dependencies or other backends.Validation
spirv-val --target-env vulkan1.0.setInstanceCountwith the repaired compiler still fails count 3; restoring it passes.CompileComprehensiveGLSLremains unchanged (upstream Make CompileComprehensiveGLSL complete on Linux Vulkan #1877).Interface-block review follow-up
9c024dfekeys linked interface blocks by block type rather than stage-local instance name. A regression with different instance names, a matrix member, and implicit / one-sided explicit block locations fails before and passes after in the existingBABYLON_NATIVE_DISABLE_WEBMIN=ONconfiguration. The default parser rejects shader IO blocks, so this new syntax-specific case explicitly skips there; all existing Vulkan shader and GPU controls remain enabled and pass in both configurations.