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

Bounding Box tool improvements #7892

Merged
merged 16 commits into from
Jul 17, 2024
Merged

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Jun 20, 2024

This PR keeps newly created bounding boxes via button interaction (and not created via drag) within the dataset bounding box to prevent initial invalid datasets (fixes #7353). Moreover, I added the option to move a bounding box around via ctrl +left mouse down.

URL of deployed dev instance (used for testing):

Steps to test:

  • Create an annotation of any dataset
  • In the dataset move to the edge of the dataset. Then go to the bounding box tab and create new bounding boxes. The bounding boxes should be kept within the dataset bounds. Click in the bbox coordinate input box. It should not be marked red as invalid.
  • Move completely out of the dataset and hide the create bbox button again. Now the bounding box should appear as normal, completely out of the bounds of the dataset. Clicking focusing the coordinate input of this bbox should mark it red.
  • Select the bounding box tool. Hold control and mouse drag to move a bounding box. Try this at different zoom levels. It should appear as expected. In very high zoom levels there might be minor differences between how much the mouse pointer and the bbox moves due to rounding issues. But I'd say that this is ok.

Edit: Nvm: the bbox coordinate is not marked red anymore as claimed by issue #7353. Maybe this was changed in the past.

TODOs:

  • regarding bbox moving: The current version is not that much in sync with the amount actually dragged as well as the alt button is already being used to move within the dataset. => The UX doesn't feel smooth. Maybe use a different button (ctrl / meta) and save the initial mouse down position to calculate the actual delta movement
    • Now, the ctrl / meta key is used. This makes this a little less confusing but using ctrl / meta to move the bounding box is not as intuitive as using alt. Else we could disable moving in the bbox tool via alt and use alt as a key to trigger the move bounding box.
  • Think about how to properly handle: In case the user creates a bounding box completely outside the dataset.
    • Just create the bounding box without restricting its bounds to the dataset and let the user cook.

Issues:


(Please delete unneeded items, merge only when none are left open)

@MichaelBuessemeyer
Copy link
Contributor Author

@daniel-wer @philippotto Who wants to review?

@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review July 3, 2024 07:45
@philippotto
Copy link
Member

Moreover, I added the option to move a bounding box around via alt +left mouse down.

On Ubuntu, alt + left mouse drag moves the current window. Could we use another modifier instead of alt?

@philippotto philippotto self-requested a review July 3, 2024 08:01
@MichaelBuessemeyer
Copy link
Contributor Author

On Ubuntu, alt + left mouse drag moves the current window. Could we use another modifier instead of alt?

Sorry the description was not up to date. I already changed the key to ctrl / meta as otherwise it might be bad UX when not close enough to a bbox and then unintentionally suddenly moving the dataset. See:

  • regarding bbox moving: The current version is not that much in sync with the amount actually dragged as well as the alt button is already being used to move within the dataset. => The UX doesn't feel smooth. Maybe use a different button (ctrl / meta) and save the initial mouse down position to calculate the actual delta movement
    • Now, the ctrl / meta key is used. This makes this a little less confusing but using ctrl / meta to move the bounding box is not as intuitive as using alt. Else we could disable moving in the bbox tool via alt and use alt as a key to trigger the move bounding box.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

cool stuff! left some comments. also: when holding ctrl or meta, it would be cool if the mouse icon would change to a "move" icon. also, the status bar should show that left drag will do a move when the modifier is pressed.

CHANGELOG.unreleased.md Outdated Show resolved Hide resolved
@MichaelBuessemeyer
Copy link
Contributor Author

when holding ctrl or meta, it would be cool if the mouse icon would change to a "move" icon.

Done, although a little quirky: When pressing ctrl & not moving the cursor, the appearance does not change, as the cursor change is done in "onMouseMove". Do yous think that's alright or should I invest more time?

also, the status bar should show that left drag will do a move when the modifier is pressed.

done :)

@philippotto
Copy link
Member

Cool!

Do yous think that's alright or should I invest more time?

i think, it's alright :)

package.json Outdated Show resolved Hide resolved
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

🎉

@MichaelBuessemeyer MichaelBuessemeyer enabled auto-merge (squash) July 17, 2024 07:52
@MichaelBuessemeyer MichaelBuessemeyer merged commit 93314fe into master Jul 17, 2024
2 checks passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the bbox-tool-improvements branch July 17, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default bounding box invalid on datasets if too close to beginning or end of dataset
3 participants