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

tim-blackbird · 2022-08-28T19:34:10Z

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.

bjorn3 · 2022-08-28T19:39:47Z

examples/3d/lighting.rs

@@ -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.);


This has less accuracy than FRAC_PI_2.

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?

I'd compare the to_bits outcome to be safe

Same result with to_bits.

DJMcNab

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.

DJMcNab · 2022-08-28T21:12:15Z

examples/2d/mesh2d_manual.rs

-        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;


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

DJMcNab · 2022-08-28T21:13:45Z

examples/3d/lighting.rs

@@ -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.);


I'd compare the to_bits outcome to be safe

Carter0 · 2022-08-29T03:42:43Z

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 · 2022-08-29T13:18:10Z

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.

cart · 2022-08-29T22:56:23Z

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 · 2022-08-30T01:14:31Z

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.

afonsolage

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 · 2022-08-30T08:26:16Z

"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.}

cart

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 · 2022-08-30T19:51:54Z

bors r+

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 · 2022-08-30T20:13:29Z

Pull request successfully merged into main.

Build succeeded:

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>

Commit 65252bb (Consistently use `PI` to specify angles in examples. (bevyengine#5825)) changed from using push_children to add_child without adjusting the comment

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>

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>

taud.

c10625f

bjorn3 reviewed Aug 28, 2022

View reviewed changes

DJMcNab reviewed Aug 28, 2022

View reviewed changes

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

afonsolage approved these changes Aug 30, 2022

View reviewed changes

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

gudbye, fren

b04a1b6

tim-blackbird force-pushed the tautology branch from 1dc55c2 to b04a1b6 Compare August 30, 2022 09:24

cargo fmt

7243727

cart approved these changes Aug 30, 2022

View reviewed changes

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 bot closed this Aug 30, 2022

tim-blackbird deleted the tautology branch October 21, 2022 15:10

AlexanderStein mentioned this pull request Sep 8, 2024

Fix comment in example #15096

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tim-blackbird commented Aug 28, 2022 •

edited

Loading

bjorn3 Aug 28, 2022

tim-blackbird Aug 28, 2022

DJMcNab Aug 28, 2022

tim-blackbird Aug 29, 2022

DJMcNab left a comment

DJMcNab Aug 28, 2022

DJMcNab Aug 28, 2022

Carter0 commented Aug 29, 2022

tim-blackbird commented Aug 29, 2022 •

edited

Loading

cart commented Aug 29, 2022

rparrett commented Aug 30, 2022

afonsolage left a comment •

edited

Loading

tim-blackbird commented Aug 30, 2022

cart left a comment

cart commented Aug 30, 2022

bors bot commented Aug 30, 2022

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

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

Conversation

tim-blackbird commented Aug 28, 2022 • edited Loading

bjorn3 Aug 28, 2022

Choose a reason for hiding this comment

tim-blackbird Aug 28, 2022

Choose a reason for hiding this comment

DJMcNab Aug 28, 2022

Choose a reason for hiding this comment

tim-blackbird Aug 29, 2022

Choose a reason for hiding this comment

DJMcNab left a comment

Choose a reason for hiding this comment

DJMcNab Aug 28, 2022

Choose a reason for hiding this comment

DJMcNab Aug 28, 2022

Choose a reason for hiding this comment

Carter0 commented Aug 29, 2022

tim-blackbird commented Aug 29, 2022 • edited Loading

cart commented Aug 29, 2022

rparrett commented Aug 30, 2022

afonsolage left a comment • edited Loading

Choose a reason for hiding this comment

tim-blackbird commented Aug 30, 2022

cart left a comment

Choose a reason for hiding this comment

cart commented Aug 30, 2022

bors bot commented Aug 30, 2022

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

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

tim-blackbird commented Aug 28, 2022 •

edited

Loading

tim-blackbird commented Aug 29, 2022 •

edited

Loading

afonsolage left a comment •

edited

Loading