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

geometry::Arc: use angles for start and end #1609

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rmburg
Copy link
Contributor

@rmburg rmburg commented Jan 27, 2025

Why? What?

What the title says. Previously, we used Point2s for the start and end points of arcs.
This made arcs over-defined. A neat side effect of this change is reduced complexity of most of the affected calculations.

ToDo / Known Issues

It's perfect

Ideas for Next Iterations (Not This PR)

It's perfect

How to Test

Stare at the code.
I can add more unit tests if there are any areas where you are unsure if the changes are correct.

@rmburg rmburg added the is:Refactoring No changes in functionality, only in coding style. label Jan 27, 2025
@knoellle knoellle self-assigned this Jan 27, 2025
Copy link
Member

@schmidma schmidma left a comment

Choose a reason for hiding this comment

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

only a few comments, leaving the approval to @knoellle as he is assigned to this PR

crates/geometry/src/angle.rs Show resolved Hide resolved
crates/control/src/path_planner.rs Outdated Show resolved Hide resolved
crates/geometry/src/angle.rs Show resolved Hide resolved
@@ -105,11 +105,12 @@ impl StepPlanner {
Pose2::from_parts(line_segment.1, rotation)
}
PathSegment::Arc(arc) => {
let start_point = arc.start_point();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this variable can be inlined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variable is used twice below, and I don't want to recompute as it's relatively expensive

crates/control/src/path_planner.rs Outdated Show resolved Hide resolved
Comment on lines +89 to +90
pub fn start_point(&self) -> Point2<Frame> {
self.circle.point_at_angle(self.start)
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if currently not used, I think the existence of this function should imply the existence of an end_point function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -78,14 +78,14 @@ impl<Frame> LineSegment<Frame> {
self.signed_acute_angle_to_orthogonal(other).abs() < epsilon
}

pub fn projection_factor(&self, point: Point2<Frame>) -> f32 {
pub fn projection_factor(&self, point: &Point2<Frame>) -> f32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To save a copy here. filter only gives me a reference.

.filter(|intersection_point| {
(0.0..1.0).contains(&self.projection_factor(intersection_point))
})

@@ -7,7 +7,7 @@ pub type Rotation3<Frame, To, T = f32> = Transform<Frame, To, nalgebra::UnitQuat

impl<From, To, T> Rotation2<From, To, T>
where
T: SimdRealField + Copy,
T: SimdRealField,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, I need to put the T: Copy bound on every place where I use a rotation generic over T. SimdRealField implies Clone, so I replaced the only implicit copy by an explicit .clone() in the impl block

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:Refactoring No changes in functionality, only in coding style.
Projects
Status: Request for Review
Development

Successfully merging this pull request may close these issues.

3 participants