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

(breaking) vec3: Rename angle_to -> angle_between #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

idbrii
Copy link
Contributor

@idbrii idbrii commented May 27, 2022

In vec2, angle_to is signed and angle_between is unsigned. We only have
the unsigned version in vec3, so use the corresponding name.

Remove the vec3 implementation from vec2 and use vec2's style
implementation. It doesn't allocate new vectors and works even if you
somehow call vec3.angle_between(vec2(), vec2()).

@xiejiangzhi
Copy link
Contributor

Nice.

vec3.angle_to has a bug for same vec3. v3.angle_to(v3) will return nan

use acos(a:dot(b) / (a:len() * b:len())) will fix it, so, could you add a testing case for this?

assert.is.equal(angle_between(a, a), '0.00')

@xiejiangzhi
Copy link
Contributor

acos(a:dot(b) / (a:len() * b:len())) also has this bug when a:len() * b:len() is zero or lua5.4, we should check a == b.

@idbrii
Copy link
Contributor Author

idbrii commented Sep 21, 2022

@xiejiangzhi I guess you mean the old code that I removed had that bug? I can add that test and ensure it passes: assert.is.equal(angle_between(a, a), '0.00')

when a:len() * b:len() is zero or lua5.4, we should check a == b.

I don't think angle_between should check for zero length vectors. At this low level, the user should be required to pass in valid inputs. There are very few asserts here and cpml doesn't have a way to strip asserts in production builds. Also, we have no correct result for angle_between(vec2.zero, vec2.zero)? I can update the doc comment to explain this restriction.

(Also not sure how lua5.4 changes anything?)

@xiejiangzhi
Copy link
Contributor

xiejiangzhi commented Sep 22, 2022

0 / 0 and acos(v) v > 1 will get nan.

maybe same case make v > 1 in mycode.

@xiejiangzhi
Copy link
Contributor

I found that in some cases abs(a:dot(b)) greater than abs(a:len() * b:len())

I think it's because of floating point issues, a == b or a == -b

@idbrii idbrii force-pushed the vec3-angle-between branch from 506c8fd to 179e717 Compare February 4, 2023 05:50
In vec2, angle_to is signed and angle_between is unsigned. We only have
the unsigned version in vec3, so use the corresponding name.

Remove the vec3 implementation from vec2 and use vec2's style
implementation. It doesn't allocate new vectors and works even if you
somehow call vec3.angle_between(vec2(), vec2()).
@idbrii idbrii force-pushed the vec3-angle-between branch from 179e717 to 19e54cf Compare February 4, 2023 05:54
@idbrii
Copy link
Contributor Author

idbrii commented Feb 4, 2023

Updated doc to mention nonzero inputs.
Added test for angle_between(a,a) == 0.

I think handling abs(a:dot(b)) > abs(a:len() * b:len()) should be done in a separate PR since it'd also move clamp to _private_util.

@shakesoda How does it look?

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.

2 participants