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

Use Math_TAU and deg2rad/etc in more places and optimize code #37547

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Apr 3, 2020

This PR has many related changes to many parts of the code that work with angles or turns.

Floating-point math is not guaranteed to be associative, so in order for these pieces of code to be optimized, you either need to include parenthesis, calculate things in advance, or use -ffast-math, or else the compiler can't optimize these. Godot does not use that compile option, so we need to be explicit to have better performance.

  • Pre-calculate step sizes in many places where for loops are used (Ctrl+F -> _step). This will improve performance quite a bit, since the code is executed many times in a row. It also improves readability in my opinion, since the old code had very complex lines.

  • Add parenthesis to Math::deg2rad and Math::rad2deg around the constants. This allows the compiler to optimize this to a single multiplication (it can't do this automatically unless we use -ffast-math).

  • Use Math::deg2rad and Math::rad2deg where applicable instead of * Math_PI / 180 etc. This should reduce code duplication, improve readability, and improve performance (just adding parenthesis would improve the performance, but using the method improves readability at the same time).

  • Replace many occurrences of Math_PI multiplied by 2 with Math_TAU. This improves readability (including by avoiding parenthesis).

  • Optimize some very ugly code in editor/node_3d_editor_gizmos.cpp at line 1074.

  • Group constant calculations together. This improves performance and readability.

  • Remove some unnecessary casts.

@reduz
Copy link
Member

reduz commented Sep 26, 2020

I am not entirely sure if I like this, I believe it makes code more confusing. Could be discussed at a PR meeting when they come back.

@Duroxxigar
Copy link
Contributor

Duroxxigar commented Sep 26, 2020

Can you elaborate on any area that makes the code more confusing? @reduz

For me, I don't see either side more or less confusing to read. Both versions are easy to read.

Copy link
Contributor

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

Looks fine.

@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants