Skip to content

Conversation

@aaronfranke
Copy link
Member

This PR is a further subset of the changes in PR #109756, previously in PR #109738. This PR includes the changing of double literals to float literals, and using real_t consistently to avoid needless casting. I also left in some adjacent changes like adding const, but removed all additions of const which didn't also have changes to float literals/types. This way this PR can improve those areas without unnecessary code churn (such as being changed again in a later PR).

@lawnjelly mentioned here that the float literal changes are easy to approve, but the rest require more consensus.

@aaronfranke aaronfranke added this to the 4.x milestone Aug 20, 2025
@aaronfranke aaronfranke requested review from a team as code owners August 20, 2025 10:36
@aaronfranke aaronfranke requested review from a team as code owners August 20, 2025 10:36
@KoBeWi
Copy link
Member

KoBeWi commented Aug 20, 2025

This PR includes the changing of double literals to float literals, and using real_t consistently to avoid needless casting.

Isn't the casting done at compile time? Why using float literals even matter?

@aaronfranke
Copy link
Member Author

@KoBeWi If you have something.length() > 0.0 then it will cast something.length() to a double at runtime if Godot is compiled with real_t defined as float. However, something.length() > 0.0f has no runtime casts, because it's either float on both sides, or if Godot is compiled with doubles, 0.0f is compile-time casted to double.


DragType resize_drag = DRAG_NONE;
real_t radius = (select_handle->get_size().width / 2) * 1.5;
const real_t radius = select_handle->get_size().width * (1.5f / 2.0f);
Copy link
Member

@lawnjelly lawnjelly Sep 29, 2025

Choose a reason for hiding this comment

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

Adding const changes are unrelated to changes to literals, and should be in a different PR imo (I wouldn't approve them personally as I'm not altogether convinced at the idea of adding const for all const local vars, which is why I haven't approved the PR).

One is a material change which can make the assembly better, the other is a stylistic change which hasn't afaik been agreed on as a core standard.

It turns out I had previously specifically mentioned the const issue as a potential blocker in #109756 (comment) .

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.

4 participants