Repository navigation
[p5.js 2.0+ Bug Report]: textureWrap() silently drops the y wrap mode when passed the object returned by its own getter #9164
Description
Activity
@harshiltewari2004 Can I work on this?
I’ve prepared a focused fix for the
textureWrap()object setter.Before: After
textureWrap(REPEAT, MIRROR), passing the getter result back withtextureWrap(textureWrap())sets the Y mode toundefined. The function replaceswrapXwith itsxvalue before readingwrapX.y.After: Read both values from the object first, then update the modes. The same round trip preserves
{ x: REPEAT, y: MIRROR }.if (Object.hasOwn(wrapX, 'x') && Object.hasOwn(wrapX, 'y')) { const { x, y } = wrapX; wrapX = x; wrapY = y; }
Object.hasOwnretains the existing own-property check and supports objects without a prototype. This fix leaves invalid-mode warning behavior unchanged.Files to change:
src/webgl/material.js: fix the object setter and correct the getter’s documentation example to{ x, y }.test/unit/webgl/p5.Texture.js: add regression tests.
Tests: Check the getter’s
xandyvalues; verify that passing its result back preserves both modes; verify distinct modes in a supplied object; check an object without a prototype; and confirm the Y mode reaches WebGL after the round trip.The isolated reproduction passes with this fix. I still need to run the full browser test suite when its dependencies are available. Does this approach and scope look right?
harshiltewari2004 commented
on Sep 15, 2026 ContributorAuthorMore actionsThanks both for the interest, and @mayanksingh-27 for writing that up — your snippet matches what I had in mind, reading both values off the object before either variable is reassigned.
I filed this intending to implement it myself, which is why the issue ends with an offer to take it. I haven't opened a PR yet because the first open question changes what the diff touches: if the prose docs get corrected to
{x, y}it's a small fix plus a doc line, but ifwrapX/wrapYis the preferred public shape then it's a breaking change to the getter and a different piece of work. I didn't want to code into an unapproved design.One note on the snippet:
Object.hasOwn(null, 'x')throws aTypeErrorjust like the currentnull.hasOwnProperty, sotextureWrap(null)is still unhandled either way — that's open question 3 above.@ksen0 could you please weigh in on the key-naming question, and assign this to whoever you think is best placed to take it? Happy to do the work if it comes my way, and equally happy to review if it goes to someone else.
harshiltewari2004 commented
on Sep 27, 2026 ContributorAuthorMore actions@davepagurek could I get your call on this one? Rather than leave it as an open question, here's the narrow scope I'd propose:
- Fix the object branch in
fn.bezierOrder's neighbourfn.textureWrap(src/webgl/material.js) so both values are read off the object before either variable is reassigned. Behaviour fortextureWrap(REPEAT, MIRROR)is unchanged. - Correct the prose docs above the getter to say
{x, y}rather than{wrapX, wrapY}. The@returnannotation andtest/unit/core/properties.jsalready use{x, y}, so this is the non-breaking direction. - Leave two things out of scope:
textureWrap(null)still throws aTypeError(true of the current code and of every fix suggested so far), and unrecognised wrap values still fall through toCLAMPwithout a warning. Both seem like separate calls. - Add regression tests in
test/unit/webgl/p5.Texture.jscovering the getter shape and the round trip, neither of which is currently tested.
If that scope looks right, could I be assigned?
- Fix the object branch in
Metadata
Metadata
Assignees
Labels
Type
Projects
- StatusShow more project fieldsNo status
Most appropriate sub-area of p5.js?
p5.js version
2.3.2
Web browser and version
Chrome 152.0.7977.83 (Official Build) (arm64)
Operating system
macOS 26.6.2 (Build 25G83), MacBook Air M1
Steps to reproduce this
What happens
fn.textureWraphas a branch that accepts the object its own getter returns, sotextureWrap(textureWrap())is meant to round-trip. It doesn't — the y mode comes backundefined, and the texture silently falls back toCLAMP.Same result for any hand-written object:
textureWrap({ x: REPEAT, y: MIRROR }).Why
src/webgl/material.js:3457-3461:The first assignment replaces
wrapXwith a string, so the second line reads.yoff that string rather than off the original object, and getsundefined.Why it's silent
undefinedis stored viasetValue('textureWrapY', undefined)and travels tosetWebGLTextureParamsinsrc/webgl/utils.js, where the wrap branching isREPEAT → … else MIRROR → … else CLAMP_TO_EDGE. An unrecognised value lands in the finalelseand is treated asCLAMP.Worth noting the asymmetry:
REPEATandMIRROReachconsole.warnwhen they have to downgrade toCLAMPon a non-power-of-two texture, so the existing code does consider a silent downgrade worth reporting.undefinedproduces no message at all.A second, smaller defect in the same function
The prose documentation at
src/webgl/material.js:3287-3288says the getter returns:The returned keys are actually
xandy. The@returnannotation directly below (line 3447) already documents{x, y}correctly, andtest/unit/core/properties.js:31uses the{x, y}shape — so the prose is the part that's wrong, not the code. Anyone writingtextureWrap().wrapXfrom the docs getsundefined.Suggested fix
Read both keys off the original object before either is reassigned. Destructuring evaluates the right-hand side first, so nothing gets clobbered:
This also removes the current
TypeErrorontextureWrap(null), sincenull.hasOwnPropertythrows before any check runs.Plus a one-line docs correction for the key names.
Test coverage
test/unit/webgl/p5.Texture.jshas threetextureWrap()tests, all on the plain two-argument setter path. Neither the getter nor the object branch is covered. I'd add:{x, y}with the current modestextureWrap(textureWrap())preserves both modestextureWrap({x: REPEAT, y: MIRROR})sets them independentlyOpen questions before I pick an approach
{x, y}is the non-breaking option and matches the annotation and existing tests, so that's my assumption — but ifwrapX/wrapYis the preferred public shape, that's a different (breaking) change and I'd rather not guess.REPEATorMIRRORquietly becomesCLAMPinsetWebGLTextureParams. Surfacing that through the FES feels consistent with the existing power-of-two warnings, but it's wider than this bug and would affect every caller, so I'd keep it out unless you'd like it included.nullguard in scope here, or better as its own thing?Happy to take this if the direction looks right.