The logic for interacting with 3D annotation layers has been updated#10299
The logic for interacting with 3D annotation layers has been updated#10299filichabout wants to merge 6 commits intodevelopfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #10299 +/- ##
===========================================
- Coverage 74.71% 73.81% -0.90%
===========================================
Files 431 432 +1
Lines 46654 46705 +51
Branches 4220 4230 +10
===========================================
- Hits 34858 34477 -381
- Misses 11796 12228 +432
🚀 New features to boost your workflow:
|
| private focusAndExpand = (): void => { | ||
| const { | ||
| objectState, canvasInstance, focusedObjectPadding, expandObject, | ||
| objectState, canvasInstance, focusedObjectPadding, |
There was a problem hiding this comment.
Expand behaviour was previously added in this pull request #10172 to improve consensus UX.
On the one hand, I believe it is necessary in review mode, and maybe not necessary in other modes. On the other hand, I do not see reasons to have different behaviour for different workspaces.
So, currently let's revert this change.
|
|
||
| if (canvasInstance instanceof Canvas && objectState.objectType !== ObjectType.TAG) { | ||
| canvasInstance.focus(objectState.clientID as number, focusedObjectPadding); | ||
| } else if (canvasInstance instanceof Canvas3d && objectState.objectType !== ObjectType.TAG) { |
There was a problem hiding this comment.
objectState.objectType !== ObjectType.TAG repeated twice, thus it may be rewritten as:
if (objectState.objectType !== ..) {
if (isCanvas2D) { ... }
else if (isCanvas3D) { ... }
}
| MOVE_RIGHT: () => { }, | ||
| ZOOM_IN: () => { }, | ||
| ZOOM_OUT: () => { }, | ||
| NEXT_OBJECT: (event: KeyboardEvent | undefined) => { |
There was a problem hiding this comment.
| NEXT_OBJECT: (event: KeyboardEvent | undefined) => { | |
| NEXT_OBJECT: (event?: KeyboardEvent) => { |
| preventDefault(event); | ||
| navigateObject(1); | ||
| }, | ||
| PREVIOUS_OBJECT: (event: KeyboardEvent | undefined) => { |
There was a problem hiding this comment.
| PREVIOUS_OBJECT: (event: KeyboardEvent | undefined) => { | |
| PREVIOUS_OBJECT: (event?: KeyboardEvent) => { |
| public focusObject(clientID: number, animate: boolean = true): void { | ||
| this.view.focusObjectByClientId(clientID, animate); | ||
| } |
There was a problem hiding this comment.
May we keep the interface of 2D and 3D canvas similar?
I mean name this method just focus and remove the second parameter animate as in current implementation is always true and seems unnecessary
| frameData, | ||
| contextMenuVisibility, | ||
| annotations, | ||
| annotations: annotations as ObjectState[], |
There was a problem hiding this comment.
Would not it be better to fix the typing for the state instead?
| keyMap: state.shortcuts.keyMap, | ||
| normalizedKeyMap: state.shortcuts.normalizedKeyMap, | ||
| annotations: state.annotation.annotations.states as ObjectState[], | ||
| activatedStateID: state.annotation.annotations.activatedStateID as number | null, |
There was a problem hiding this comment.
is already number | null. Type case is useless
| normalizedKeyMap: state.shortcuts.normalizedKeyMap, | ||
| annotations: state.annotation.annotations.states as ObjectState[], | ||
| activatedStateID: state.annotation.annotations.activatedStateID as number | null, | ||
| curZLayer: state.annotation.annotations.zLayer.cur as number, |
| canvas.keyControls(new KeyboardEvent('keydown', { code, altKey, shiftKey })); | ||
| }; | ||
|
|
||
| const navigateObject = (step: number): void => { |
There was a problem hiding this comment.
The implementation seems very similar with 2D workspace, do you see any ways to reduce code duplication?
| const objectState = annotations.find((state) => state.clientID === clientID); | ||
|
|
||
| if (objectState) { | ||
| const sidebarItem = window.document.getElementById( | ||
| `cvat-objects-sidebar-state-item-${clientID}`, | ||
| ); | ||
|
|
||
| if (sidebarItem) { | ||
| sidebarItem.scrollIntoView(); | ||
| } | ||
| } |
There was a problem hiding this comment.
It should not be here. When cursor just hovered, view should not be scrolled
| onSplitAnnotations(state); | ||
| }; | ||
|
|
||
| const onCanvasDoubleClicked = (event: CustomEvent<{ clientID: number | null }>): void => { |
There was a problem hiding this comment.
Let's implement the same logic as in onCanvasShapeClicked of 2D canvas when shape just clicked (not doubleclicked, so, event name should be canvas.clicked, to keep naming consistent.
6624ad5 to
ddd20fe
Compare
cvat-ui/src/utils/objects-sidebar.ts
Outdated
| @@ -0,0 +1,42 @@ | |||
| import { ObjectState } from 'cvat-core-wrapper'; | |||
|
|
|||
| export function getSidebarItemId(state: ObjectState): string { | |||
There was a problem hiding this comment.
Used only internally, not necessary to be exported
cvat-ui/src/utils/objects-sidebar.ts
Outdated
| return `cvat-objects-sidebar-state-item-${clientID}`; | ||
| } | ||
|
|
||
| export function scrollSidebarItemIntoViewById(id: string): void { |
There was a problem hiding this comment.
Used only internally, not necessary to be exported
cvat-ui/src/utils/objects-sidebar.ts
Outdated
| expandObject(state); | ||
| } | ||
|
|
||
| export function scrollAndExpandByClientID( |
There was a problem hiding this comment.
The wrapper seems unnecessary as the find logic maybe implemented in calling function (and there is only one place it called from)
3d69d02 to
1ff9c4f
Compare
1ff9c4f to
6e9f33a
Compare


Motivation and context
Consistent behaviour for Tab/Shift+Tab on 2D and 3D workspace
Checklist
developbranchI have updated the documentation accordinglyI have added tests to cover my changesI have linked related issuesLicense
Feel free to contact the maintainers if that's a concern.