Layer snapping #386

0HyperCube · 2021-10-25T12:07:16Z

Closes #200

Display lines (probably wait for overlay system?)
Don't search for every layer each mouse move
Snap new shapes while they're being created
Hook up the snap (🧲) icon to entirely enable/disable snapping behavior (on by default)

This change is

cloudflare-workers-and-pages · 2021-10-25T12:12:20Z

Deploying with Cloudflare Pages

Latest commit:	`8a28a78`
Status:	✅ Deploy successful!
Preview URL:	https://07ef8fc4.graphite-master.pages.dev

View logs

Keavon · 2021-10-25T16:43:34Z

Wow, this is already working very nicely! I think a smaller snap value would be good though, it only needs a few pixels. I also added one item to your todo list above: snapping being-created shapes.

Keavon · 2021-11-15T00:17:26Z

This is looking awesome! Please reduce SNAP_TOLERANCE from 5 to 3 which aligns better with user expectations and, especially while we have no snapping visualizations yet, avoids user confusion about why the snapping behavior is often being triggered so generously for random things.

I also added a task to the list in this main post:

Hook up the snap (🧲) icon to entirely enable/disable snapping behavior (on by default)

If you'd prefer that I do that, let me know and I can make a commit to do that in the next few days.

Lastly, we'll need to eventually support snapping while creating lines with the Line Tool and points with the Pen Tool. If you'd prefer to merge first and address that in a later PR, that would be fine— up to you. Perhaps just the Line Tool, for now, might be easy to do first.

Is there anything else that you know needs to be done before we begin code review and prepare to merge this? Thanks again for continuing to work on this during the past month of slow progress. Things will be picking up again in the next few weeks when I regain free time!

0HyperCube · 2021-11-15T19:43:34Z

Please reduce SNAP_TOLERANCE from 5 to 3

Done

Hook up the snap (magnet) icon to entirely enable/disable snapping behavior (on by default)

Done

Lastly, we'll need to eventually support snapping while creating lines with the Line Tool and points with the Pen Tool. If you'd prefer to merge first and address that in a later PR, that would be fine— up to you. Perhaps just the Line Tool, for now, might be easy to do first.

Done

Is there anything else that you know needs to be done before we begin code review and prepare to merge this? Thanks again for continuing to work on this during the past month of slow progress. Things will be picking up again in the next few weeks when I regain free time!

Thanks. I've marked it ready for review.

frontend/wasm/src/api.rs

editor/src/document/document_file.rs

editor/src/document/snapping.rs

editor/src/tool/tools/resize.rs

editor/src/tool/tools/select.rs

editor/src/consts.rs

Keavon · 2021-11-20T00:26:20Z

Looks great! Thanks for humoring my pedantry @0HyperCube in that round of reviews. We should be good to merge but perhaps, since the weekend is now here, we can wait to see if @TrueDoctor has the chance to take a look with his superior Rust experience. But feel free to merge anytime.

TrueDoctor · 2021-11-23T15:29:04Z

I am not really a fan of passing a mutable document to the tools, there should be relatively easy way to avoid having to do that. If I am missing crucial benefits of using a &mut please correct me

0HyperCube · 2021-11-23T16:58:27Z

I am not really a fan of passing a mutable document to the tools, there should be relatively easy way to avoid having to do that. If I am missing crucial benefits of using a &mut please correct me

@TrueDoctor It is needed to cache the snap targets to avoid walking through the layer tree each frame. I am unsure where to put the cache without having pass a mutable reference to the document message handler to the tools.

TrueDoctor · 2021-11-23T17:23:36Z

@0HyperCube Could you put the cache into the tool storage?
We will eventually try to execute different steps of the pipeline in parallel and for that we don't want different steps to depend on a mutable reference at the same time

0HyperCube · 2021-11-23T21:35:17Z

@TrueDoctor Sure, done.

TrueDoctor · 2021-11-23T21:36:55Z

Nice this looks promising, will take a final look and then you should be good to go

TrueDoctor · 2021-11-23T21:53:08Z

editor/src/tool/tools/resize.rs

