Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polygon Selection #261

Merged
merged 3 commits into from
Nov 6, 2024
Merged

Polygon Selection #261

merged 3 commits into from
Nov 6, 2024

Conversation

simonbethke
Copy link
Contributor

I just added a little Polygon selection tool to the toolbar.

polyselection.mp4

@simonbethke
Copy link
Contributor Author

simonbethke commented Nov 6, 2024

Important things to test for QA are:

  • Close with double click and by clicking the start
  • When finishing the selection by clicking the start again the dashed line gets brighter (the exact position of the last click is not used as poly-point)
  • When finishing with double-click, the position of the double-click is used as last poly-point
  • Change tool with unfinished polygon and get back to this tool (the line should start from the beginning)
  • Changing the view is locked as long as I am editing a polygon but works with the tool selected as long as no poly is made
  • Regression Testing:
    • Adding (+Ctrl) and Removing (+Shift) the selection from other selections should work too
    • A selection can be canceled using the ESC key
  • The tool can be accessed with shortcut key "O". (I wish I could have used "P", but thats already the word "Picker" that is used as synonym for "Rectangle".... I don't know why)

@simonbethke
Copy link
Contributor Author

simonbethke commented Nov 6, 2024

Question:

  • If when closing the last point is not used as selection-point, should the dashed line also snap to that start point during edit?

Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing. I just tested this and it works great! Thanks so much!

A few notes:

  • The double-click to close is a great addition. Wondering how we can let users know about it.
  • 4px to close felt to me a little small. I'd make it bigger... maybe 8px?
  • I'd quite like backspace to delete the previously placed point (I can can do this later).
  • The mask canvas element should be shared with brush tool (I will do this later).
  • Localisation of the other languages is missing - I will add that once this PR is merged.
  • We should put polygon tool on keyboard 'p' shortcut. I can do this once PR is merged.

Thanks again!

Copy link
Contributor

@willeastcott willeastcott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this wins 'PR of the week' competition. 🎉

@willeastcott
Copy link
Contributor

I note this partially addresses #76. That issue states that polygon selection is actually more important than lasso selection. But we should probably leave it open assuming we also intend to add a lasso select.

@simonbethke
Copy link
Contributor Author

simonbethke commented Nov 6, 2024

Actually photoshop adds even more magic:
When holding Shift, you get the last part of that line snapped to 45° angles. However, this comes with the price that in photoshop for adding this selection to the already existing one, you have to hold Shift when starting the selection, not when completing it.
But they also change the cursor to one with + / - for the boolean operations.

I could even think of some original magic that I do not know from any other application. That would be dragging a poly-point could bend the last part of the line. Similar to bezier splines.

@slimbuck slimbuck merged commit 32b20cd into playcanvas:main Nov 6, 2024
2 checks passed
@slimbuck slimbuck mentioned this pull request Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants