Refactor Color Code

[JustZambetti]

Increasing Access

Making the code easier to read

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• Build Process
• Unit Testing
• Internalization
• Friendly Errors
• Other (specify if possible)

Feature enhancement details

I noticed a bunch of tiny problems in src/color/p5.Color.js:

• Criptic names that could be replaced:
  pInst and vals for example could be replaced with p5Instance and values respectively

  constructor(pInst, vals) {
    // Record color mode and maxes at time of construction.
    this._storeModeAndMaxes(pInst._colorMode, pInst._colorMaxes);

• Literals in setRed, setBlue, setGreen, setAlpha which could be replaced with constants
  For example, this._array[1] could become this._array[GreenLevel]

setGreen(new_green) {
    this._array[1] = new_green / this.maxes[constants.RGB][1];
    this._calculateLevels();
  }

• _calculateLevels() might be broken down using a simpler method _updateLevel(level). In many cases only that smaller method would be called (on setRed(), setBlue(),... for example)

_calculateLevels() {
   const array = this._array;
   // (loop backwards for performance)
   const levels = (this.levels = new Array(array.length));
   for (let i = array.length - 1; i >= 0; --i) {
     levels[i] = Math.round(array[i] * 255);
   }

   // Clear cached HSL/HSB values
   this.hsla = null;
   this.hsba = null;
 }

• _getMode(), _getMaxes() and the export default are never used, they can be removed.
• there are errors in the javadoc (non-matching field names)
• method _parseInputs() is too long (207 lines of code) and therfore hard to read
• namedColors enum could be moved in another file since it's very long

I would like to work on this issue

[welcome]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, please make sure to fill out the inputs in the issue forms. Thank you!

[Vishal2002]

Can I also work on this issue.

[lindapaiste]

Some of the JSDoc issues are due inconsistent variable names which is easy to fix. Others are 🤬🤯 because we often have multiple override signatures with conflicting variable names and types. JSDoc seems to only use the last signature. I haven’t figured out a way to fix this without messing up the documentation.

[lindapaiste]

There's always a tradeoff between performance and code-cleanliness when it comes to extracting things into functions, but I have some ideas where it maybe makes sense.

This part of the code appears 7 times:

if (!this.hsba) {
  this.hsba = color_conversion._rgbaToHSBA(this._array);
}

It could be extracted into a private function, like this._getHsba or this._ensureHsbaExists or something. Same for HSL.

Even better would be to store the actual array as this._hsba and use a getter to lazy-load the values. We could use this.hsba directly in code and we wouldn't ever need to check if it exists.

get hsba() {
  if (!this._hsba) {
    this._hsba = color_conversion._rgbaToHSBA(this._array);
  }
  return this._hsba;
}

There are so many places where we multiply a normalized channel value by it's corresponding value is this.maxes. eg. this.hsla[1] * this.maxes[constants.HSL][1]. There could be a utility function for this like _getChannelValue(mode, index).

Likewise, we could have a _setChannelValue(mode, index) which we call internally in setRed, setGreen, etc.

I could go on but I don't know if any of these justify the performance hit of additional function calls. Like there's a note in the code about something that was done specifically for performance:

const array = this._array;
// (loop backwards for performance)
const levels = (this.levels = new Array(array.length));
for (let i = array.length - 1; i >= 0; --i) {
  levels[i] = Math.round(array[i] * 255);
}

Where it could be written much cleaner as:

this.levels = this._array.map(value => Math.round(value * 255));

But it seems like it was not done that way on purpose.

[limzykenneth]

As part of 2.0 RFC at #6678, I would like to rewrite the color module entirely with potentially different backwards incompatible API. The old implementation may be kept around as a bridging addon library between 1.x and 2.0 only. If the desire is to still work on refactoring this codebase I don't mind but just want to note it may not be included in the next major version.