+			if ipp.keyboard.get(lock_ratio as usize) {
+				size = size.abs().max(size.abs().yx()) * size.signum();
+			}
+			if ipp.keyboard.get(center as usize) {


Not at all your pr, just something I noticed:
Should we add a ipp.keyboard.is_presseed(key: Key) method?

TrueDoctor

Looks good to me, great work

Keavon · 2021-11-23T22:12:20Z

@0HyperCube Could you put the cache into the tool storage? We will eventually try to execute different steps of the pipeline in parallel and for that we don't want different steps to depend on a mutable reference at the same time

I am not sure if the tool storage is the best spot for it, since caching is a global system, not specific to one tool. The Select Tool is certainly the most obvious, but also the Path Tool will use it for snapping vertices, the G (grab) action will use it, and plenty others I'm not thinking of now. We could save that for a future refactor though, I'm just commenting this to mention it. Thoughts? @TrueDoctor @0HyperCube

TrueDoctor · 2021-11-23T22:20:29Z

The main thing is that caching should not take place in the document itself and this is a fine solution for now.
having this in the tool is also kinda nice because the cache is unlikely to get invalidated during the usage of a tool which would not be the case for a global cache. @Keavon

0HyperCube · 2021-11-23T22:22:03Z

@Keavon The path tool snapping is already implemented with a separate cache in the above refactor. The cache is cleared after the tool is finished (i.e. when your line is created or when your grab is released) - this is because most of the snapping operations mutate the new snap targets. If the cache is empty then there are just 56 bytes of stack (size_of::<Option<SnapHandler>>())and no bytes of heap used.

Keavon · 2021-11-23T23:35:15Z

Sounds good. I'm happy to merge now. Do you want to have the honor of pushing the button?

0HyperCube and others added 2 commits October 25, 2021 13:04

Test snapping

fac73f2

Merge branch 'master' into snapping

aef2b4c

Keavon added the hacktoberfest-accepted label Nov 1, 2021

0HyperCube and others added 4 commits November 9, 2021 20:21

Snap new shapes

635fcfb

Merge branch 'master' into snapping

c0b04df

Fix snapping when zoomed

455dc59

Refactor to use viewport bounds

b294ee0

0HyperCube added 3 commits November 15, 2021 16:50

Reduce snap tolerance to 3

7c5f6ef

Snap line and path tool

159aa4f

Add disable snapping and refactor

4c28667

0HyperCube marked this pull request as ready for review November 15, 2021 19:43

Keavon requested review from Keavon and TrueDoctor and removed request for TrueDoctor November 18, 2021 22:22

Keavon reviewed Nov 19, 2021

View reviewed changes

0HyperCube added 4 commits November 19, 2021 16:53

new_snap -> new status

d5b9a92

Rearrange import

137ceb3

Cleanup

f0fe09c

Fix incorrect variable name

61efabb

Keavon approved these changes Nov 20, 2021

View reviewed changes

Store snap data in tool data

8a28a78

TrueDoctor reviewed Nov 23, 2021

View reviewed changes

TrueDoctor approved these changes Nov 23, 2021

View reviewed changes

0HyperCube merged commit bf1706b into master Nov 24, 2021

0HyperCube deleted the snapping branch November 24, 2021 16:50

Uh oh!

Layer snapping #386

Layer snapping #386

Uh oh!

Conversation

0HyperCube commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudflare-workers-and-pages bot commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

Keavon commented Oct 25, 2021

Uh oh!

Keavon commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0HyperCube commented Nov 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Keavon commented Nov 20, 2021

Uh oh!

TrueDoctor commented Nov 23, 2021

Uh oh!

0HyperCube commented Nov 23, 2021

Uh oh!

TrueDoctor commented Nov 23, 2021

Uh oh!

0HyperCube commented Nov 23, 2021

Uh oh!

TrueDoctor commented Nov 23, 2021

Uh oh!

TrueDoctor Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

TrueDoctor left a comment

Choose a reason for hiding this comment

Uh oh!

Keavon commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TrueDoctor commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0HyperCube commented Nov 23, 2021

Uh oh!

Keavon commented Nov 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0HyperCube commented Oct 25, 2021 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 25, 2021 •

edited

Loading

Keavon commented Nov 15, 2021 •

edited

Loading

Keavon commented Nov 23, 2021 •

edited

Loading

TrueDoctor commented Nov 23, 2021 •

edited

Loading