Circle #3860
Circle #3860
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Irev-Dev
force-pushed
the
kurt-circle-rebase
branch
from
4f5731c
to
5b31a6b
Compare
Irev-Dev
changed the base branch from
kurt-refactor-client-side-scene
to
main
Irev-Dev
force-pushed
the
kurt-circle-rebase
branch
from
39095be
to
2a233d8
Compare
|
Doc comments that use |
|
The error message I see a lot of is modeling-app/src/wasm-lib/kcl/src/std/convert.rs Lines 52 to 58 in db6036f When extruding a circle, |
Irev-Dev
force-pushed
the
kurt-circle-rebase
branch
from
db6036f
to
94952a1
Compare
Irev-Dev
force-pushed
the
kurt-circle-rebase
branch
from
94952a1
to
2a468c9
Compare
|
That is super helpful @jtran thank you. I think it's because https://github.com/KittyCAD/modeling-app/pull/3860/files#diff-b007215ec7d21c68d6a4487af28c6a25fa8cf5fc95ae008e07e5680261cab1f1L73-L80 circle used to call |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3860 +/- ##
==========================================
+ Coverage 87.15% 87.16% +0.01%
==========================================
Files 69 69
Lines 25121 25176 +55
==========================================
+ Hits 21894 21945 +51
- Misses 3227 3231 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
This reverts commit 505bb20.
|
|
|
|
|
|
1 similar comment
|
|
| @@ -1438,6 +1438,19 @@ pub enum Path { | |||
| /// arc's direction | |||
| ccw: bool, | |||
| }, | |||
| /// a complete arc | |||
| Circle { | |||
There was a problem hiding this comment.
Sorry if this is obvious, but, why is Circle a kind of path segment? How can you add more path segments before or after a circle? It doesn't make sense to me to have a circle as part of a sequence of paths. Shouldn't it just be a shortcut for creating a path with 1 segment, a TangentialArc from 0 to 360 degrees?
There was a problem hiding this comment.
inner_arc is very messed up because some of the arc structs are messed up, it returns a path segment using ToPoint, I didn't want to change how the arc segments work in case that's a breaking change. I know if the other arcs are not working correctly I'm just leaving the dirt under the rug and not doing anything about it, but I plan on taking a second pass where I add arcs to the client side scene in which case the arcs structs in rust will probably need some attention too, in which case I'll likely remove this struct again.
I was talking to @jtran about it too, I'll likely remove all but one of the arcs, I don't think there's a need for tangencialArc and arc for example, I think we got confused between the API for users and the functions available to them vs the data we store under-the-hood, I think we only need a struct for each curve type, so straight lines and arcs at this point, but later splines etc etc.
Do you think it's okay to put this circle struct in here short term?
There was a problem hiding this comment.
Yeah in the interests of getting this merged I'm happy to include it, but it doesn't really make sense. Let's consolidate this when you fix up the arcs though. Could you just make an issue for it, and leave the issue in the source code here as a comment? So we don't lose track of it.
There was a problem hiding this comment.
Sounds good 414b11d
|
|
| canvas.addEventListener('mouseup', sceneInfra.onMouseUp, false) | ||
| canvas.addEventListener( | ||
| 'mouseup', | ||
| toSync(sceneInfra.onMouseUp, reportRejection), |
There was a problem hiding this comment.
I think this is the appropriate way to make us toSync right?
I had to change the return types of these callbacks to be promisy/async, but these went listeners need to be synchronous.
|
|
|
|
Remaining tasks
Expected [..] to be a sketchGroup, but it wasn't.Tests