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.

Sign up for GitHub

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

Math fixes #545

Merged
merged 9 commits into from
May 6, 2022
Merged

Math fixes #545

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
773f0ed
correct triangle Winding and imports
connor-lennox May 6, 2022
f00b094
more efficient winding direction calculation
connor-lennox May 6, 2022
0863aab
correct comments/function naming
connor-lennox May 6, 2022
60f14d3
switch f64 types to Scalar
connor-lennox May 6, 2022
59cc33d
simplify camera translation
connor-lennox May 6, 2022
75d5757
tweak comment
connor-lennox May 6, 2022
d076af5
use explicit type name
connor-lennox May 6, 2022
fcc2eac
shorten vertex coord calculation
connor-lennox May 6, 2022
5a75026
simplify translation construction
connor-lennox May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions crates/fj-math/src/transform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ use std::ops;

use nalgebra::Perspective3;

use crate::Scalar;

use super::{Aabb, Point, Segment, Triangle, Vector};

/// An affine transform
Expand Down Expand Up @@ -89,13 +91,16 @@ impl Transform {
fovy: f64,
znear: f64,
zfar: f64,
) -> [f64; 16] {
) -> [Scalar; 16] {
let projection = Perspective3::new(aspect_ratio, fovy, znear, zfar);
let mut res = [0f64; 16];
res.copy_from_slice(
(projection.to_projective() * self.0).matrix().as_slice(),
);
res
(projection.to_projective() * self.0)
.matrix()
.as_slice()
.iter()
.map(|f| Scalar::from(*f))
.collect::<Vec<Scalar>>()
Copy link
Owner

Choose a reason for hiding this comment

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

It's a bit unfortunate that we're going through a Vec here, due to the heap allocation that's probably expensive. I don't know how to change it off the top of my head, however. I'd have to experiment with it hands-on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also wanted to avoid a Vec here. You can't collect an iterator into an array, though. One alternative way to implement this is to iterate over the slice elements and assign into some mutable array (not as pretty, but avoids the Vec)

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, why not leave the code as it was before, with a slight modification:

let projection = Perspective3::new(aspect_ratio, fovy, znear, zfar);
let mut res = [0f64; 16];
res.copy_from_slice(
    (projection.to_projective() * self.0).matrix().as_slice(),
);
res.map(|f| Scalar::from(*f))

Should work?

But yeah, it's not that important. I'd be happy to merge a change that avoids the Vec, but I'm also fine with leaving it as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be a good way to avoid the Vec while still keeping things tidy. I don't think this will make or break the performance but it'll be good to keep in mind for later.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed. No reason to bend over backwards right now. If there's really a problem, we can fix it.

.try_into()
.unwrap()
}

/// Transform the given axis-aligned bounding box
Expand Down
6 changes: 4 additions & 2 deletions crates/fj-math/src/triangle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,15 @@ impl Triangle<3> {
dir: Vector<3>,
max_toi: f64,
solid: bool,
) -> Option<f64> {
) -> Option<Scalar> {
let ray = Ray {
origin: origin.to_na(),
dir: dir.to_na(),
};

self.to_parry().cast_local_ray(&ray, max_toi, solid)
self.to_parry()
.cast_local_ray(&ray, max_toi, solid)
.map(|f| f.into())
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/fj-viewer/src/graphics/transform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ impl Transform {
camera.far_plane(),
);

Self(transform.map(|val| val as f32))
Self(transform.map(|scalar| scalar.into_f32()))
}

/// Compute transform used for normals
Expand Down