Ralith / hypermine

A hyperbolic voxel game
Apache License 2.0
160 stars 20 forks source link

Replace lazy_static! with OnceLock #383

Closed inthar-raven closed 7 months ago

inthar-raven commented 7 months ago

(ref #381)

patowen commented 7 months ago

I'm wondering if this can be refactored even further. Now that we don't have to put everything in one big lazy_static block, we might not need the helper functions at all, and instead, all the static OnceLocks could live in each of the public functions instead.

(If it does need to be separate, putting these functions in a private module within dodeca.rs would be useful for avoiding any name collisions and would probably help with readability).

patowen commented 7 months ago

Regarding my previous comment, Initial experimentation on my end suggests that it is indeed possible, although it would be trickier due to not as easily being able to directly convert one array to another. An example implementation for Side::normal_64 could be as simple as

/// Outward normal vector of this side
#[inline]
pub fn normal_f64(self) -> &'static na::Vector4<f64> {
    static SIDE_NORMALS_64: OnceLock<[na::Vector4<f64>; SIDE_COUNT]> = OnceLock::new();
    &SIDE_NORMALS_64.get_or_init(|| {
        let phi = libm::sqrt(1.25) + 0.5; // golden ratio
        let f = math::lorentz_normalize(&na::Vector4::new(1.0, phi, 0.0, libm::sqrt(phi)));

        let mut result: [na::Vector4<f64>; SIDE_COUNT] = [na::zero(); SIDE_COUNT];
        let mut i = 0;
        for (x, y, z, w) in [
            (f.x, f.y, f.z, f.w),
            (-f.x, f.y, -f.z, f.w),
            (f.x, -f.y, -f.z, f.w),
            (-f.x, -f.y, f.z, f.w),
        ] {
            for (x, y, z, w) in [(x, y, z, w), (y, z, x, w), (z, x, y, w)] {
                result[i] = na::Vector4::new(x, y, z, w);
                i += 1;
            }
        }
        result
    })[self as usize]
}

However, the implementation for Side::normal is not as easy, as it would need to be something like

/// Outward normal vector of this side
#[inline]
pub fn normal(self) -> &'static na::Vector4<f32> {
    static SIDE_NORMALS_F32: OnceLock<[na::Vector4<f32>; SIDE_COUNT]> = OnceLock::new();
    &SIDE_NORMALS_F32.get_or_init(|| {
        Side::iter()
            .map(|side| side.normal_f64().cast())
            .collect::<Vec<_>>()
            .try_into()
            .unwrap()
    })[self as usize]
}

Similar changes would have to be made to any field that depends on another field (which is most of them). @Ralith may have ideas on whether this extra complexity is worth this consolidation, but my current thought is that it probably isn't (although it could be in the future if paired with some other refactors that would make these implementations easier).

I think my suggestion for now to avoid making this PR too involved would be to disregard my previous suggestion in removing the helper functions and instead just dump all the things that used to be in lazy_static! into a private module within dodeca.rs.

Ralith commented 7 months ago

It's rarely a bad idea to keep things simple and narrowly scoped.

patowen commented 7 months ago

Looks like the CI failure in CI / Check protos is probably resolved (https://github.com/orgs/community/discussions/120966). I'll try rerunning that job.

EDIT: It worked this time.

inthar-raven commented 7 months ago

No idea why Clippy didn't catch formatting errors locally...

patowen commented 7 months ago

No idea why Clippy didn't catch formatting errors locally...

The errors caught by Clippy are different from the formatting issues caught by rustfmt. To catch the latter, you have to run cargo fmt to have it automatically format your code, or cargo fmt --check if you just want to check for formatting errors (although I recommend the former).

inthar-raven commented 7 months ago

Done. Changes should just be on Cargo.toml, cursor.rs and dodeca.rs now.