Store StandardMaterial diffuse in a material-owned uniform buffer - #9381
mvaligursky wants to merge 1 commit into
Conversation
The first typed material property. StandardMaterial declares diffuse through a MaterialProperty descriptor (public name, uniform name, type, in-place converter) and gets an explicit accessor; the dynamic property machinery keeps only its reset and copy metadata. A material with typed properties owns a persistent uniform buffer and a single-buffer bind group, created on the first prepareForRender and uploaded only when its data version changed. The uniform buffer format and bind group format are shared per device through a layout cache keyed by the sorted set of properties, so materials with the same properties share one layout in any declaration order. The forward and shadow loops bind the material bind group at the material bind group index on each material switch, or the empty group for materials without one. material_diffuse is no longer published as a parameter. Setters mark a property modified and getters snapshot the exposed value so an in-place mutation is detected. update() compares, converts straight into the buffer storage and bumps the data version. The renderer applies nothing: a change made without a subsequent update() is not applied, and the debug build warns once per material per update cycle, naming the material, its id, the changed properties and the nodes rendering it, plus the creation and last-change stacks when the new TRACEID_MATERIAL_UPDATE trace channel is enabled. The debug code lives in material-debug.js and is called from Debug.call only, so release builds carry neither it nor the per-material debug state. Shader processing receives the material uniform format at BINDGROUP_MATERIAL: the WGSL processor references the block that owns a uniform instead of assuming the view block, and the WebGL2 processor emits a block per supplied format. No chunk changes. MeshInstance#setParameter warns in the debug build when overriding a uniform stored in the material uniform buffer, which is a follow-up. UniformBuffer#upload uploads a persistent buffer written directly. The type fixup only appends imports the declaration file does not already contain. The material test example cycles its diffuse colors every frame as a regression check.
Public API reportThis PR changes the public API surface (+3 / −1), per the docs' rules (@ignore / @Private / undocumented are excluded). Show API diff-StandardMaterial.diffuse: Color
+StandardMaterial.get diffuse(): Color
+StandardMaterial.set diffuse(value: Color)
+const TRACEID_MATERIAL_UPDATE: "MaterialUpdate"Informational only — this never fails the build. |
Build size reportThis PR changes the size of the minified bundles.
|
mvaligursky
left a comment
mvaligursky
left a comment
There was a problem hiding this comment.
Automated PR review by Codex (GPT-5) at head 90b48ae80068ab28514aed74f9834d97e746704e.
I reviewed API/backward compatibility, property update and clone/reset semantics, material switching in forward and shadow passes, WebGL2/WebGPU shader processing and bind-group layout, context/device lifecycle, release stripping, performance/resource ownership, and tests. I found one blocking compatibility regression, detailed inline. In particular, this is exercised by existing Engine examples, so it should be resolved in this PR rather than deferred to a later follow-up.
Verification: git diff --check; focused material/shader tests (37 passing); full unit suite (2910 passing, 2 pending); lint; release UMD build; declaration build and type tests. CI is green. I did not claim pixel equivalence because no controlled raw cross-build capture matrix was available.
| Debug.removed('MeshInstance#setParameter: the "passFlags" argument has been removed and is ignored.'); | ||
| } | ||
| if (this._material?.propertyDescriptors?.some(property => property.uniformName === name)) { | ||
| Debug.warnOnce(`MeshInstance#setParameter: '${name}' is stored in the material uniform buffer and cannot be overridden per mesh instance yet, the value is ignored. Set the material property instead.`, this); |
There was a problem hiding this comment.
[P1] Preserve existing diffuse parameter overrides in this migration. This makes the documented MeshInstance#setParameter precedence contract a production no-op for material_diffuse: shader processing removes that uniform from the mesh path and reads ub_material.material_diffuse, while this warning is stripped from release builds. This is already used by the repository's ar-hit-test example to give instances sharing one StandardMaterial distinct colors. The analogous public Material#setParameter('material_diffuse', ...) path also silently stops working (for example, world-to-screen) and receives no warning at all. Please land per-instance/material parameter compatibility in the same change, or leave diffuse on the existing uniform path until the follow-up is ready; merging the acknowledged gap independently breaks current users and checked-in examples.
1 participant
Moves the first StandardMaterial property,
diffuse, from the per-draw parameter path into a uniform buffer owned by the material and bound at the material bind group. Second step of the material uniform-buffer refactor, after #9370 reserved the bind group.Changes:
MaterialPropertydescriptors (material-property.js) describe a typed property: public name, uniform name, type and an in-place converter.StandardMaterialdeclaresdiffuseas the first one and gets an explicit accessor; the dynamic property machinery keeps only its reset and copy metadata.UniformBufferand a single-bufferBindGroup, created on the firstprepareForRenderand uploaded only when its data version changed. The formats are shared per device through a layout cache keyed by the sorted set of properties (material-uniform-buffer-layout.js), so materials with the same properties share one layout in any declaration order.update()compares, converts straight into the buffer storage and bumps the data version. The renderer applies nothing: a change made without a subsequentupdate()is not applied, and the debug build warns once per material per update cycle, naming the material, its id, the changed properties and the nodes rendering it. With the newTRACEID_MATERIAL_UPDATEtrace channel enabled, the warning also carries the stacks of where the material was created and last changed. The debug code lives inmaterial-debug.js, is called fromDebug.callonly, and the release bundle contains neither it nor the per-material debug state.material_diffuseis no longer published as a parameter, so it leaves the per-switch scope sweep.StandardMaterialsupplies the material uniform format atBINDGROUP_MATERIAL; the WGSL processor references the block that owns a uniform (ShaderProcessorOptions#getUniformBindGroup) instead of assuming the view block, and the WebGL2 processor emits a block per supplied format. No chunk changes.MeshInstance#setParameterwarns in the debug build when overriding a uniform stored in the material uniform buffer; per-instance overrides of those are a follow-up before the next release.UniformBuffer#upload()uploads the storage of a persistent buffer that was written directly.Coloritself.API Changes:
StandardMaterial#diffuseis an explicit accessor with unchanged semantics. As before, a change made after the material's first frame reaches the GPU only whenmaterial.update()is called; the debug build now reports such unapplied changes instead of ignoring them silently.TRACEID_MATERIAL_UPDATEtrace channel (Debug category).StandardMaterial#getUniform('diffuse')no longer exists (undocumented method, used internally for texture transforms).Examples:
test/material-testcycles the diffuse colors of its three pills through the hue wheel every frame, two by in-place mutation and one by assignment, as a per-frame regression check of both write paths.Performance:
update()and one whole-buffer upload per frame, and a material switch costs onesetBindGroup. No allocations after a property's first exposure.