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 the freshly stabilized f32::total_cmp methods #5152

Open
alice-i-cecile opened this issue Jun 30, 2022 · 5 comments
Open

Use the freshly stabilized f32::total_cmp methods #5152

alice-i-cecile opened this issue Jun 30, 2022 · 5 comments
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@alice-i-cecile
Copy link
Member

The FloatOrd type exists to workaround the inability to order Rust floats.

Fortunately, the f32::total_cmp method just stabilized.

We should at least use this internally, and investigate whether we can remove this wrapper type completely.

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change A-Math Fundamental domain-agnostic mathematical operations labels Jun 30, 2022
@DJMcNab
Copy link
Member

DJMcNab commented Jun 30, 2022

Luckily, f32 is small enough to test this

But I suspect that total_ord would disagree with the hash implementation.

(We should add a test to loop through all f32s and confirm that Hash's requirements (wrt equality) hold)

@alice-i-cecile alice-i-cecile changed the title Use the freshly stabilize f32::total_cmp methods Use the freshly stabilized f32::total_cmp methods Jul 1, 2022
@mockersf
Copy link
Member

mockersf commented Jul 1, 2022

We probably won't be free of FloatOrd as it's used to be generic overt type that impl Ord, which f32 doesn't even with total_cmp.
So this could be just a simplification of the Ord impl for FloatOrd

@nhunds
Copy link

nhunds commented Jul 4, 2022

I'm new to bevy and open source contribution. Is my understanding right that the only work to do on this Issue is to replace the initial partial_cmp() function call with a total_cmp() call? Or are there any differences in the current implementation and the total_cmp() implementation in the results of the ordering?

@alice-i-cecile
Copy link
Member Author

Is my understanding right that the only work to do on this Issue is to replace the initial partial_cmp() function call with a total_cmp() call

This is the first step. Feel free to open a PR with this change and the reviewers will be able to provide more guidance :)

Like @DJMcNab says though, we should consider testing to ensure that the implementation of PartialEq and Hash still agree, even in pathological edge cases.

@NthTensor
Copy link
Contributor

The same approach taken in #10558 should be used here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants