Canvas: Add snapping to vertex edit #84417
Conversation
drew08t
changed the title
drew08t
changed the title
drew08t
requested review from
nmarrs,
baldm0mma and
Develer
and removed request for
a team
| const angleOverall = calculateAngle(vx1, vy1, vx2, vy2); | ||
| const angleBefore = calculateAngle(vx1, vy1, x, y); | ||
| deleteVertex = Math.abs(angleBefore - angleOverall) < CONNECTION_VERTEX_SNAP_TOLERANCE; | ||
| } |
There was a problem hiding this comment.
Nit but this logic might be good to clean up and put into a separate func
| @@ -275,6 +281,8 @@ export class Connections { | |||
|
|
|||
| // Handles mousemove and mouseup events when dragging an existing vertex | |||
| vertexListener = (event: MouseEvent) => { | |||
There was a problem hiding this comment.
Can be done later after G11 cutoff but we might want to consider breaking out vertex logic into separate file maybe 🤔
|
|
||
| if (deleteVertex) { | ||
| // Display temporary vertex removal | ||
| this.scene.selecto!.rootContainer!.style.cursor = 'not-allowed'; |
There was a problem hiding this comment.
is the cursor style change there to indicate that the vertexes are being removed? I'm not sure this is necessary (i.e. end user probably doesn't care if vertexes are being removed and just care about ending line) / might be confusing to end user as it seems like they are doing an action that isn't allowed or something 🤔
Screen.Recording.2024-03-18.at.9.02.12.PM.mov
There was a problem hiding this comment.
I want to agree here, feels like this cursor doesn't quite match up with the outcome expected. Maybe would be better to have a treatment for the line or something like that?
There was a problem hiding this comment.
Agreed, appreciate having more eyes on this!
There was a problem hiding this comment.
It's subtle, but by simply hiding the vertex during "deletion snap", I think is good enough for now.
|
LGTM, great that we have snapping. 👍 |
| @@ -18,6 +19,11 @@ import { | |||
| import { CONNECTION_ANCHOR_ALT, ConnectionAnchors, CONNECTION_ANCHOR_HIGHLIGHT_OFFSET } from './ConnectionAnchors'; | |||
| import { ConnectionSVG } from './ConnectionSVG'; | |||
|
|
|||
| export const CONNECTION_VERTEX_ID = 'vertex'; | |||
| export const CONNECTION_VERTEX_ADD_ID = 'vertexAdd'; | |||
| const CONNECTION_VERTEX_ORTHO_TOLERANCE = 0.05; // Cartesian ratio against vertical or horizontal tolerance | |||
There was a problem hiding this comment.
Lovely use of constants ❤️
|
|
||
| if (deleteVertex) { | ||
| // Display temporary vertex removal | ||
| this.scene.selecto!.rootContainer!.style.cursor = 'not-allowed'; |
There was a problem hiding this comment.
I want to agree here, feels like this cursor doesn't quite match up with the outcome expected. Maybe would be better to have a treatment for the line or something like that?
Add snapping to vertex edit of connections in canvas.
Disable snapping using control key:
Snapping during vertex creation:
Horizontal and vertical snap during vertex edit:
Deleting a vertex:
Closes #83269