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

Remove Angle? #22

Closed
repi opened this issue Sep 17, 2019 · 5 comments
Closed

Remove Angle? #22

repi opened this issue Sep 17, 2019 · 5 comments

Comments

@repi
Copy link
Contributor

repi commented Sep 17, 2019

Have there been any thoughts about keeping or removing the Angle type?

I'm a bit conflicted but it feels a bit out of place to have a higher level type wrapper like Angle to make radians vs degrees explicit and safe, but at the same time not have a type such as Point3.

I'm not proposing to add a Point3 though and think it is good to just have the core primitive types here.

I do like the explicitness of Angle::from_radians but I could just as well see glam declaring that all angles are radians (which is how they are stored anyway) and that is how we've have defined inside our engine as well.

So I do have a slight preference to simply not having an Angle type, and wanted to start a discussion to see what @bitshifter and everyone else thinks.

@bitshifter
Copy link
Owner

I think it probably does make sense to remove it on balance. A lot of my use of glam has involved manually entering angles for various things, those tend to be in degrees so the explicitness has been good. That said, it's not that common to do that usually. Also less code is good.

@bitshifter
Copy link
Owner

bitshifter commented Oct 4, 2019

Note that everything that did take Angle as a parameter now takes f32 in radians with the exception of perspective matrix creation which expects fov_y parameters to be in degrees. @repi let me know if that's what you'd expect with regards to the fov_y parameter.

I plan to released this and a bunch of other breaking changes as version 0.8.0, once I've got everything else done.

@repi
Copy link
Contributor Author

repi commented Oct 4, 2019

Sweet. And yeah the fov_y parameter should be in radians as well

@bitshifter
Copy link
Owner

EmbarkStudios/rust-ecosystem#36 (comment) fixed in release 0.8.0.

@repi
Copy link
Contributor Author

repi commented Oct 14, 2019

Awesome thanks!

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

No branches or pull requests

2 participants