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

Float literals - fix main primitives to use real_t casting #58488

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

lawnjelly
Copy link
Member

Uses (real_t) casting to ensure appropriate calculations are done in 32 bit where real_t is compiled as 32 bit.

2nd part of #57703

Notes

  • There are also a couple of places I've added extra brackets where my compiler has indicated the order of precedence is ambiguous.

Uses (real_t) casting to ensure appropriate calculations are done in 32 bit where real_t is compiled as 32 bit.
@akien-mga
Copy link
Member

akien-mga commented Feb 25, 2022

This might have been discussed already but wouldn't it be simpler to put the (real_t) cast in the math defines directly? Or do we want them to stay double by default and just cast to real_t in critical paths where it makes sense?

Also, how does this compare with #58214 ?

@lawnjelly
Copy link
Member Author

lawnjelly commented Feb 25, 2022

This might have been discussed already but wouldn't it be simpler to put the (real_t) cast in the math defines directly? Or do we want them to stay double by default and just cast to real_t in critical paths where it makes sense?

I did mention this in the first PR. One disadvantage of putting the cast in the math defines is that sometimes when compiling real_t as float we actually may want to use a double precision constant. So we probably either end up needing two sets of defines, or we cast when using.

This is less relevant with constants such as CMP_EPSILON, but important with constants with large number of significant figures like PI etc.

Either is okay .. changing the define means it will be used in more places automatically, whereas casting in place makes it more explicit what is going on from the calling code. Pick your poison! 😀

(I'm happy to go with either approach BTW, but I think the consensus during the PR meeting was to go with changing the literals and casting, limiting the changes to the math / bottleneck classes).

Also, how does this compare with #58214 ?

These are reasonably separable as problems (although there may be some overlap in code area - if we merge one PR the other might need changing a little and vice versa). One is to ensure 32 bit calculations are done as 32 bit when real_t is float, and one is about changing epsilons when compiling with real_t as double.

I don't think a decision on one affects the other, aside from minor modifications.

@akien-mga akien-mga merged commit de94461 into godotengine:master Mar 1, 2022
@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.

3 participants