Add friendly FES error for dimension mismatch on shared variable assi…

[harshiltewari2004]

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 as ERROR: 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 userError convention).

Implementation

A new dimensionMismatchError(declaredDim, actualDim, varName) helper in src/strands/strands_FES.js is wired into the four sites that produce assignment IR nodes for strands-tracked variables:

1. bridge() in src/strands/strands_node.js — primary path for sharedX = value after the transpiler rewrites the assignment to .bridge(...).
2. bridgeSwizzle() in src/strands/strands_node.js — for swizzle writes like sharedX.xyz = value. Declared dimension comes from swizzlePattern.length.
3. swizzleTrap.set in src/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.
4. Object.defineProperty setter in src/strands/strands_api.js — for hook-struct field writes like worldInputs.position = newVec. Declared dimension comes from propertyType.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.js and test/unit/webgpu/p5.Shader.js under the p5.strands suite:

• 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 match

Test infrastructure note

Two small adjustments in test/unit/webgl/p5.Shader.js weren't part of the fix itself but were needed for the tests to run cleanly:

• The vi.mock factory for strands_FES was extended to include dimensionMismatchError. The existing mock only stubbed userError and internalError, so the new import broke module loading.
• The afterEach mock-clearing hook in the p5.strands error messages suite was changed to beforeEach. The original only cleaned up after each test, so the suite's first test inherited mockUserError.mock.calls[0] from any prior test in the file that had fired a userError. This was a latent fragility that the new tests happened to expose — beforeEach defends against it generally.

Screenshots of the change

N/A — error-message change only. Before/after messages included above.

PR Checklist

• npm run lint passes
• Inline reference is included / updated (N/A — no public API added)
• Unit tests are included / updated

[davepagurek]

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.

[davepagurek]

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?

[davepagurek]

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?

[davepagurek]

Looks like the same pattern applies here too.

[harshiltewari2004]

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.
One thing I want to confirm before I do: we established on #8812 that scalar broadcasting is allowed (mySharedVec3 = 4 broadcasting 4 to all components, consistent with myVec = 5 for non-shared vectors). So when the check catches a plain number, should it (a) treat a dimension-1 scalar as valid broadcast and only flag genuine mismatches like mySharedVec3 = someVec2, or (b) warn on any non-matching dimension including scalars? Want to make sure the post-coercion check preserves the broadcasting behavior we agreed on.

[harshiltewari2004]

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.

[davepagurek]

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.

[harshiltewari2004]

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.

[harshiltewari2004]

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:
const valueDim = value?.isStrandsNode ? value.dimension : (Array.isArray(value) ? value.length : 1); if (valueDim !== this._originalDimension && valueDim !== 1) { dimensionMismatchError(this._originalDimension, valueDim, this._originalIdentifier); }

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.
I'll apply the same value-dimension pattern at the other three sites, each using its own declared dimension.

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?

[harshiltewari2004]

Hi @davepagurek gentle bump on this — no rush, just flagging it's still open whenever you have a chance. The main thing I'd value your read on is the existing scalar → sharedVec3 test: since we landed on scalar broadcast being valid, should that test be retargeted to a real mismatch (vec2 → vec3), or am I reading the intent wrong? Happy to proceed once that's clear.

[davepagurek]

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?

I think in this case it makes sense to remove that particular assertion since scalars can "turn into" vectors safely, and add a test preventing vec2s from being used as vec3s like you said. thanks!

[harshiltewari2004]

Got it, will do — thanks

[harshiltewari2004]

Pushed the fix. Summary of what changed:

Dimension check now reads the value's real dimension before coercion. The earlier version only fired when value?.isStrandsNode was true, so plain-number assignments (mySharedVec3 = 4) slipped through. Moving the check after coercion doesn't help either, because primitiveConstructorNode echoes back whatever dimension it's handed rather than deriving it from the value — so a coerced node's .dimension isn't the value's true dimension. Instead I compute it independently up front:

const valDim = value?.isStrandsNode ? value.dimension : (Array.isArray(value) ? value.length : 1);
if (valDim !== declaredDim && valDim !== 1) { /* error */ }

The !== 1 keeps scalar broadcast valid, per your confirmation. Applied at three sites: bridge() and bridgeSwizzle() in strands_node.js, and the shared-var setter in strands_api.js.

Left swizzleTrap.set in ir_builders.js untouched — it already branches correctly on RHS shape (broadcasts dim-1, maps matching dims, errors otherwise), so it was effectively the reference for the other three. Flagging it so the untouched swizzle-trap path doesn't look like an oversight.

Tests (both webgl and webgpu):

• Added the vec2→vec3 mismatch test you asked for.
• For the old scalar→sharedVec3 throw test: rather than deleting it, I converted it to assert not.toThrow(), since scalar broadcast is now valid and it seemed worth documenting + guarding against a regression. Happy to just remove it instead if you'd prefer — your call.
• Kept the existing bridgeSwizzle mismatch and matching-dims tests as-is.

All four pass on both renderers; full suite is green (a couple of visual screenshot cases timed out under load but pass in isolation, unrelated to this change).

[p5-bot]

Continuous Release

CDN link

• https://raw.esm.sh/pr/p5@2749b03/lib/p5.min.js

Published Packages

• https://pkg.pr.new/p5@2749b03

Commit hash: 19462ad

Previous deployments

---

This is an automated message.

[davepagurek]

Thanks for your work on this, the logic looks good! Just some minor notes.

[davepagurek]

Definitely minor, but it might be a bit easier to read to space this out a bit, like:

const valDim = val?.isStrandsNode
  ? val.dimension
  : (Array.isArray(val) ? val.length : 1 );

[harshiltewari2004]

Done, spaced it out across all three sites (bridge, bridgeSwizzle, and the setter).

[davepagurek]

Was this breaking something?

[harshiltewari2004]

Yeah — this fixes a pre-existing isolation issue that my new tests exposed. mockUserError is module-level, so its .mock.calls accumulate across the whole file. The p5.strands error messages suite reads mock.calls[0], and with afterEach clearing it was relying on nothing upstream in the file having fired userError before it ran. My new bridge/bridgeSwizzle throw-tests do fire it (via dimensionMismatchError), so by the time that suite started, mock.calls[0] was a stale dimension-mismatch call and its first test failed. Switching to beforeEach guarantees a clean mock at the start of each test in the suite, which fixes it and guards against the same thing happening from any future test added above it.

[davepagurek]

Do you mind fixing the indentation in this file?

[harshiltewari2004]

Fixed, re-indented the new tests to match the suite.