Add friendly FES error for dimension mismatch on shared variable assi…#8823
Add friendly FES error for dimension mismatch on shared variable assi…#8823harshiltewari2004 wants to merge 1 commit into
Conversation
davepagurek
left a comment
There was a problem hiding this comment.
Nice work on this so far, and thanks for adding tests! I left some comments about situations where we might not run the dimension check, which seems to be around assigning to something that isnt a strands node, which could be like mySharedVec3 = 4. Once we address that, it's maybe worth adding a test case for that too.
| }, | ||
| set(val) { | ||
|
|
||
| if(val?.isStrandsNode&&val.dimension!==propertyType.dataType.dimension){ |
There was a problem hiding this comment.
There's a chance that the value is not yet a strands node, but in the if statement below on line 689 we create a strands node if it isn't one already. We could possibly move this check to happen after that, and then check for dimensions, to make sure we don't accidentally miss a scenario that should trigger a warning if e.g. the user assigned a plain number instead of a strands node?
|
|
||
| // For varying variables, we need both assignment generation AND a way to reference by identifier | ||
| if (this._originalIdentifier) { | ||
| if(value?.isStrandsNode && value.dimension!==this._originalDimension){ |
There was a problem hiding this comment.
Similar to before, in an if statement above here we create newVal if it wasn't originally a strands node. Maybe we should capture that and use it so that we don't accidentally skip this check?
|
|
||
| // For varying variables, create swizzle assignment | ||
| if (this._originalIdentifier) { | ||
| if(value?.isStrandsNode && value.dimension!==swizzlePattern.length){ |
There was a problem hiding this comment.
Looks like the same pattern applies here too.
|
Thanks @davepagurek for the review! I see the pattern — the check currently runs before the raw value gets coerced into a strands node, so plain assignments like mySharedVec3 = 4 skip it. I'll move the dimension check to after node creation at each of the sites you flagged. |
|
Hi @davepagurek ,gentle bump — just wanted to check if the scalar broadcast question above is settled before I start on the fix. Happy to proceed once I have a steer on that. |
|
I believe in call cases in strands, we don't distinguish between number literals and scalar strands nodes, so assigning a 1D node to a shared vec3 should still be valid. This is to be consistent with how GLSL handles float literals in operations with vectors -- when in doubt, let's default to what GLSL does. |
|
Got it — so the check should flag a genuine mismatch (e.g. vec2 → vec3) but let a scalar (dim 1) broadcast through, matching GLSL. I'll move the check to after node coercion at all four sites and add the scalar exception, plus a test for the mySharedVec3 = 4 case you mentioned. Will push shortly. |
|
Thanks for the review. Here's the approach I'm planning — digging into this surfaced a wrinkle worth confirming before I push. The literal "move the check after coercion and read the coerced node's dimension" doesn't work here, because primitiveConstructorNode doesn't derive a dimension from the value — it echoes back whatever typeInfo.dimension it's handed. In bridge() that's this.dimension (the target's), so a coerced node always carries the target dimension and the check could never fire on it. So instead the fix reads the value's true dimension directly, independent of coercion: The valueDim !== 1 guard preserves the scalar-broadcast case we agreed on in #8812 — a plain scalar assigned to a higher-dimension shared var is allowed, only genuine mismatches (e.g. vec2 → vec3) throw. This also fixes the original bug you flagged: the old value?.isStrandsNode && guard skipped raw-number assignments like mySharedVec3 = 4 entirely. One thing I want to check before editing tests: the existing reports a friendly error when assigning a scalar to a sharedVec3 test expects a throw on scalar → vec3, but that's the broadcast case we said is valid. Should that test be retargeted to a real mismatch (vec2 → vec3), or am I misreading the intent there? |
Resolves #8812
Changes
Adds a friendly error for the case where a value of the wrong dimension is assigned to a p5.strands shared variable. Previously, code like:
js let worldPosX = sharedVec3(); worldPosX = inputs.position.x; // scalar assigned to vec3 would emit invalid shader source (
vec3 = float) and bubble up as a cryptic GPU shader-compiler message such asERROR: 0:98: 'assign' : cannot convert from 'highp float' to 'highp 3-component vector of float', with no way for the user to map it back to the JS variable that caused it.After this PR, the same code throws at IR-construction time with:
[p5.strands dimension mismatch]: Cannot assign a value of dimension 1 to `worldPosX`, which expects dimension 3. This follows the four-call-site approach discussed in the plan comment on #8812 (factor out a shared helper, throw early via the existing
userErrorconvention).Implementation
A new
dimensionMismatchError(declaredDim, actualDim, varName)helper insrc/strands/strands_FES.jsis wired into the four sites that produce assignment IR nodes for strands-tracked variables:bridge()insrc/strands/strands_node.js— primary path forsharedX = valueafter the transpiler rewrites the assignment to.bridge(...).bridgeSwizzle()insrc/strands/strands_node.js— for swizzle writes likesharedX.xyz = value. Declared dimension comes fromswizzlePattern.length.swizzleTrap.setinsrc/strands/ir_builders.js— the existing partial dimension check was refactored to use the shared helper, preserving the broadcast (dim === 1) and exact-match cases.Object.definePropertysetter insrc/strands/strands_api.js— for hook-struct field writes likeworldInputs.position = newVec. Declared dimension comes frompropertyType.dataType.dimension.Broadcasting from a scalar (
dim === 1) is preserved as before, consistent with non-shared assignment behavior.Tests
Three tests added in both
test/unit/webgl/p5.Shader.jsandtest/unit/webgpu/p5.Shader.jsunder thep5.strandssuite:reports a friendly error when assigning a scalar to a sharedVec3 (bridge)reports a friendly error on dimension mismatch via swizzle write (bridgeSwizzle)does not error when shared variable assignment dimensions matchTest infrastructure note
Two small adjustments in
test/unit/webgl/p5.Shader.jsweren't part of the fix itself but were needed for the tests to run cleanly:vi.mockfactory forstrands_FESwas extended to includedimensionMismatchError. The existing mock only stubbeduserErrorandinternalError, so the new import broke module loading.afterEachmock-clearing hook in thep5.strands error messagessuite was changed tobeforeEach. The original only cleaned up after each test, so the suite's first test inheritedmockUserError.mock.calls[0]from any prior test in the file that had fired auserError. This was a latent fragility that the new tests happened to expose —beforeEachdefends against it generally.Screenshots of the change
N/A — error-message change only. Before/after messages included above.
PR Checklist
npm run lintpasses