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

Default to f32 instead of f64, removing unnecessary casts #360

Merged
merged 3 commits into from
Apr 14, 2024

Conversation

patowen
Copy link
Collaborator

@patowen patowen commented Feb 25, 2024

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 in dodeca.cs, only returned f64, 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 return f64 instead.

To take advantage of these changes, some functions, such as the ones in traversal were altered to take in an f32 instead of an f64, ensuring that all math is done with f32. As a side note, this highlighted an oversight in ensure_nearby and nearby_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 to f32). This PR also fixes that oversight.

This PR still uses f64 for Plane to avoid making #102 worse, and it uses f64 when interacting with na::RealField because otherwise, casting types to na::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 vs f64 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 accommodating f64 entirely and use f32 for everything, simplifying the code base. Part of this would involve editing math.rs to use f32 instead of na::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 in dodeca and math to work with f32 or f64. It would likely be a good idea for us to define our own trait implemented by f32 and f64 and stop using na::RealField (at least for things that depend on dodeca), since that would give us more control. For instance, it would allow us to write more generic dodeca methods that return f32 or f64 depending on context.

common/src/traversal.rs Show resolved Hide resolved
@patowen patowen merged commit 1fc63f4 into Ralith:master Apr 14, 2024
6 checks passed
@patowen patowen deleted the default-to-f32 branch April 14, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants