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

p5.Vector reflect() unexpectedly modifies surface normal argument #7088

Closed
1 of 17 tasks
nbogie opened this issue Jun 8, 2024 · 3 comments · Fixed by #7103
Closed
1 of 17 tasks

p5.Vector reflect() unexpectedly modifies surface normal argument #7088

nbogie opened this issue Jun 8, 2024 · 3 comments · Fixed by #7103
Assignees

Comments

@nbogie
Copy link
Contributor

nbogie commented Jun 8, 2024

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
  • Internationalization
  • Friendly errors
  • Other (specify if possible)

p5.js version

1.9.4

Web browser and version

all

Operating system

all

Steps to reproduce this

Snippet:

function setup() {
	instanceDemo();
	staticDemo();
}

function instanceDemo() {
	const surfaceNormal = createVector(1, 0);
	const incidentVector = createVector(1, 1);
	
	incidentVector.reflect(surfaceNormal);

	//We'd hope surfaceNormal remains [1, 0, 0], but it'll be [2, 0, 0];
	console.log(surfaceNormal.toString());
}

function staticDemo() {
	const surfaceNormal = createVector(1, 0);
	const incidentVector = createVector(1, 1);
	
	p5.Vector.reflect(incidentVector, surfaceNormal);

	//We'd hope surfaceNormal remains [1, 0, 0], but it'll be [2, 0, 0];
	console.log(surfaceNormal.toString());
}

Expected behaviour:

I'd expect the surface normal argument to be unchanged by the calculation, in both instance and static cases.

Actual behaviour:

The surface normal argument is mutated during the calculation, in both instance and static cases.

Misc:

Here's a (simplification of a) real usage where the bug caught me out: https://openprocessing.org/sketch/2295422

@nbogie nbogie added the Bug label Jun 8, 2024
Copy link

welcome bot commented Jun 8, 2024

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!

@nbogie
Copy link
Contributor Author

nbogie commented Jun 8, 2024

I'd like to contribute a fix and regression tests, if it turns out that that's wanted!

Looks like the code has been this way since addition of the functions in v1.0.0

@limzykenneth
Copy link
Member

It seems like on this line the surface normal is multiplied by two using the instance method which modifies the vector where it probably should use a static method that does not modify the original instead.

@nbogie You can go ahead with a fix. Thanks!

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

Successfully merging a pull request may close this issue.

2 participants