Skip to content

Conversation

aaronfranke
Copy link
Member

Revival of PR #108358. I looked through each commit, and modified it a little bit, but mostly this is the work of @AThousandShips so I kept the same author on the commits.

Even if the benefits are too negligible to show up on benchmarks, I think this is still worth doing. We don't have to label this PR as performance if the performance difference isn't measurable, we could just call it code hygiene to use the squared functions where applicable because that way we're following the same guideline internally as we recommend to users ("prefer it if you need to compare vectors or need the squared distance for some formula").

The changes I made on top of ATS's work:

  • Remove parenthesis for assignments as noted here Use square versions of distance/length where appropriate #108358 (comment)
  • Only make a variable to hold length squared when it's used multiple times, otherwise do an in-place multiply.
  • Chance one case of float to real_t in godot_body_pair_3d.cpp, avoids a needless cast in double builds.
  • Order variables by size (Vector2, then real_t, then int).
  • Add const in a few places.
  • Rename the commits to say what they do.

@aaronfranke aaronfranke added this to the 4.x milestone Aug 18, 2025
@aaronfranke aaronfranke requested review from a team as code owners August 18, 2025 14:32
@aaronfranke aaronfranke requested review from a team as code owners August 18, 2025 14:32
@clayjohn
Copy link
Member

See Lawnjelly's comment on the previous PR #108358 (comment)

I agree with lawnjelly.

We shouldn't be making code less clear and adding git noise when there is no measurable benefit. The only reason to use squared distance is when the algorithm requires squared distance or the performance benefit justifies it.

Changing from linear distance to squared distance is the opposite of good code hygiene, it is literally making the code harder to read and maintain.

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.

3 participants