From fe65e4d41eaf7bbb088f0ccc52c02958547111b7 Mon Sep 17 00:00:00 2001 From: Akshat Date: Sun, 25 Oct 2026 22:33:07 +0530 Subject: [PATCH 1/2] fix(color): stop re-wrapping p5.Color for every sketch Resolves #9299 --- src/color/creating_reading.js | 26 +++-- src/color/p5.Color.js | 109 ++++++------------- test/unit/color/instances.js | 192 ++++++++++++++++++++++++++++++++++ 3 files changed, 241 insertions(+), 86 deletions(-) create mode 100644 test/unit/color/instances.js diff --git a/src/color/creating_reading.js b/src/color/creating_reading.js index 33aed4f46f..239fba091d 100644 --- a/src/color/creating_reading.js +++ b/src/color/creating_reading.js @@ -71,6 +71,10 @@ export const OKLCH = 'oklch'; */ export const RGBA = 'rgba'; +// red(), green() and blue() use the display-p3 ranges for display-p3 colors and +// the RGB ranges for every other color. +const rgbMode = color => (color.mode === RGBP3 ? RGBP3 : RGB); + function creatingReading(p5, fn) { fn.RGB = RGB; fn.RGBP3 = RGBP3; @@ -410,12 +414,16 @@ function creatingReading(p5, fn) { return args[0]; // Do nothing if argument is already a color object. } + // Colors reach their sketch through a holder that is cleared when the + // sketch is removed, so they don't keep a removed sketch alive. + this._colorOwner ??= { pInst: this }; + const arg = Array.isArray(args[0]) ? args[0] : args; return new Color( arg, this._renderer.states.colorMode, this._renderer.states.colorMaxes[this._renderer.states.colorMode], - { clamp: true } + { clamp: true, owner: this._colorOwner } ); }; @@ -568,8 +576,8 @@ function creatingReading(p5, fn) { */ fn.red = function (c) { // p5._validateParameters('red', arguments); - // Get current red max - return this.color(c)._getRed(); + const color = this.color(c); + return color._getRed(this._renderer.states.colorMaxes[rgbMode(color)][0]); }; /** @@ -721,8 +729,8 @@ function creatingReading(p5, fn) { */ fn.green = function (c) { // p5._validateParameters('green', arguments); - // Get current green max - return this.color(c)._getGreen(); + const color = this.color(c); + return color._getGreen(this._renderer.states.colorMaxes[rgbMode(color)][1]); }; /** @@ -874,8 +882,8 @@ function creatingReading(p5, fn) { */ fn.blue = function (c) { // p5._validateParameters('blue', arguments); - // Get current blue max - return this.color(c)._getBlue(); + const color = this.color(c); + return color._getBlue(this._renderer.states.colorMaxes[rgbMode(color)][2]); }; /** @@ -995,8 +1003,8 @@ function creatingReading(p5, fn) { */ fn.alpha = function (c) { // p5._validateParameters('alpha', arguments); - // Get current alpha max - return this.color(c)._getAlpha(); + const color = this.color(c); + return color._getAlpha(this._renderer.states.colorMaxes[color.mode][3]); }; /** diff --git a/src/color/p5.Color.js b/src/color/p5.Color.js index b8ce458e13..08f245bb52 100644 --- a/src/color/p5.Color.js +++ b/src/color/p5.Color.js @@ -88,7 +88,13 @@ class Color { Color.#grayscaleMap[mode] = definition.fromGray; } - constructor(vals, colorMode, colorMaxes, { clamp = false } = {}) { + // Holds the sketch (or graphics) that created this color so that the + // setters can read its colorMode() ranges. See the `color` addon below. + #owner; + + constructor(vals, colorMode, colorMaxes, { clamp = false, owner } = {}) { + this.#owner = owner; + // This changes with the color object this._cachedMode = colorMode || RGB; @@ -332,7 +338,13 @@ class Color { space: Color.colorMap[lerpMode] })(amt); - return new Color(obj, mode || this.mode); + return new Color(obj, mode || this.mode, undefined, { owner: this.#owner }); + } + + // The colorMode() ranges of the sketch that created this color, or undefined + // if there is none or it has been removed. + _getOwnerMaxes() { + return this.#owner?.pInst?._renderer?.states?.colorMaxes; } /** @@ -1097,82 +1109,25 @@ function color(p5, fn, lifecycles) { p5.Color.addColorMode(OKLAB, OKLab); p5.Color.addColorMode(OKLCH, OKLCHSpace); - lifecycles.presetup = function () { - const pInst = this; - - // Decorate set methods - const setMethods = ['Red', 'Green', 'Blue', 'Alpha']; - for (let i in setMethods) { - const method = setMethods[i]; - const setCopy = p5.Color.prototype['set' + method]; - p5.Color.prototype['set' + method] = function (newval, max) { - max = max || pInst?._renderer?.states?.colorMaxes?.[RGB][i]; - return setCopy.call(this, newval, max); - }; - } - - // Decorate get methods - function decorateGet(channel, modes) { - const getCopy = p5.Color.prototype['_get' + channel]; - p5.Color.prototype['_get' + channel] = function (max) { - if (Object.keys(modes).includes(this.mode)) { - max = - max || - pInst?._renderer?.states?.colorMaxes?.[this.mode][modes[this.mode]]; - } else { - const defaultMode = Object.keys(modes)[0]; - max = - max || - pInst?._renderer?.states?.colorMaxes?.[defaultMode][ - modes[defaultMode] - ]; - } - - return getCopy.call(this, max); - }; + // The setters take values in the range of the sketch's colorMode(), so they + // look up the ranges of the sketch that created the color. This is done once + // for the shared prototype, not once per sketch. + const setRangeFor = channel => setter => + function (value, max) { + const maxes = this._getOwnerMaxes()?.[RGB]; + return setter.call(this, value, max || maxes?.[channel]); + }; + p5.registerDecorator('p5.Color.prototype.setRed', setRangeFor(0)); + p5.registerDecorator('p5.Color.prototype.setGreen', setRangeFor(1)); + p5.registerDecorator('p5.Color.prototype.setBlue', setRangeFor(2)); + p5.registerDecorator('p5.Color.prototype.setAlpha', setRangeFor(3)); + + // Colors only reach their sketch through a holder, which is cleared here so + // that a removed sketch isn't kept in memory by its colors. + lifecycles.remove = function () { + if (this._colorOwner) { + this._colorOwner.pInst = null; } - - decorateGet('Red', { - [RGB]: 0, - [RGBP3]: 0 - }); - decorateGet('Green', { - [RGB]: 1, - [RGBP3]: 1 - }); - decorateGet('Blue', { - [RGB]: 2, - [RGBP3]: 2 - }); - decorateGet('Alpha', { - [RGB]: 3, - [RGBP3]: 3, - [HSB]: 3, - [HSL]: 3, - [HWB]: 3, - [LAB]: 3, - [LCH]: 3, - [OKLAB]: 3, - [OKLCH]: 3 - }); - - decorateGet('Hue', { - [HSL]: 0, - [HSB]: 0, - [HWB]: 0, - [LCH]: 2, - [OKLCH]: 2 - }); - decorateGet('Saturation', { - [HSL]: 1, - [HSB]: 1 - }); - decorateGet('Brightness', { - [HSB]: 2 - }); - decorateGet('Lightness', { - [HSL]: 2 - }); }; } diff --git a/test/unit/color/instances.js b/test/unit/color/instances.js new file mode 100644 index 0000000000..b84e98dab0 --- /dev/null +++ b/test/unit/color/instances.js @@ -0,0 +1,192 @@ +import p5 from '../../../src/app.js'; +import '../../js/chai_helpers'; + +suite('color/Instances', function () { + let sketches = []; + + // Creates a sketch and waits for its setup to run. + async function createSketch(configure) { + return new Promise(resolve => { + const sketch = new p5(p => { + p.setup = () => { + p.createCanvas(10, 10); + configure?.(p); + resolve(p); + }; + }); + sketches.push(sketch); + }); + } + + // Removes a sketch ahead of the cleanup after each test. + async function removeSketch(sketch) { + sketches = sketches.filter(s => s !== sketch); + await sketch.remove(); + } + + afterEach(async function () { + await Promise.all(sketches.map(sketch => sketch.remove())); + sketches = []; + }); + + suite('ranges of the sketch that made the color', function () { + test('red(), green(), blue() and alpha() use the sketch that calls them', async function () { + const unit = await createSketch(p => p.colorMode(p.RGB, 1)); + // Created later, so it used to decide the ranges of every sketch. + const standard = await createSketch(); + + const c = unit.color(0.5, 0.25, 0.75, 0.125); + assert.approximately(unit.red(c), 0.5, 1e-6); + assert.approximately(unit.green(c), 0.25, 1e-6); + assert.approximately(unit.blue(c), 0.75, 1e-6); + assert.approximately(unit.alpha(c), 0.125, 1e-6); + + const d = standard.color(255, 0, 51, 255); + assert.equal(standard.red(d), 255); + assert.equal(standard.green(d), 0); + assert.equal(standard.blue(d), 51); + assert.equal(standard.alpha(d), 255); + }); + + test('red(), green(), blue() and alpha() use their own ranges in a p5.Graphics', async function () { + const sketch = await createSketch(); + const g = sketch.createGraphics(10, 10); + g.colorMode(g.RGB, 1); + + const c = g.color(0.5, 0.25, 0.75, 0.125); + assert.approximately(g.red(c), 0.5, 1e-6); + assert.approximately(g.green(c), 0.25, 1e-6); + assert.approximately(g.blue(c), 0.75, 1e-6); + assert.approximately(g.alpha(c), 0.125, 1e-6); + + // The sketch is unaffected by the colorMode() of its graphics buffer. + assert.equal(sketch.red(sketch.color(255, 0, 0)), 255); + }); + + test('red(), green() and blue() keep the RGB ranges in other color modes', async function () { + const sketch = await createSketch(p => { + p.colorMode(p.RGB, 100); + p.colorMode(p.HSB, 360, 100, 100, 1); + }); + + const c = sketch.color(0, 100, 100); + assert.approximately(sketch.red(c), 100, 0.5); + assert.approximately(sketch.green(c), 0, 0.5); + assert.approximately(sketch.blue(c), 0, 0.5); + }); + + test('setRed(), setGreen(), setBlue() and setAlpha() use the sketch that made the color', async function () { + const unit = await createSketch(p => p.colorMode(p.RGB, 1)); + const standard = await createSketch(); + + // array() gives the channels from 0 to 1 whatever the colorMode() is. + const c = unit.color(0, 0, 0, 0); + c.setRed(0.5); + c.setGreen(0.25); + c.setBlue(0.75); + c.setAlpha(0.125); + assert.arrayApproximately(c.array(), [0.5, 0.25, 0.75, 0.125], 1e-6); + + const d = standard.color(0, 0, 0, 0); + d.setRed(255); + d.setGreen(51); + d.setBlue(102); + d.setAlpha(204); + assert.arrayApproximately(d.array(), [1, 0.2, 0.4, 0.8], 1e-6); + }); + + test('setters use the ranges of a p5.Graphics for its colors', async function () { + const sketch = await createSketch(); + const g = sketch.createGraphics(10, 10); + g.colorMode(g.RGB, 1); + + const c = g.color(0, 0, 0); + c.setRed(0.5); + assert.approximately(c.array()[0], 0.5, 1e-6); + }); + + test('colors from lerpColor() use the ranges of the sketch', async function () { + const unit = await createSketch(p => p.colorMode(p.RGB, 1)); + await createSketch(); + + const mixed = unit.lerpColor( + unit.color(0, 0, 0), + unit.color(1, 1, 1), + 0.5 + ); + mixed.setRed(0.25); + assert.approximately(mixed.array()[0], 0.25, 1e-6); + }); + + test('setters accept an explicit range', async function () { + const sketch = await createSketch(); + + const c = sketch.color(0, 0, 0); + c.setRed(50, 100); + assert.approximately(c.array()[0], 0.5, 1e-6); + }); + }); + + suite('sketches and the shared p5.Color prototype', function () { + const methods = [ + 'setRed', + 'setGreen', + 'setBlue', + 'setAlpha', + '_getRed', + '_getGreen', + '_getBlue', + '_getAlpha', + '_getHue', + '_getSaturation', + '_getBrightness', + '_getLightness' + ]; + + test('creating more sketches does not wrap its methods again', async function () { + await createSketch(); + const before = methods.map(method => p5.Color.prototype[method]); + + await createSketch(); + await createSketch(); + + methods.forEach((method, i) => { + assert.strictEqual( + p5.Color.prototype[method], + before[i], + `${method} was replaced` + ); + }); + }); + + test('colors do not keep a removed sketch alive', async function () { + const sketch = await createSketch(); + const c = sketch.color(255, 0, 0); + assert.strictEqual( + c._getOwnerMaxes(), + sketch._renderer.states.colorMaxes + ); + + await removeSketch(sketch); + assert.isUndefined(c._getOwnerMaxes()); + }); + + test('colors can still be converted to JSON', async function () { + const sketch = await createSketch(); + const c = sketch.color(255, 0, 0); + + assert.doesNotThrow(() => JSON.stringify(c)); + }); + + test('colors still work after their sketch is removed', async function () { + const sketch = await createSketch(p => p.colorMode(p.RGB, 1)); + const c = sketch.color(0.5, 0.25, 0.75); + await removeSketch(sketch); + + // Without a sketch the setters fall back to the default 0 to 1 range. + c.setRed(1); + assert.approximately(c._getRed(), 1, 1e-6); + assert.approximately(c._getGreen(), 0.25, 1e-6); + }); + }); +}); From 33fa03aafe0b18cbc990d0a6b56fdda228784e89 Mon Sep 17 00:00:00 2001 From: Akshat Date: Tue, 27 Oct 2026 18:49:40 +0530 Subject: [PATCH 2/2] fix(color): use the color mode alpha range in setAlpha --- src/color/p5.Color.js | 11 +++++++---- test/unit/color/instances.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/color/p5.Color.js b/src/color/p5.Color.js index 08f245bb52..2ad3bfe351 100644 --- a/src/color/p5.Color.js +++ b/src/color/p5.Color.js @@ -1110,12 +1110,15 @@ function color(p5, fn, lifecycles) { p5.Color.addColorMode(OKLCH, OKLCHSpace); // The setters take values in the range of the sketch's colorMode(), so they - // look up the ranges of the sketch that created the color. This is done once - // for the shared prototype, not once per sketch. + // look up the ranges of the sketch that created the color. Red, green and + // blue use the RGB ranges. Alpha uses the ranges of the color's own mode, like + // alpha() does. This is done once for the shared prototype, not once per + // sketch. const setRangeFor = channel => setter => function (value, max) { - const maxes = this._getOwnerMaxes()?.[RGB]; - return setter.call(this, value, max || maxes?.[channel]); + const mode = channel === 3 ? this.mode : RGB; + const range = this._getOwnerMaxes()?.[mode]?.[channel]; + return setter.call(this, value, max || range); }; p5.registerDecorator('p5.Color.prototype.setRed', setRangeFor(0)); p5.registerDecorator('p5.Color.prototype.setGreen', setRangeFor(1)); diff --git a/test/unit/color/instances.js b/test/unit/color/instances.js index b84e98dab0..13f36ef58f 100644 --- a/test/unit/color/instances.js +++ b/test/unit/color/instances.js @@ -95,6 +95,36 @@ suite('color/Instances', function () { assert.arrayApproximately(d.array(), [1, 0.2, 0.4, 0.8], 1e-6); }); + test('setAlpha() uses the alpha range of the color mode', async function () { + const sketch = await createSketch(p => p.colorMode(p.HSB)); + + const c = sketch.color(200, 50, 80); + c.setAlpha(0.5); + assert.arrayApproximately( + c.array(), + sketch.color(200, 50, 80, 0.5).array(), + 1e-6 + ); + + sketch.colorMode(sketch.HSL, 360, 100, 100, 255); + const d = sketch.color(200, 50, 80); + d.setAlpha(51); + assert.approximately(d.array()[3], 0.2, 1e-6); + }); + + test('setRed(), setGreen() and setBlue() use the RGB ranges in other color modes', async function () { + const sketch = await createSketch(p => { + p.colorMode(p.RGB, 100); + p.colorMode(p.HSB, 360, 100, 100, 1); + }); + + const c = sketch.color(0, 0, 0); + c.setRed(100); + c.setGreen(50); + assert.approximately(c.array()[0], 1, 1e-6); + assert.approximately(c.array()[1], 0.5, 1e-6); + }); + test('setters use the ranges of a p5.Graphics for its colors', async function () { const sketch = await createSketch(); const g = sketch.createGraphics(10, 10);