Math cleanup #523

connor-lennox · 2022-05-05T02:22:56Z

Addresses #518 .

The work here is split into two main sections:

Additions to fj-math that forward behaviour from nalgebra and parry
Reworking components in fj-viewer and fj-kernel to use only the types from fj-math

One major change is the conversion of fj-math::Transform from wrapping a TGeneral to TAffine nalgebra::Transform. This doesn't have any impact on anything yet (since the TGeneral capabilities were not being used anywhere) but it's something to consider for if fj-math::Transform becomes more generalized in the future.

As always, happy to discuss these changes either here or on Matrix. This is (one of) my first forays into open-source Rust so please don't be afraid to pick it apart and show me how things could be better implemented.

hannobraun · 2022-05-05T08:44:45Z

Thank you for the pull request, @connor-lennox! I've had a quick look, and this looks good. I'll do a proper review later today.

This is (one of) my first forays into open-source Rust so please don't be afraid to pick it apart and show me how things could be better implemented.

Understood. I'll be extra nitpicky then 😁

hannobraun

Thank you, @connor-lennox, this looks great!

I've found one case, where your modifications changed the behavior of the application (mouse wheel input). I've left a comment with a suggestion there. I think I can accept my own suggestion after submitting the review, and merge this immediately.

I've left a bunch of other comment with feedback and suggestions. All of them are optional, and some points (the ones where I noted that I'm being nitpicky), I wouldn't even have mentioned, if you hadn't specifically encouraged the feedback.

One additional note (also nitpicky): Some of the commits could be split into multiple smaller ones, with each smaller commit being focused on one specific change. If some changes were uncontroversial, while others required more discussion, I might want to merge the uncontroversial ones immediately. That would be easier, if every change were its own commit. Doesn't make a difference in this case though.

Feel free to follow up with another pull request that incorporates (some of) my feedback, or not, as you see fit.

hannobraun · 2022-05-05T11:08:51Z

crates/fj-math/src/triangle.rs

@@ -1,5 +1,27 @@
 use super::{Point, Scalar};

+use parry2d_f64::utils::point_in_triangle::{corner_direction, Orientation};


Okay, this is going to be extra-nitpicky. The style that most of Fornjot's code follows is to keep a specific order of imports: First from the standard library, then from 3rd-party libraries, then from crate::, the relative imports (self::, super::). This new import breaks that order.

This is not important, and I wouldn't have said anything, if you hadn't requested the extra attention, but it can make code slightly more readable, by making imports slightly easier to find. It's also what rust-analyzer does by default, if you use it to automatically add an import (at least in VS Code).

hannobraun · 2022-05-05T11:13:22Z

crates/fj-math/src/triangle.rs

+/// Winding direction of a triangle.
+pub enum Winding {
+    /// Counter-clockwise
+    Ccw,
+    /// Clockwise
+    Cw,
+    /// Neither (straight lines)
+    None,
+}
+
+impl From<Orientation> for Winding {
+    fn from(o: Orientation) -> Self {
+        match o {
+            Orientation::Ccw => Winding::Ccw,
+            Orientation::Cw => Winding::Cw,
+            Orientation::None => Winding::None,
+        }
+    }
+}


I prefer ordering code from high-level to low-level. In this case, this would mean first Triangle, then stuff triangle uses or returns further down.

I think this makes code easier to read sequentially, as it's easier to understand the high-level concepts first. By the time you get to the details, you've already seen what the module is about and how the details are used, which makes it easier to understand those too.

By doing this consistently, it makes things within a module also easier to find.

Again, this is nitpicky and not that important.

Also, I don't think we need Winding::None, as Triangle is validated on construction. We can just panic when encountering Orientation::None (maybe using unreachable!()), as that would be a bug in our code somewhere.

hannobraun · 2022-05-05T11:19:07Z

crates/fj-math/src/triangle.rs

+        let [v0, v1, v2] = self.points;
+        corner_direction(&v0.to_na(), &v1.to_na(), &v2.to_na()).into()


Alternative (not tested, might not compile):

Suggested change

let [v0, v1, v2] = self.points;

corner_direction(&v0.to_na(), &v1.to_na(), &v2.to_na()).into()

let [v0, v1, v2] = self.points.map(|point| point.to_na());

corner_direction(&v0, &v1, &v2).into()

hannobraun · 2022-05-05T11:21:09Z

crates/fj-math/src/point.rs

@@ -58,6 +58,11 @@ impl<const D: usize> Point<D> {
            coords: self.coords.to_xyz(),
        }
    }
