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

[Merged by Bors] - Consistently use PI to specify angles in examples. #5825

Closed
wants to merge 3 commits into from

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Aug 28, 2022

Examples inconsistently use either TAU, PI, FRAC_PI_2 or FRAC_PI_4.
Often in odd ways and without useing the constants, making it difficult to parse.

  • Use PI to specify angles.
  • General code-quality improvements.
  • Fix borked hierarchy example.

@@ -34,7 +36,7 @@ fn setup(

// left wall
let mut transform = Transform::from_xyz(2.5, 2.5, 0.0);
transform.rotate_z(std::f32::consts::FRAC_PI_2);
transform.rotate_z(TAU / 4.);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has less accuracy than FRAC_PI_2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the loss of precision is relevant to any of our current examples,
but I wanted to investigate the difference in precision so i tried this:

assert_eq!(
    std::f64::consts::TAU / 4.,
    std::f64::consts::FRAC_PI_2
);

This assertion does not fail however. Is this test too naive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd compare the to_bits outcome to be safe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same result with to_bits.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in theory I think this makes sense, however there are a few things which make me hesitant. In my experience, education about how to think about radians is all in terms of Pi, not Tau. Therefore, this runs into issues of causing challenges for those with an intuition for radians in the standard forms.

Basically, none of this code is self describing to an audience without a formal education in relatively advanced maths, and familiarity with Tau. I think this is unnecessary. There are two paths forward I can see.

One is to recognise that the name Tau is not at all evocative of its meaning. Therefore, we can add const FULL_ROTATION_RADIANS: f64 = TAU; in bevy_utils. This has the issue of being a non-standard constant, but the name should be descriptive enough to make reasoning about it much easier.

The second is to give up on radians entirely, trust llvm to optimise things properly and use degrees, converting as needed. The main issue I can see there is that there is (currently 👀) no type system support for that, so it's potentially easy to accidentally pass e.g. 180 radians to a function, causing very weird bugs.

let a = std::f32::consts::FRAC_PI_2 - i as f32 * std::f32::consts::TAU / 10.0;
// Radius of internal vertices (2, 4, 6, 8, 10) is 100, it's 200 for external
// The angle between each vertex is 1/10 of a full rotation.
let a = i as f32 * TAU / 10.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be tempted to use degrees here, and then convert to radians as needed.

@@ -34,7 +36,7 @@ fn setup(

// left wall
let mut transform = Transform::from_xyz(2.5, 2.5, 0.0);
transform.rotate_z(std::f32::consts::FRAC_PI_2);
transform.rotate_z(TAU / 4.);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd compare the to_bits outcome to be safe

@Carter0
Copy link
Contributor

Carter0 commented Aug 29, 2022

So in theory I think this makes sense, however there are a few things which make me hesitant. In my experience, education about how to think about radians is all in terms of Pi, not Tau. Therefore, this runs into issues of causing challenges for those with an intuition for radians in the standard forms.

Basically, none of this code is self describing to an audience without a formal education in relatively advanced maths, and familiarity with Tau. I think this is unnecessary. There are two paths forward I can see.

One is to recognise that the name Tau is not at all evocative of its meaning. Therefore, we can add const FULL_ROTATION_RADIANS: f64 = TAU; in bevy_utils. This has the issue of being a non-standard constant, but the name should be descriptive enough to make reasoning about it much easier.

The second is to give up on radians entirely, trust llvm to optimise things properly and use degrees, converting as needed. The main issue I can see there is that there is (currently 👀) no type system support for that, so it's potentially easy to accidentally pass e.g. 180 radians to a function, causing very weird bugs.

I think TAU is a lot easier to understand than PI once you get the hang of it. And almost all game math libraries use radians so I don't really think it makes sense to use degrees anywhere.

@tim-blackbird
Copy link
Contributor Author

tim-blackbird commented Aug 29, 2022

Basically, none of this code is self describing to an audience without a formal education in relatively advanced maths

This makes TAU seem like something tricky when it is rather straightforward.
If someone knows PI then they know that PI * 2 is a full rotation.
The missing piece would be knowing that TAU == PI * 2 which is easy enough to memorize.

And for people new to radians altogether. Teaching that TAU is the same as 360 degrees is more palatable than PI being 180 degrees and having to think in half-rotations.

Reading TAU / 4. it's quick to see that that is a 1/4 of a turn.
Just like TAU * 2. * time.delta_seconds() is something being rotated 2 times per second.
Comparing that to PI:
PI / 2. or FRAC_PI_2 is 1/4 of a turn.
PI * 4 * time.delta_seconds() is something being rotated 2 times per second.
You end up having to multiply or divide by 2 in your head.

familiarity with Tau

Fair. Can't understand what you don't know, and schools don't seem to teach it.
It is very simple though.

One is to recognise that the name Tau is not at all evocative of its meaning.

Neither is PI, but of course more people already know PI.
I'd rather teach people what TAU means through comments in the examples.
FULL_ROTATION_RADIANS would need an f32 version as well.
It's also even more verbose than to_radians.

The second is to give up on radians entirely

😅 kind of difficult when every math lib uses radians. Trust me. I'd replace everything with degrees if I could, hah.
It is probably worthwhile to switch to using to_radians in some examples though,
but it's too verbose to use everywhere. 180_f32.to_radians() vs TAU / 2..

TL;DR
Thinking of angles in terms of full rotations is intuitive, but then PI requires dividing/multiplying by two in your head every time you read or write an angle.
TAU is a very simple constant. A full rotation in radians. People more familiar with PI would still pick it up very quickly.
I expect people new to using radians have an easier time learning if they are taught to use TAU as a full rotation rather than PI as a half rotation.

Using degrees would be nice but since we need to use radians in the end it introduces verbose conversions in the middle of the actual math.

I'm going to try to use to_radians in some of the examples and see how that goes.

@Weibye Weibye added C-Code-Quality A section of code that is hard to understand or change A-Core Common functionality for all bevy apps labels Aug 29, 2022
@cart
Copy link
Member

cart commented Aug 29, 2022

I think TAU is a lot easier to understand than PI once you get the hang of it.

Thats the problem. Examples are intended to be minimal / quick / easy to understand / etc and are often peoples' first exposure to the engine.

The percentage of programmers that know what PI represents off the top of their head is probably close to 98%. The percentage of programmers that know what TAU represents off the top of their heard is probably closer to 5%.

It doesn't matter that it is a "simple" concept (or marginally more "natural" once you understand it). The vast majority of people think about radians in terms of PI. Using TAU in our examples adds one more (unnecessary) learning hurdle to the Bevy onboarding process, which is already full of necessary learning hurdles.

"Optimizing" this part of peoples' workflows doesn't seem worth the price of admission, especially in this part of our learning material.

@rparrett
Copy link
Contributor

I don't have an opinion about the TAU/PI debate, but I appreciated all the little bits of incidental cleanup in this PR, especially importing (your favorite constant). I checked a few of the examples where the changes were less trivial and they checked out.

Copy link
Contributor

@afonsolage afonsolage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the TAU vs PI discussion, I think the reworks is pretty good.

I'm in favor of TAU, since I think it's easier to understand, but the majority of newcomers won't, most of them still struggle to understand radians vs degree, so maybe we should add on bevy_utils or bevy_math some consts like DEGREE_360, DEGREE_180, DEGREE_90, DEGREE_45 and accept that examples will be better written in degrees, since anyone who knows PI or TAU will understand what is a transform.rotate_z(DEGREE_90)

@tim-blackbird
Copy link
Contributor Author

"Optimizing" this part of peoples' workflows doesn't seem worth the price of admission, especially in this part of our learning material.

You're right. The most important thing is the examples being easy to start out with.
I was thinking that since there is already quite a few things to learn adding one more wouldn't hurt, but I realize now that is not the right way to think about it.

consts like DEGREE_360, DEGREE_180, DEGREE_90, DEGREE_45

DEGREE_360 is 360 degrees... but in radians, that might be confusing.

PS: A lot of the discussion around PI/TAU is extremely cringe.
The only argument I'll make is that TAU avoids some mental math when reading/writing for me personally.
If you want a good laugh, go read the PI/TAU manifestos. They're so bad haha.

@tim-blackbird tim-blackbird changed the title Use TAU to specify angles in examples. Consistently use PI to specify angles in examples. Aug 30, 2022
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. My last holdout was the precision of PI / 2. and PI / 4. vs FRAC_PI_2 and FRAC_PI_4. (a previous comment already showed this wasn't a problem for TAU, but that doesn't necessarily make it true for PI).

They are equal!

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f3c460954233270951053011f30d5fcb

Under those circumstances, I agree with the choice to use the one PI constant for this. Simpler and more flexible.

@cart
Copy link
Member

cart commented Aug 30, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 30, 2022
Examples inconsistently use either `TAU`, `PI`, `FRAC_PI_2` or `FRAC_PI_4`.
Often in odd ways and without `use`ing the constants, making it difficult to parse.

 * Use `PI` to specify angles.
 * General code-quality improvements.
 * Fix borked `hierarchy` example.


Co-authored-by: devil-ira <justthecooldude@gmail.com>
@bors bors bot changed the title Consistently use PI to specify angles in examples. [Merged by Bors] - Consistently use PI to specify angles in examples. Aug 30, 2022
@bors bors bot closed this Aug 30, 2022
@tim-blackbird tim-blackbird deleted the tautology branch October 21, 2022 15:10
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
Examples inconsistently use either `TAU`, `PI`, `FRAC_PI_2` or `FRAC_PI_4`.
Often in odd ways and without `use`ing the constants, making it difficult to parse.

 * Use `PI` to specify angles.
 * General code-quality improvements.
 * Fix borked `hierarchy` example.


Co-authored-by: devil-ira <justthecooldude@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
Examples inconsistently use either `TAU`, `PI`, `FRAC_PI_2` or `FRAC_PI_4`.
Often in odd ways and without `use`ing the constants, making it difficult to parse.

 * Use `PI` to specify angles.
 * General code-quality improvements.
 * Fix borked `hierarchy` example.


Co-authored-by: devil-ira <justthecooldude@gmail.com>
AlexanderStein pushed a commit to AlexanderStein/bevy that referenced this pull request Sep 8, 2024
Commit 65252bb (Consistently use `PI` to specify angles in examples.
(bevyengine#5825)) changed from using push_children to add_child without
adjusting the comment
github-merge-queue bot pushed a commit that referenced this pull request Sep 8, 2024
Commit 65252bb (Consistently use `PI` to specify angles in examples.
(#5825)) changed from using push_children to add_child without adjusting
the comment

# Objective

- Fix the comments to align with the code

Co-authored-by: Alexander Stein <alexander.stein@mailbox.org>
BD103 pushed a commit to BD103/bevy that referenced this pull request Sep 9, 2024
Commit 65252bb (Consistently use `PI` to specify angles in examples.
(bevyengine#5825)) changed from using push_children to add_child without adjusting
the comment

# Objective

- Fix the comments to align with the code

Co-authored-by: Alexander Stein <alexander.stein@mailbox.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants