processing / processing/p5.js

[p5.js 2.0+ Bug Report]: textureWrap() silently drops the y wrap mode when passed the object returned by its own getter

Open
#9,164 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Area:WebGL p5.js 2.0+
Dominant language
JavaScript
Stars
24k
Forks
3.8k
Avg merge
3d 16h
Merged PRs (30d)
25

Description

Most appropriate sub-area of p5.js?
  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • WebGPU
  • p5.strands
  • Build process
  • Unit testing
  • Internationalization
  • Friendly errors
  • Other (specify if possible)
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.textureWrap has a branch that accepts the object its own getter returns, so textureWrap(textureWrap()) is meant to round-trip. It doesn't — the y mode comes back undefined, and the texture silently falls back to CLAMP.

function setup() {
  createCanvas(100, 100, WEBGL);

  textureWrap(REPEAT, MIRROR);
  console.log(textureWrap());   // { x: 'repeat', y: 'mirror' }  — correct

  textureWrap(textureWrap());   // round-trip through the object branch
  console.log(textureWrap());   // { x: 'repeat', y: undefined }  — y is gone
}

Same result for any hand-written object: textureWrap({ x: REPEAT, y: MIRROR }).

Why

src/webgl/material.js:3457-3461:

// accept what is returned from the getter
if (wrapX.hasOwnProperty('x') && wrapX.hasOwnProperty('y')) {
  wrapX = wrapX.x;
  wrapY = wrapX.y;
}

The first assignment replaces wrapX with a string, so the second line reads .y off that string rather than off the original object, and gets undefined.

Why it's silent

undefined is stored via setValue('textureWrapY', undefined) and travels to setWebGLTextureParams in src/webgl/utils.js, where the wrap branching is REPEAT → … else MIRROR → … else CLAMP_TO_EDGE. An unrecognised value lands in the final else and is treated as CLAMP.

Worth noting the asymmetry: REPEAT and MIRROR each console.warn when they have to downgrade to CLAMP on a non-power-of-two texture, so the existing code does consider a silent downgrade worth reporting. undefined produces no message at all.

A second, smaller defect in the same function

The prose documentation at src/webgl/material.js:3287-3288 says the getter returns:

{ wrapX: CLAMP, wrapY: REPEAT }

The returned keys are actually x and y. The @return annotation directly below (line 3447) already documents {x, y} correctly, and test/unit/core/properties.js:31 uses the {x, y} shape — so the prose is the part that's wrong, not the code. Anyone writing textureWrap().wrapX from the docs gets undefined.

Suggested fix

Read both keys off the original object before either is reassigned. Destructuring evaluates the right-hand side first, so nothing gets clobbered:

if (wrapX && typeof wrapX === 'object' && 'x' in wrapX && 'y' in wrapX) {
  ({ x: wrapX, y: wrapY } = wrapX);
}

This also removes the current TypeError on textureWrap(null), since null.hasOwnProperty throws before any check runs.

Plus a one-line docs correction for the key names.

Test coverage

test/unit/webgl/p5.Texture.js has three textureWrap() tests, all on the plain two-argument setter path. Neither the getter nor the object branch is covered. I'd add:

  • the getter returns {x, y} with the current modes
  • textureWrap(textureWrap()) preserves both modes
  • textureWrap({x: REPEAT, y: MIRROR}) sets them independently

Open questions before I pick an approach

  1. Docs or code for the key mismatch? Correcting the prose to {x, y} is the non-breaking option and matches the annotation and existing tests, so that's my assumption — but if wrapX/wrapY is the preferred public shape, that's a different (breaking) change and I'd rather not guess.
  2. Should an unrecognised wrap value warn? Right now anything that isn't REPEAT or MIRROR quietly becomes CLAMP in setWebGLTextureParams. 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.
  3. Is the null guard in scope here, or better as its own thing?

Happy to take this if the direction looks right.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/webgl/material.js around the textureWrap object branch and its getter documentation, then inspect setWebGLTextureParams in src/webgl/utils.js. Run the existing textureWrap tests in test/unit/webgl/p5.Texture.js and add coverage for getter output, round-tripping, and object input. Done means both wrap modes are preserved and the documented getter keys match the returned object.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
computer-graphics
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.