Default to f32 instead of f64, removing unnecessary casts #360
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
A common issue I have encountered when trying to implement various features was the fact that much of Hypermine's code relies on
f32
, but certain common functions, especially indodeca.cs
, only returnedf64
, requiring a lot of redundant casting.Changes
This PR changes dodeca to return
f32
types by default, with alternative methods with_f64
appended to their name to allow them to returnf64
instead.To take advantage of these changes, some functions, such as the ones in
traversal
were altered to take in anf32
instead of anf64
, ensuring that all math is done withf32
. As a side note, this highlighted an oversight inensure_nearby
andnearby_nodes
, as a depth-search search was used instead of the superior breadth-first search, causing unnecessary numerical instability (which resulted in NaNs after the switch tof32
). This PR also fixes that oversight.This PR still uses
f64
forPlane
to avoid making #102 worse, and it usesf64
when interacting withna::RealField
because otherwise, casting types tona::RealField
does not work. (See dimforge/simba#54)Moving forward
This PR is incremental in nature, as it does not yet fully solve the
f32
vsf64
disconnect. To do that, I can imagine two approaches, depending on what we want Hypermine's engine to be capable of, although there are almost certainly other ways:Option 1 (Move entirely to
f32
): Stop accommodatingf64
entirely and usef32
for everything, simplifying the code base. Part of this would involve editingmath.rs
to usef32
instead ofna::RealField + Copy
, which could simplify things greatly. The main downside would be that some ad-hoc implementations would run into numerical issues more quickly. For instance, the issue related to #102 would be worsened until we fix it.Option 2 (Embrace
f64
as an first-class option): Use traits more consistently to allow our core functions indodeca
andmath
to work withf32
orf64
. It would likely be a good idea for us to define our own trait implemented byf32
andf64
and stop usingna::RealField
(at least for things that depend ondodeca
), since that would give us more control. For instance, it would allow us to write more genericdodeca
methods that returnf32
orf64
depending on context.