Support arcs in sketches #1484
Support arcs in sketches #1484
Conversation
crates/fj-operations/src/sketch.rs
Outdated
| let arc_circle_data = fj_math::ArcCircleData::from_endpoints_and_angle(start_point, segment.endpoint, fj_math::Scalar::from_f64(angle.rad())); | ||
| for circle_minmax_angle in [0., PI/2., PI, 3.*PI/2.] { | ||
| let mm_angle = fj_math::Scalar::from_f64(circle_minmax_angle); | ||
| if arc_circle_data.start_angle < mm_angle && mm_angle < arc_circle_data.end_angle { | ||
| points.push(arc_circle_data.center + [arc_circle_data.radius * circle_minmax_angle.cos(), arc_circle_data.radius * circle_minmax_angle.sin()]); | ||
| } | ||
| } |
There was a problem hiding this comment.
believe it or not, this is cargo fmt'd code 😅
There was a problem hiding this comment.
Ha, looks like the shorter variable name helped rustfmt sort itself out 😄
antonok-edm
force-pushed
the
sketch-arcs
branch
from
c27a339
to
186a0e1
Compare
|
I also didn't touch sketched |
There was a problem hiding this comment.
Thank you, @antonok-edm, this is absolutely awesome!
I've left some comments with things that I noticed. Only one of those needs to be addressed (the potential bug in the bounding bugs calculation). Everything else is 100% optional and can be addressed in a follow-up pull request, or not at all.
There are a few problems with model validation in some cases. I found that in particular, adding arcs to the 5-pointed star will cause a lot of errors. But from general testing on simple shapes, I've found that the arcs are quite reliable.
That's perfectly fine. My only concern are regressions to existing functionality. I'm perfectly happy to merge not-quite-reliable new features 😄
My main concern is the user experience, as it's quite easy to cause the app to crash by changing the angle value in the test model. Eventually, I'd like to be able to mark features as experimental, requiring users to opt into them explicitly (#431).
But until we have that ability, I'd rather merge new features and gain the benefits from them immediately. Keeping pull requests open long-term until they are "perfect" is not a good solution, in my opinion.
I'm happy to reorganize things here as necessary - I got it "working" but I didn't spend too long thinking about how/where to define things like
ArcCircleData.
I've left some comments with my thoughts, but as I said above, all of that is optional. More improvements are of course very welcome, but those can happen in follow-up PRs. If it weren't for the bounding box issue, I'd merge this immediately.
I also might not be using the most idiomatic patterns in some places.
I was actually quite impressed by that builder code you added! I think a month from now, I won't be able to tell which parts of that builder I wrote 😄
As a general note though, what's considered idiomatic in the context of Fornjot is in flux, as I constantly discover better patterns. Nothing is set in stone, so in case you have any feedback, I'd love to hear it!
I also didn't touch sketched
Circles here but it'd be pretty simple afterwards to represent them internally as a polychain with a single arc edge.
Also fine! These kinds of cleanups don't need to hold up this pull request.
Okay, so the next step to move this forward would be to address the bounding box calculation, which I think should be rather simple.
If you want to address any of my other comments at the same time (or have ideas of your own), feel free to do that. But we don't need to keep this PR open longer than necessary. Happy to merge this now, and make further improvements whenever is convenient.
| let path = curve | ||
| .read() | ||
| .path | ||
| .expect("Expected path that was just created"); |
There was a problem hiding this comment.
General idea, not directly related to this pull request: Maybe these curve update methods should return the path they create. That would follow the conventions of other builder methods, and it would make code like this a bit nicer.
crates/fj-operations/src/sketch.rs
Outdated
| let arc_circle_data = fj_math::ArcCircleData::from_endpoints_and_angle(start_point, segment.endpoint, fj_math::Scalar::from_f64(angle.rad())); | ||
| for circle_minmax_angle in [0., PI/2., PI, 3.*PI/2.] { | ||
| let mm_angle = fj_math::Scalar::from_f64(circle_minmax_angle); | ||
| if arc_circle_data.start_angle < mm_angle && mm_angle < arc_circle_data.end_angle { | ||
| points.push(arc_circle_data.center + [arc_circle_data.radius * circle_minmax_angle.cos(), arc_circle_data.radius * circle_minmax_angle.sin()]); | ||
| } | ||
| } |
antonok-edm
force-pushed
the
sketch-arcs
branch
from
186a0e1
to
3685c6f
Compare
antonok-edm
force-pushed
the
sketch-arcs
branch
from
680f9ae
to
5d38e15
Compare
|
@hannobraun I addressed most of your feedback here but I'm now stuck with the missing corner issue. I haven't been able to find anything wrong with the bounding box logic and I'm not even able to reproduce that issue myself, which is making it quite difficult to track down. Are you able to reproduce it consistently? Is it definitely caused by this PR? |
There was a problem hiding this comment.
Thank you for the follow-up, @antonok-edm!
As I said in my reply to your comment, that bounding box bug is actually pre-existing and I was misreading your code. Sorry for that!
This is good to go now. Merging!
Thanks again. It's great to have this new CAD modeling capability!
closes #100
screenshot of the
testmodel as modified from this branchThere are a few problems with model validation in some cases. I found that in particular, adding arcs to the 5-pointed star will cause a lot of errors. But from general testing on simple shapes, I've found that the arcs are quite reliable.
I'm happy to reorganize things here as necessary - I got it "working" but I didn't spend too long thinking about how/where to define things like
ArcCircleData. I also might not be using the most idiomatic patterns in some places.