Fix #91 #111
Fix #91 #111
Conversation
…rtices needed to approximate a circle
There was a problem hiding this comment.
Thank you, @gzsombor, this is awesome! I'm really enjoying getting rid of that crummy loop 😄
This looks great. I'm going to merge it. Especially good job on the testing!
I've left a bunch of comments, but all of those are just nitpicking on a high level. Definitely none of them are a blocker, and I'm going to merge the pull request right away. If you want to, feel free to address those in a follow-up PR. (Or if anyone else happens to come by, wanting to help out, that goes for you too.)
| @@ -34,31 +34,7 @@ impl Circle { | |||
| // and the circle. This is the same as the difference between | |||
| // the circumscribed circle and the incircle. | |||
| // | |||
There was a problem hiding this comment.
That empty comment line served as a separator between paragraphs in the comment, and is no longer necessary.
| // |
| @@ -73,4 +49,46 @@ impl Circle { | |||
| out.push(point); | |||
| } | |||
| } | |||
|
|
|||
| fn number_of_vertices(&self, tolerance: f64, radius: f64) -> i64 { | |||
There was a problem hiding this comment.
We'll never have a negative number of vertices, so using i64 (or any other i type) seems slightly inappropriate.
| 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)
| if tolerance > radius / 2. { | ||
| 3 | ||
| } else { | ||
| (PI / (1. - (tolerance / radius)).acos()).ceil() as i64 |
There was a problem hiding this comment.
See comment about i64 above.
| (PI / (1. - (tolerance / radius)).acos()).ceil() as i64 | |
| (PI / (1. - (tolerance / radius)).acos()).ceil() as u64 |
| @@ -73,4 +49,46 @@ impl Circle { | |||
| out.push(point); | |||
| } | |||
| } | |||
|
|
|||
| fn number_of_vertices(&self, tolerance: f64, radius: f64) -> i64 { | |||
There was a problem hiding this comment.
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.
| fn verify_result(tolerance: f64, radius: f64, n: i64) { | ||
| let circle = Circle { | ||
| center: Point::origin(), | ||
| radius: vector![100., 0., 0.], |
There was a problem hiding this comment.
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.
|
Thanks for the comments, yes, you're right, the self is not needed, and u32 is better type for counting things. |
with an exact formula to calculate the number of vertices needed to approximate a circle