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

Changes to Py5Vector set_mag() and normalize() #261

Merged
merged 3 commits into from
Mar 13, 2023
Merged

Changes to Py5Vector set_mag() and normalize() #261

merged 3 commits into from
Mar 13, 2023

Conversation

AsadNizami
Copy link
Contributor

Fix for #243.
set_mag() works when the magnitude is negative. However for cases like (0, 0, 1) it returns (-0, -0, -1). Is this okay, or should I change it to (0, 0, -1)?

@hx2A
Copy link
Collaborator

hx2A commented Mar 12, 2023

I think it is OK. @villares , what do you think?

In cases like this, we often compare py5 with Processing to get an idea of what is appropriate or what users would expect. Processing's API is a useful guide.

For this Processing code:

PVector v1 = new PVector(0, 0, 5);
PVector v2 = new PVector(0, 0, 0);

println(v1.copy().setMag(-1));
println(v1.copy().div(-1));
println(v2.copy().normalize());
[ -0.0, -0.0, -1.0 ]
[ -0.0, -0.0, -5.0 ]
[ 0.0, 0.0, 0.0 ]

If Processing is OK with negative zero, py5 should be also.

This PR would do the exact same thing as Processing except normalizing a zero vector would issue a warning 'Using normalize on a vector of zero magnitude has no effect'. I would maybe change the wording of that to "zero vector" instead of "vector of zero magnitude", but other than that, I think this PR is great.

@villares
Copy link
Collaborator

Wonderful improvement! Thank you @AsadNizami!

@AsadNizami AsadNizami marked this pull request as draft March 12, 2023 16:00
@AsadNizami
Copy link
Contributor Author

Pls review!

@AsadNizami AsadNizami marked this pull request as ready for review March 12, 2023 16:04
@hx2A hx2A merged commit 6397c80 into py5coding:main Mar 13, 2023
@hx2A
Copy link
Collaborator

hx2A commented Mar 13, 2023

Thank you, @AsadNizami !

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.

3 participants