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

Fix #91 #111

Merged
merged 1 commit into from
Jan 29, 2022
Merged
Changes from all commits
Commits
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
68 changes: 43 additions & 25 deletions src/kernel/geometry/curves/circle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,31 +34,7 @@ impl Circle {
// and the circle. This is the same as the difference between
// the circumscribed circle and the incircle.
//
Copy link
Owner

Choose a reason for hiding this comment

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

That empty comment line served as a separator between paragraphs in the comment, and is no longer necessary.

Suggested change
//

// Let's figure which regular polygon we need to use, by just
// trying out some of them until we find one whose maximum error
// is less than or equal to the tolerance.
let mut n = 3;
loop {
let incircle_radius = radius * (PI / n as f64).cos();
let maximum_error = radius - incircle_radius;

if maximum_error <= tolerance {
break;
}

n += 1;

// If `n` is too large, due to an unreasonably low `tolerance`
// value, this loop might take a long time, and the program will
// just hang.
//
// Logging a warning or adding an `assert!` isn't really a solution,
// since we can only speculate what is reasonable for a specific use
// case.
//
// We can probably do away with this loop completely:
// https://github.com/hannobraun/Fornjot/issues/91
}
let n = self.number_of_vertices(tolerance, radius);

for i in 0..n {
let angle = 2. * PI / n as f64 * i as f64;
Expand All @@ -73,4 +49,46 @@ impl Circle {
out.push(point);
}
}

fn number_of_vertices(&self, tolerance: f64, radius: f64) -> i64 {
Copy link
Owner

@hannobraun hannobraun Jan 29, 2022

Choose a reason for hiding this comment

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

We'll never have a negative number of vertices, so using i64 (or any other i type) seems slightly inappropriate.

Suggested change
fn number_of_vertices(&self, tolerance: f64, radius: f64) -> i64 {
fn number_of_vertices(&self, tolerance: f64, radius: f64) -> u64 {

Applying this change by itself will cause a build error. I'll try to leave suggestions in the other locations too, but I can't guarantee I caught them all. (edit: I missed all the ones in the tests)

Copy link
Owner

Choose a reason for hiding this comment

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

This method doesn't access its &self argument, and I think it should be removed. It can then be accessed at the call site as Self::number_of_vertices, or in the tests as Circle::number_of_vertices.

Given the context of the call site, I think taking the radius here is the right choice. Taking &self in addition is unnecessary and confusing (see comment in test::verify_result).

An alternative would be to move it out of the impl block and make it a freestanding function. Doesn't make a difference really. I'd leave it where it is. It belongs to Circle, so having it in that namespace seems appropriate.

assert!(tolerance > 0.);
if tolerance > radius / 2. {
3
} else {
(PI / (1. - (tolerance / radius)).acos()).ceil() as i64
Copy link
Owner

Choose a reason for hiding this comment

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

See comment about i64 above.

Suggested change
(PI / (1. - (tolerance / radius)).acos()).ceil() as i64
(PI / (1. - (tolerance / radius)).acos()).ceil() as u64

}
}
}

#[cfg(test)]
mod tests {
use crate::math::Point;
use nalgebra::vector;
use std::f64::consts::PI;

use super::Circle;

#[test]
fn test_vertices_counting() {
verify_result(50., 100., 3);
verify_result(10., 100., 7);
verify_result(1., 100., 23);
}

fn calculate_error(radius: f64, n: i64) -> f64 {
radius - radius * (PI / n as f64).cos()
}

fn verify_result(tolerance: f64, radius: f64, n: i64) {
let circle = Circle {
center: Point::origin(),
radius: vector![100., 0., 0.],
Copy link
Owner

Choose a reason for hiding this comment

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

This is a bit confusing. It gave me pause during the review, thinking you had forgotten to set the radius correctly here, but it is correct. number_of_vertices ignores self and gets the radius as a parameter.

If the situation were different, I'd suggest adding a short comment to explain here. But in this case, removing the &self argument (and hence the need for a Circle here) is the better solution. See comment above.

};
assert_eq!(n, circle.number_of_vertices(tolerance, radius));

assert!(calculate_error(radius, n) <= tolerance);
if n > 3 {
assert!(calculate_error(radius, n - 1) >= tolerance);
}
}
}