processing / p5.js

p5.js is a client-side JS platform that empowers artists, designers, students, and anyone to learn to code and express themselves creatively on the web. It is based on the core principles of Processing. http://twitter.com/p5xjs —
http://p5js.org/
GNU Lesser General Public License v2.1
21.11k stars 3.22k forks source link

Fix vector reflect() mutating surface normal arg (#7088) #7103

Closed nbogie closed 6 days ago

nbogie commented 1 week ago

Resolves #7088

Changes:

Added unit tests to check the reflect functions (static and instance) don't alter the given surfaceNormal vector.

Changed the common p5.Vector reflect function so that the given surfaceNormal vector is not mutated unexpectedly.

Did so by making a local copy of the input vector:

reflect(surfaceNormal) {
    const surfaceNormalCopy = p5.Vector.normalize(surfaceNormal);
    return this.sub(surfaceNormalCopy.mult(2 * this.dot(surfaceNormalCopy)));
}

Screenshots of the change: n/a

PR Checklist

Note: eslint passes but emits one warning.

p5.js/src/core/reference.js:0:0: File ignored because of a matching ignore pattern. Use "--no-ignore" to override. [Warning]

misc implementation notes

Also considered this alternative implementation using the static methods to avoid a local copy of the vector, but I think it turns out harder to read, and requires the normalisation calculation happen twice.

reflect(surfaceNormal) {
    return this.sub(
      p5.Vector.mult(
        p5.Vector.normalize(surfaceNormal),
        2 * this.dot(p5.Vector.normalize(surfaceNormal))
      )
    );
  }
limzykenneth commented 6 days ago

Thanks @nbogie. I think having a local copy of the vector is the better approach as it is also more memory efficient.