CVAT-3D - Part1 #3738
CVAT-3D - Part1 #3738
Conversation
…e,shape selection and cuboid draw issue fixes
|
@manasars , could you please merge the latest develop branch? It has some fixes to make the tests more stable. After that the PR should be green. |
Hi @nmanovic : I merged the latest develop code , but now I see below error in tests: Looks like its the same behaviour in other PRs as well. |
| resetZoom: boolean; | ||
| canvasBackgroundColor: string; |
There was a problem hiding this comment.
I understand how opacity, outlined, colorBy relates to shapes. But I don't really understand why resetZoom and canvasBackgroundColor are shape properties.
| } | ||
|
|
||
| export enum UpdateReasons { | ||
| IMAGE_CHANGED = 'image_changed', | ||
| OBJECTS_UPDATED = 'objects_updated', | ||
| UPDATE_CANVAS_BACKGROUND = 'update_canvas_background', |
There was a problem hiding this comment.
I do not see any notifications with UPDATE_CANVAS_BACKGROUND. How is it used?
| this.data.drawData.enabled = false; | ||
| this.data.mode = Mode.IDLE; | ||
| this.cancel(); | ||
| this.data.shapeProperties = { | ||
| ...shapeProperties, | ||
| }; | ||
| this.notify(UpdateReasons.OBJECTS_UPDATED); | ||
| if (opacity !== shapeProperties.opacity || selectedOpacity !== shapeProperties.selectedOpacity) { | ||
| this.notify(UpdateReasons.UPDATE_OPACITY); |
There was a problem hiding this comment.
Why do we need separated notify for opacity?
What if in one canvas request we push opacity, selectedOpacity for example with outlineColor?
| canvasPerspectiveView.addEventListener('mouseleave', (event: MouseEvent): void => { | ||
| if (this.mode === Mode.DRAW) { | ||
| event.preventDefault(); | ||
| this.cancelDraw(); |
There was a problem hiding this comment.
Why do we need stop drawing when mouse gets out of the canvas?
| @@ -396,6 +401,7 @@ const CanvasWrapperComponent = (props: Props): ReactElement => { | |||
| canvasInstanceDOM.perspective.removeEventListener('click', onCanvasClick); | |||
| canvasInstanceDOM.perspective.removeEventListener('canvas.fit', onResize); | |||
| canvasInstanceDOM.perspective.removeEventListener('canvas.groupped', onCanvasObjectsGroupped); | |||
| canvasInstanceDOM.perspective.removeEventListener('canvas.settings', onCanvasShapeDrawn); | |||
There was a problem hiding this comment.
Why do we need to call onCanvasShapeDrawn (which is literally should be called after an object is drawn on 3D canvas) after constructor() of Canvas3DView? Could you explain logic?
| } else if (reason === UpdateReasons.SHAPE_ACTIVATED) { | ||
| const { clientID } = this.model.data.activeElement; | ||
| this.setupObjects(); | ||
| if (clientID !== 'null') { | ||
| this.setupObjects(true); |
There was a problem hiding this comment.
Why do we need to pass the argument here (based on this.model.data.activeElement)? In this.setupObjects we also get access to this.model.data.activeElement (right in the first line)
| this.setHelperVisibility(false); | ||
| for (let i = 0; i < this.model.data.objects.length; i++) { | ||
| const object = this.model.data.objects[i]; | ||
| if (selected) { | ||
| const target = this.views.perspective.scene.getObjectByName(clientID); |
There was a problem hiding this comment.
clientID is not changed in the loop, why do we get target identifier in the loop?
There was a problem hiding this comment.
If target is const, than line:
```this.views.perspective.scene.children[0].remove(target);`` tries to remove the same element a lot of times.
Please, correct me if I am wrong
| this.views.side.scene.children[0].children = []; | ||
| this.views.front.scene.children[0].children = []; | ||
| this.views.top.scene.children[0].children = []; |
There was a problem hiding this comment.
Why do we need to clear side/front/top scenes a lot of times if only one object can be there at the same time?
| const { canvasBackgroundColor } = this.model.data.shapeProperties; | ||
| this.views.top.scene.background = new THREE.Color(canvasBackgroundColor); | ||
| this.views.perspective.scene.background = new THREE.Color(canvasBackgroundColor); | ||
| this.views.side.scene.background = new THREE.Color(canvasBackgroundColor); | ||
| this.views.front.scene.background = new THREE.Color(canvasBackgroundColor); |
There was a problem hiding this comment.
Did you try to test this code?
|
@manasars , we are not going to access these changes. Too many new comments from our team. |
Changes Include:
developbranchLicense
Feel free to contact the maintainers if that's a concern.