Repository navigation
Conversation
|
@0HyperCube Hey, could you please take a look at this PR when you get time? |
|
@Keavon PTAL. |
@shubhtrek this is perhaps not especially helpful. Keavon has a large review backlog.
Have you considered adding tests? Tests are vital when maintaining large pieces of software. Thanks. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@0HyperCube Hey! Added a unit test |
0HyperCube
left a comment
0HyperCube
left a comment
There was a problem hiding this comment.
Thank you for adding the test. It seems to be written reasonably, I have just a couple of nits regarding adding some utility functions that could be shared with other tests.
Perhaps also an additional test to show that clicking when there is just one segment connected uses the handle (as shown) could really solidify the desired behaviour?
using_handle_when_one_connected.mp4
| let b_id = vector | ||
| .point_domain | ||
| .ids() | ||
| .iter() | ||
| .copied() | ||
| .find(|&id| { | ||
| let pos = vector.point_domain.position_from_id(id).unwrap(); | ||
| layer_to_viewport.transform_point2(pos).distance(B) < 1e-6 | ||
| }) | ||
| .expect("Anchor B should exist"); |
There was a problem hiding this comment.
Consider creating a helper function e.g. get_anchor_near.
| let bd_segment = vector | ||
| .segment_domain | ||
| .ids() | ||
| .iter() | ||
| .copied() | ||
| .find(|&segment| { | ||
| let start = vector.segment_start_from_id(segment); | ||
| let end = vector.segment_end_from_id(segment); | ||
| (start == Some(b_id) && end == Some(d_id)) || (start == Some(d_id) && end == Some(b_id)) | ||
| }) | ||
| .expect("Segment between B and D should exist"); |
There was a problem hiding this comment.
Consider making this also a function.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
4e4aee1 to
f79bb57
Compare
… points connected to multiple segments
…egment colinear handle test Address review nits from 0HyperCube: - Extract repeated point-lookup into a reusable get_anchor_near helper - Extract repeated segment-lookup into a reusable get_segment_between helper - Add test verifying single-segment anchors preserve colinear handles
f79bb57 to
cf57806
Compare
cf57806 to
69d3b46
Compare
|
@0HyperCube PTAL |
This branch has not been deployed
2 participants
Closes #4618
Description
When using the Pen tool to extend a path from a point connected to multiple segments, the editor was mirroring the existing handle and making it colinear with the new handle. This was causing weird handle behaviour when editing the path.
Now, handle mirroring and colinearity are applied only when extending from an endpoint connected to one or zero segments. If the point is already connected to multiple segments, the new handle stays at the anchor point until we drag it.