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

Replace lazy_static! with OnceLock #383

Merged
merged 3 commits into from
Apr 26, 2024
Merged

Conversation

inthar-raven
Copy link
Contributor

(ref #381)

Copy link
Owner

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to remove the lazy_static dependency from Cargo.toml.

common/src/cursor.rs Outdated Show resolved Hide resolved
common/src/cursor.rs Outdated Show resolved Hide resolved
common/src/dodeca.rs Outdated Show resolved Hide resolved
common/src/dodeca.rs Outdated Show resolved Hide resolved
@patowen
Copy link
Collaborator

patowen commented Apr 24, 2024

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
Copy link
Collaborator

patowen commented Apr 24, 2024

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
Copy link
Owner

Ralith commented Apr 24, 2024

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

common/src/dodeca.rs Outdated Show resolved Hide resolved
common/src/dodeca.rs Outdated Show resolved Hide resolved
@patowen
Copy link
Collaborator

patowen commented Apr 24, 2024

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.

common/src/dodeca.rs Outdated Show resolved Hide resolved
@inthar-raven
Copy link
Contributor Author

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

@patowen
Copy link
Collaborator

patowen commented Apr 26, 2024

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).

Copy link
Owner

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, pretty close now! It's delightful to see rustfmt start working for all that formerly macro-wrapped code.

client/src/graphics/voxels/mod.rs Outdated Show resolved Hide resolved
common/src/dodeca.rs Outdated Show resolved Hide resolved
common/src/dodeca.rs Show resolved Hide resolved
client/src/graphics/voxels/mod.rs Outdated Show resolved Hide resolved
common/src/traversal.rs Outdated Show resolved Hide resolved
server/src/sim.rs Outdated Show resolved Hide resolved
@inthar-raven
Copy link
Contributor Author

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

Copy link
Owner

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Ralith Ralith merged commit 00f7f23 into Ralith:master Apr 26, 2024
6 checks passed
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.

3 participants