+
+    /// Gives the distance between two Points.
+    pub fn distance(p1: &Point<D>, p2: &Point<D>) -> Scalar {


Alternative:

Suggested change

pub fn distance(p1: &Point<D>, p2: &Point<D>) -> Scalar {

pub fn distance_to(&self, other: &Point<D>) -> Scalar {

Not sure if that's actually better. It might be a bit nicer to call.

hannobraun · 2022-05-05T11:24:36Z

crates/fj-math/src/transform.rs

@@ -1,12 +1,20 @@
 use std::ops;

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

 /// A transform


Suggested change

/// A transform

/// An affine transform

hannobraun · 2022-05-05T12:23:40Z

crates/fj-viewer/src/graphics/vertices.rs

+            position: [
+                point.x.into_f32(),
+                point.y.into_f32(),
+                point.z.into_f32(),
+            ],


Can be shortened:

Suggested change

position: [

point.x.into_f32(),

point.y.into_f32(),

point.z.into_f32(),

],

position: point.coords.components.map(|scalar| scalar.into_f32()),

crates/fj-viewer/src/input/handler.rs

hannobraun · 2022-05-05T12:34:52Z

crates/fj-viewer/src/input/movement.rs

+                        * Transform::translation(Vector::from_components_f64(
+                            [offset.x.into(), offset.y.into(), 0.0],
+                        ));


Can be shortened, I think:

Suggested change

* Transform::translation(Vector::from_components_f64(

[offset.x.into(), offset.y.into(), 0.0],

));

* Transform::translation(Vector::from(

[offset.x, offset.y, 0.0],

));

Or even shorter, although maybe less clear:

Suggested change

* Transform::translation(Vector::from_components_f64(

[offset.x.into(), offset.y.into(), 0.0],

));

* Transform::translation(offset.to_xyz());

hannobraun · 2022-05-05T12:38:24Z

crates/fj-viewer/src/input/rotation.rs

+            let rotate_around: Vector<3> =
+                self.focus_point.0.unwrap_or_else(Point::origin).coords;


Why the explicit type annotation?

Suggested change

let rotate_around: Vector<3> =

self.focus_point.0.unwrap_or_else(Point::origin).coords;

let rotate_around =

self.focus_point.0.unwrap_or_else(Point::origin).coords;

hannobraun · 2022-05-05T12:38:58Z

crates/fj-viewer/src/input/rotation.rs


            let f = 0.005;

            let angle_x = diff_y * f;
            let angle_y = diff_x * f;

-            let trans = Translation::from(rotate_around);
+            let trans = Transform::translation(rotate_around);


Again, this is nitpicky, but we're rotation around a point, not a vector. So maybe leave rotate_around as a Point, and add the .coords here, where a vector is needed:

Suggested change

let trans = Transform::translation(rotate_around);

let trans = Transform::translation(rotate_around.coords);

connor-lennox added 5 commits May 4, 2022 12:44

clear parry reference from fj-kernal delaunay

87db8f4

add additional functionality

8e13433

convert to TAffine, new functions

fc2d870

convert nalgebra/parry types to fj-math types

d21534b

remove nalegebra/parry dependencies

7e0eed2

connor-lennox requested a review from hannobraun as a code owner May 5, 2022 02:22

hannobraun previously approved these changes May 5, 2022

View reviewed changes

Fix zoom behavior

124ed26

hannobraun dismissed their stale review via 124ed26 May 5, 2022 12:42

hannobraun merged commit ca75e22 into hannobraun:main May 5, 2022

connor-lennox deleted the math-cleanup branch May 6, 2022 16:14

hannobraun mentioned this pull request May 7, 2022

Clean up use of math code #518

Closed

hannobraun mentioned this pull request May 19, 2022

Support arbitrary transformation matrices #101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Math cleanup #523

Math cleanup #523

connor-lennox commented May 5, 2022

hannobraun commented May 5, 2022

hannobraun left a comment

hannobraun May 5, 2022

hannobraun May 5, 2022

hannobraun May 5, 2022

hannobraun May 5, 2022

hannobraun May 5, 2022

hannobraun May 5, 2022

hannobraun May 5, 2022

hannobraun May 5, 2022

hannobraun May 5, 2022

		@@ -1,5 +1,27 @@
		use super::{Point, Scalar};

		use parry2d_f64::utils::point_in_triangle::{corner_direction, Orientation};

		let [v0, v1, v2] = self.points;
		corner_direction(&v0.to_na(), &v1.to_na(), &v2.to_na()).into()

	pub fn distance(p1: &Point<D>, p2: &Point<D>) -> Scalar {
	pub fn distance_to(&self, other: &Point<D>) -> Scalar {

		let rotate_around: Vector<3> =
		self.focus_point.0.unwrap_or_else(Point::origin).coords;

	let trans = Transform::translation(rotate_around);
	let trans = Transform::translation(rotate_around.coords);

Math cleanup #523

Math cleanup #523

Conversation

connor-lennox commented May 5, 2022

hannobraun commented May 5, 2022

hannobraun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment