Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

nbogie
Copy link
Contributor

@nbogie nbogie commented Jun 23, 2024

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

  • npm run lint passes
  • [Inline documentation] is included / updated - n/a (bugfix)
  • [Unit tests] are included / updated

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 limzykenneth merged commit eb61f7a into processing:main Jun 24, 2024
2 checks passed
@limzykenneth
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

p5.Vector reflect() unexpectedly modifies surface normal argument
2 participants