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

Add keyboard shortcuts for scaling, allow loading files recursively #161

Merged
merged 6 commits into from
Apr 30, 2024

Conversation

adivardi
Copy link
Contributor

This PR adds keyboard shortcuts for increasing/decreasing bbox scale (length, width, height).
I extended the documentation to reflect this change.

It also includes a small change to load pointcloud files recursively.

docs/shortcuts.md Show resolved Hide resolved
labelCloud/control/bbox_controller.py Outdated Show resolved Hide resolved
labelCloud/control/bbox_controller.py Show resolved Hide resolved
@ch-sa ch-sa added the feature New feature or request label Apr 22, 2024
@ch-sa
Copy link
Owner

ch-sa commented Apr 22, 2024

Hi @adivardi,

thanks for suggesting the PR.

You propose a lot of changes in bulk. The change about adding the scaling shortcuts look good to me. Also replacing the , and . shortcuts is fine with me.

I am only more conservative about breaking the other shortcuts since people might got used to it.
I suggest you split this PR into adding the scaling and recursive point cloud lookup, which we can quickly merge and the remaining changes, which we can then discuss 1 by 1 in a second PR.

For the remaining changes, I would like to end up with a solution that does not break QWERTZ keyboard layouts.
They acknowledge the issue here for PyQt but seem to not offer a solution (besides standard keys).

@adivardi
Copy link
Contributor Author

@ch-sa
Thanks for the super quick review. I renamed the var as requested and left one comment.

@ch-sa
Copy link
Owner

ch-sa commented Apr 24, 2024

  • black formatting
  • check tests

@adivardi
Copy link
Contributor Author

@ch-sa
It seems the black version used in CI is the latest (24.4.1), and is different than the one in the requirements file.
I updated the file and ran black on all files, I hope the tests would pass now.

@adivardi
Copy link
Contributor Author

adivardi commented Apr 29, 2024

@ch-sa
I fixed the mypy warnings.

Just a suggestion, wouldn't it be easier to run the workflow before the approval?

@adivardi adivardi requested a review from ch-sa April 29, 2024 06:22
@ch-sa
Copy link
Owner

ch-sa commented Apr 29, 2024

Yes, but I manually need to approve pipelines to run on every change ...

@adivardi
Copy link
Contributor Author

adivardi commented Apr 29, 2024

Finally all tests passed 😄

@ch-sa ch-sa merged commit 0e49ce1 into ch-sa:master Apr 30, 2024
4 checks passed
@ch-sa
Copy link
Owner

ch-sa commented Apr 30, 2024

Merged :)

@adivardi adivardi deleted the av/scaling branch May 1, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants