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

CVAT 3D - Milestone-4 #2891

Merged
merged 28 commits into from
Mar 23, 2021
Merged

CVAT 3D - Milestone-4 #2891

merged 28 commits into from
Mar 23, 2021

Conversation

manasars
Copy link
Contributor

@manasars manasars commented Mar 1, 2021

CVAT-3D-Milestone4 : Implement initial cuboid placement in 3D View and selected cuboid adjustment in Top, Side and Front views.

Changes include:
Implemented initial cuboid placement in 3D View.
Implemented display of top, front, and side view of the active cuboid.
Implemented adjusting the borders of views to allow changing the width and height of a specific view.
Implemented centre axis for PCD rotation.
Fixed the UTF-8 encoding exception which occurred while creating zip file in MACOS.

Test Cases:
Manual Unit testing done locally.
Existing test cases work as expected.
System Test cases will be shared.

[x ] I submit my changes into the develop branch

[ x] I submit my code changes under the same MIT License that covers the project.

@coveralls
Copy link

coveralls commented Mar 1, 2021

Coverage Status

Coverage increased (+0.04%) to 75.257% when pulling e53501f on manasars:cvat-3D-M4 into 4f7b1f9 on openvinotoolkit:develop.

cvat-ui/package.json Outdated Show resolved Hide resolved
@bsekachev
Copy link
Member

Hi guys, here are my comments about the patch. I will check the code additionally.

  • N shortcut does not work to draw a shape
  • Shift+N shortcut does not work to redraw a shape
  • I cannot up/down cuboids.

Weren't these features implemented yet?

  • As I understand we decided that cuboid track is not applicable for 3D, right? Why do we still have options between a shape and a track when create a cuboid? Also we have options (from rectangle, from 4 points) which are also inapplicable here I believe.

  • Looks like keys: U I O J K L do not work.

  • Text is not fitted for side views when they are thin (screenshot below)
    Screenshot from 2021-03-12 11-31-55

  • I noticed strange behavior when cuboid and view are moved during I actually do not move the cursor (attached gif below, second half)
    Peek 2021-03-12 11-37

  • When I start drawing performance is awful (a lot of freezes, according to chrome profiler each mouse movement call 250ms of computations). I have Core i7 8th generation CPU. A screenshot also attached. Are you planning to work on it?

Screenshot from 2021-03-12 11-46-47

  • Also when I open a task, in console I have some warnings:
Warning: Failed prop type: Invalid prop `height` of type `string` supplied to `Resizable`, expected `number`.
    in Resizable (created by ResizableBox)
    in ResizableBox (created by CanvasWrapperComponent)
    in div (created by CanvasWrapperComponent)
    in main (created by Basic)
    in Basic (created by Content)
    in Content (created by CanvasWrapperComponent)
    in CanvasWrapperComponent (created by ConnectFunction)
    in ConnectFunction (created by StandardWorkspace3DComponent)
    in section (created by BasicLayout)
    in BasicLayout (created by Layout)
    in Layout (created by StandardWorkspace3DComponent)
    in StandardWorkspace3DComponent (created by AnnotationPageComponent)
    in main (created by Basic)
    in Basic (created by Content)
    in Content (created by AnnotationPageComponent)
    in section (created by BasicLayout)
    in BasicLayout (created by Layout)
    in Layout (created by AnnotationPageComponent)
    in AnnotationPageComponent (created by ConnectFunction)
    in ConnectFunction (created by Context.Consumer)
    in withRouter(Connect(AnnotationPageComponent)) (created by Context.Consumer)
    in Route (created by CVATApplication)
    in Switch (created by CVATApplication)
    in GlobalHotKeys (created by CVATApplication)
    in main (created by Basic)
    in Basic (created by Content)
    in Content (created by CVATApplication)
    in section (created by BasicLayout)
    in BasicLayout (created by Layout)
    in Layout (created by CVATApplication)
    in GlobalErrorBoundary (created by ConnectFunction)
    in ConnectFunction (created by CVATApplication)
    in CVATApplication (created by Context.Consumer)
    in withRouter(CVATApplication) (created by ConnectFunction)
    in ConnectFunction
    in Router (created by BrowserRouter)
    in BrowserRouter
    in Provider

@bsekachev
Copy link
Member

Implemented centre axis for PCD rotation.

I am not sure it is in the center of .pcd (gif attached). Please, correct me if I am wrong.
Peek 2021-03-12 11-56

@bsekachev
Copy link
Member

And also I have some issues with context image (It is not a part of this milestone, but we expect everything should be already worked there).

We have two test archives:
https://github.com/openvinotoolkit/cvat/blob/develop/cvat/apps/engine/tests/assets/test_pointcloud_pcd.zip
https://github.com/openvinotoolkit/cvat/blob/develop/cvat/apps/engine/tests/assets/test_velodyne_points.zip

Both of them contains .pcd and .png. But context image can be opened only in the second. Could you please comment?
On the second we also see that context image overfitted its container. So, it also looks like a bug.

Screenshot from 2021-03-12 12-01-09

@manasars
Copy link
Contributor Author

Hi @bsekachev ,

  1. The cuboid track and Drawing method have now been removed, it was not pushed earlier since we discussed during Milestone-5 features discussion.
  2. U I O J K L keys work with the combination of Alt, since we have conflicting keys as discussed in Milestone3.
  3. The text for views has been changed to Top, Side and Front as per SOW, so the fit issue will not occur now.
  4. The strange behaviour noticed while moving cuboid and scene has been fixed.
  5. The performance of this particular PCD(kitchen.zip) is similar in supervise.ly as well. We also noticed that this PCD does not load properly in supervise.ly and looks like the pcd is very dense, we checked with other PCD files and performance seems to be fine, if any further tuning required we will look into it in the upcoming milestones.
  6. PFA performance comparison for the kitchen.zip w.r.t supervise.ly

performance-comparision
7. The context image fit issue and test archive file context-image load issue have been fixed.
8. The short cut key Shift+N , N was missed from our end, let us know if it is fine to push the change along with next Milestone.

@bsekachev
Copy link
Member

bsekachev commented Mar 17, 2021

@manasars

The cuboid track and Drawing method have now been removed, it was not pushed earlier since we discussed during Milestone-5 features discussion.

But are you planning to remove them in the future?

U I O J K L keys work with the combination of Alt, since we have conflicting keys as discussed in Milestone3.

What about adding a tooltip because it is not clear that I should use 'Alt' together with these buttons. What is more, I see you use Button element to display shortcuts. IMHO buttons are assumed to do some actions, but they do not. Can we add the same actions for these button as shortcuts do?

Screenshot from 2021-03-17 11-09-04

The text for views has been changed to Top, Side and Front as per SOW, so the fit issue will not occur now.
The strange behaviour noticed while moving cuboid and scene has been fixed.
The context image fit issue and test archive file context-image load issue have been fixed.

Thanks, I'll check it one more time. @dvkruchinin could you please check if now you can write a tests for context image?

The short cut key Shift+N , N was missed from our end, let us know if it is fine to push the change along with next Milestone.

Please, discuss your plans with @nmanovic .

PFA performance comparison for the kitchen.zip w.r.t supervise.ly

As I see CVAT cuboid placing is 3x slower than in supervise.ly. I believe it should be accelerated. @nmanovic , what do you think?

@dvkruchinin
Copy link
Contributor

@bsekachev

could you please check if now you can write a tests for context image?

Sure. I`ll check.

@manasars manasars requested a review from azhavoro as a code owner March 19, 2021 07:14
@bsekachev
Copy link
Member

bsekachev commented Mar 19, 2021

Hi @manasars

I see CI tests were failed now. @dvkruchinin has setup some screenshot tests for the feature.
There is a test where two screenshots are compared (between and after wheeling TOP/SIDE/FRONT views).
The reason why the test failed is that both screenshots are the same (they are expected to be changed after wheeling).

Screenshot before:
topview_before_wheel

Screenshot after:
topview_after_wheel

You can find these screenshots pressing build detais and reviewing artifacts:
Screenshot from 2021-03-19 12-58-22

As you can see the both screenshot are equal and they are empty (no any points seen) what is unexpected.
Personally I tried to create a task with this file cvat/apps/engine/tests/assets/test_pointcloud_pcd.zip and all the views except of perspective are empty.

Screenshot from 2021-03-19 12-53-44

I've already reported this bug in the previous milestone, but nobody was able to reproduce it. Some time after it disappeared itself and now appeared again. Also we can see it is reproducible on CI.

UPD:

Uploaded the same file on supervise.ly (all the points are more noticeable there):
Screenshot from 2021-03-19 14-24-48

@manasars
Copy link
Contributor Author

manasars commented Mar 19, 2021

Hi @manasars

I see CI tests were failed now. @dvkruchinin has setup some screenshot tests for the feature.
There is a test where two screenshots are compared (between and after wheeling TOP/SIDE/FRONT views).
The reason why the test failed is that both screenshots are the same (they are expected to be changed after wheeling).

Hi @bsekachev ,
I downloaded the screenshot from the build artefact and was able to see the side and front views.PFB attachment:
Screen Shot 2021-03-19 at 5 27 29 PM
The points are not very bright but are visible.
Also when we ran the cypress locally, I get as test cases as passed:
Screen Shot 2021-03-19 at 5 32 20 PM

Also, when I upload the zip file locally, am able to see the pcd in all views:
Screen Shot 2021-03-19 at 5 54 20 PM

We are not yet sure on what is causing the issue in the build, trying to replicate in other systems.

@bsekachev
Copy link
Member

and was able to see the side and front views

Agree, that it is visible on CI screenshots (different .PCD is used on CI), but the object is much less visible on these views with file I've specified above when it could be noticeable on supervise.ly without any issues. I think we should start from fixing it.
Also I think black and blue are not enough contrast (supervise.ly used black/white as default).

In CVAT current implementation visibility apparently depends on points density somehow.
@dvkruchinin reported that increasing the density helps to pass these tests.


Probably one more issue is that the code in this patch affected somehow TOP view area and on screenshot we can't see any objects (as if it is displaced out of viewport)

Screenshot before wheel (develop):
before

Screenshot before wheel (this branch)
before_1

Important note: These screenshots are done in headless mode;
So, if the tests are successful for you, please, try additionally headless mode.

@dvkruchinin
Copy link
Contributor

dvkruchinin commented Mar 19, 2021

Hi @manasars ,

In addition to the message @bsekachev .
There is a feeling that the display of the point cloud on TOP does not work correctly (maybe I'm wrong).
cvat-canvas3d-fullsize_develop
This is a point cloud in the form of a cube in the amount of 100,000 points. But for TOP VIEW, it looks like the camera is farther away than on the SIDE/FRONT. (The picture was taken on the develop branch).
cy.get('.cvat-canvas3d-fullsize').first().screenshot('id_canvas3d_container')

And as wrote above @bsekachev Getting screenshots is different between the this branch and the develop branch.
This branch:
id_canvas3d_container_cvat-3d-m4
cy.get('#canvas3d-container').screenshot('id_canvas3d_container')
In this case id_canvas3d_container' is just the name of the image.

@manasars
Copy link
Contributor Author

Hi @dvkruchinin ,

In Milestone-4 we implemented resizing of views due to which the div has changed and delta differs, hence difference in screenshots between develop and this branch.

@bsekachev
Copy link
Member

Hi @manasars

Let me rephrase. It is expected, that the object is visible and centered in all the additional views regardless on availableness resizing feature. Also object looks smaller on top view (as @dvkruchinin mentioned), what is probably because of wrong camera position setup.

If you disagree, please explain your point of view.

@manasars
Copy link
Contributor Author

Hi @manasars

Let me rephrase. It is expected, that the object is visible and centered in all the additional views regardless on availableness resizing feature. Also object looks smaller on top view (as @dvkruchinin mentioned), what is probably because of wrong camera position setup.

If you disagree, please explain your point of view.

Hi @bsekachev ,

Yes agreed, the top view positioning needs to be adjusted, this came up as a part of adjusting the camera to support PCDs with extreme coordinates, we are working on the fix.

@bsekachev bsekachev self-requested a review March 22, 2021 14:11
@bsekachev
Copy link
Member

@manasars

Please, merge the latest develop. We expect CI should be fixed.

Comment on lines 26 to 44
imagePullPolicy: 'IfNotPresent'
env:
- name: POSTGRES_DB
valueFrom:
secretKeyRef:
name: cvat-postgres-secret
key: POSTGRES_DB
- name: POSTGRES_USER
valueFrom:
secretKeyRef:
name: cvat-postgres-secret
key: POSTGRES_USER
- name: POSTGRES_PASSWORD
valueFrom:
secretKeyRef:
name: cvat-postgres-secret
key: POSTGRES_PASSWORD
- name: POSTGRES_DB
valueFrom:
secretKeyRef:
name: cvat-postgres-secret
key: POSTGRES_DB
- name: POSTGRES_USER
valueFrom:
secretKeyRef:
name: cvat-postgres-secret
key: POSTGRES_USER
- name: POSTGRES_PASSWORD
valueFrom:
secretKeyRef:
name: cvat-postgres-secret
key: POSTGRES_PASSWORD
ports:
- containerPort: 5432
- containerPort: 5432
Copy link
Member

Choose a reason for hiding this comment

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

Have you done these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

Comment on lines 22 to 29
- image: redis:4.0.5-alpine
name: cvat-redis
imagePullPolicy: Always
ports:
- containerPort: 6379
resources:
limits:
cpu: '0.1'
Copy link
Member

Choose a reason for hiding this comment

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

Have you done these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted

Comment on lines 32 to 90
- name: DJANGO_MODWSGI_EXTRA_ARGS
value: ''
- name: UI_PORT
value: '80'
- name: UI_HOST
value: 'cvat-frontend-service'
- name: ALLOWED_HOSTS
value: '*'
- name: CVAT_REDIS_HOST
value: 'cvat-redis-service'
- name: CVAT_POSTGRES_HOST
value: 'cvat-postgres-service'
- name: CVAT_POSTGRES_USER
valueFrom:
secretKeyRef:
name: cvat-postgres-secret
key: POSTGRES_USER
- name: CVAT_POSTGRES_DBNAME
valueFrom:
secretKeyRef:
name: cvat-postgres-secret
key: POSTGRES_DB
- name: CVAT_POSTGRES_PASSWORD
valueFrom:
secretKeyRef:
name: cvat-postgres-secret
key: POSTGRES_PASSWORD
ports:
- containerPort: 8080
- containerPort: 8080
volumeMounts:
- mountPath: /home/django/data
name: cvat-backend-data
subPath: data
- mountPath: /home/django/keys
name: cvat-backend-data
subPath: keys
- mountPath: /home/django/logs
name: cvat-backend-data
subPath: logs
- mountPath: /home/django/models
name: cvat-backend-data
subPath: models
- mountPath: /home/django/data
name: cvat-backend-data
subPath: data
- mountPath: /home/django/keys
name: cvat-backend-data
subPath: keys
- mountPath: /home/django/logs
name: cvat-backend-data
subPath: logs
- mountPath: /home/django/models
name: cvat-backend-data
subPath: models
initContainers:
- name: user-data-permission-fix
image: busybox
command: ["/bin/chmod", "-R", "777", "/home/django"]
command: ['/bin/chmod', '-R', '777', '/home/django']
volumeMounts:
- mountPath: /home/django/data
name: cvat-backend-data
subPath: data
- mountPath: /home/django/keys
name: cvat-backend-data
subPath: keys
- mountPath: /home/django/logs
name: cvat-backend-data
subPath: logs
- mountPath: /home/django/models
name: cvat-backend-data
subPath: models
- mountPath: /home/django/data
name: cvat-backend-data
subPath: data
- mountPath: /home/django/keys
name: cvat-backend-data
subPath: keys
- mountPath: /home/django/logs
name: cvat-backend-data
subPath: logs
- mountPath: /home/django/models
name: cvat-backend-data
subPath: models
Copy link
Member

Choose a reason for hiding this comment

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

Have you done these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted code

@@ -25,5 +25,5 @@ spec:
image: openvino/cvat_ui:v1.2.0
imagePullPolicy: Always
ports:
- containerPort: 80
- containerPort: 80
Copy link
Member

Choose a reason for hiding this comment

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

Please, look at kubernetes-templates/*, I believe you didn't change these files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like pre-commit hook autocorrected the files, we shall revert.

@manasars
Copy link
Contributor Author

We shall be pushing again, the kubernetes reversion has not happened yet

@manasars
Copy link
Contributor Author

Hi @bsekachev , we have merged with latest develop code and the CI build is successful.

@bsekachev bsekachev merged commit 538bc93 into cvat-ai:develop Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants