CVAT-3D Milestone1 #2534

manasars · 2020-12-07T12:56:42Z

CVAT-3D : Create a 3D task - Support to upload PCD/BIN files in archived zip.

Changes include:
Added Bin as accepted MIME type under Image Set
Added Dimension Validation Class to verify PCD vs PhotoCD and BIN to PCD conversion.
Added Support for Point Cloud and BIN Files for 3D Tasks.
Added an extra attribute dimension across API's and Formats.
Modified MPEG and Zip Chunk Writer to support PCD

Test Cases:
Manual Unit testing done locally.
Existing test cases work as expected.

[x ] I submit my changes into the develop branch
[ x] I have added tests to cover my changes
[ x] I submit my code changes under the same MIT License that covers the project.
Feel free to contact the maintainers if that's a concern.

…open3D

…d created a relatedfiles table for PCD to Image mapping.

…ration Scripts and Dimension attribute for MpegChunk Writers

zhiltsov-max

Changes regarding formats look ok. The only thing I can suggest is adding some kind of enumeration for available values for dimension args.

cvat/apps/engine/media_extractors.py

cvat/apps/dataset_manager/serializers.py

cvat/apps/dataset_manager/formats/registry.py

cvat/apps/engine/media.mimetypes

nmanovic · 2020-12-09T19:30:03Z

cvat/apps/engine/media_extractors.py

@@ -179,25 +194,48 @@ def _make_name():

 class ZipReader(ImageListReader):
    def __init__(self, source_path, step=1, start=0, stop=None):
-        self._zip_source = zipfile.ZipFile(source_path[0], mode='r')
+        self._zip_source = zipfile.ZipFile(source_path[0], mode='a')


What is a reason to change mode here?

We add the converted (bin to PCD), into the in memory zip again, thus we require it to be "a" mode, against creating an entirely new zip

cvat/apps/engine/media_extractors.py

nmanovic · 2020-12-09T19:45:02Z

cvat/apps/engine/media_extractors.py

+
+        pcd_version = ["0.7", "0.6", "0.5", "0.4", "0.3", "0.2", "0.1", ".7", ".6", ".5", ".4", ".3", ".2", ".1"]
+        with open(path, "rb") as file_read:
+            data = file_read.read(70)


I saw code to parse the header above. It is better to reuse it.

cvat/apps/engine/models.py

nmanovic · 2020-12-09T20:06:13Z

cvat/apps/engine/media_extractors.py

+        if not self.path:
+            return
+        actual_path = self.path
+        for root, dirs, files in os.walk(actual_path):


@zhiltsov-max , should the code (import velodyne dataset) migrate to datumaro? Or we prefer to keep it here for a while.

cvat-core/src/api-implementation.js

cvat-ui/src/components/actions-menu/actions-menu.tsx

vnishukov · 2020-12-10T09:12:09Z

cvat-ui/src/components/actions-menu/dump-submenu.tsx

@@ -19,15 +19,17 @@ interface Props {
    menuKey: string;
    dumpers: any[];
    dumpActivities: string[] | null;
+    taskDimension: string;


Corresponding above, something like this:

taskDimension: TaskDimension.D2 | TaskDimension.D3

Changed code accordingly.

Lets' use unified name here DimensionType (like in Python code)

@bsekachev can we use cvat-core/src/enums.js here? (it might be a hard link of 2 separate modules, I believe)

cvat-ui/src/components/actions-menu/load-submenu.tsx

cvat/apps/engine/media_extractors.py

azhavoro · 2020-12-10T11:11:03Z

cvat/apps/engine/media_extractors.py

+    def get_zip_filename(self):
+        return self._zip_source.filename
+
+    def initialize_for_3d(self, source_path, step=1, start=0, stop=None):


If you inherit a new class, this method is no longer needed

I agree. Instead of multiple "if" in the class, let us create a new one like LidarDataReader or something like that.

Image dataset and pcd dataset can be in a zip format, to classify if the dataset is for a 2D task or 3D task.
If we create another class as "LidarDataReader", we need to unzip the zip file twice, once for extracting the data in "ZipReader",then again in "LidarReader" class since zip is a common media-type.

https://github.com/openvinotoolkit/cvat/blob/0877de4885232cdfa7d6b8e3d9516a756df06f99/cvat/apps/engine/media_extractors.py#L545

Instead we modified the "ZipReader" class to and used reconcile method to distinguish between the dimensionType and now the multiple if's are not present.
It is only checked once on initialisation.

cvat-ui/src/components/actions-menu/dump-submenu.tsx

cvat-ui/src/components/actions-menu/export-submenu.tsx

cvat-ui/src/components/actions-menu/load-submenu.tsx

cvat-ui/src/components/model-runner-modal/detector-runner.tsx

cvat/apps/engine/models.py

vnishukov

Enum for cvat_ui

cvat/apps/engine/models.py

Dockerfile

manasars · 2020-12-24T14:37:42Z

Hi @nmanovic ,

The ci/cd build has passed, latest code pushed to PR.

nmanovic · 2020-12-24T17:06:14Z

cvat-ui/src/reducers/interfaces.ts

@@ -569,3 +569,8 @@ export interface CombinedState {
    shortcuts: ShortcutsState;
    review: ReviewState;
 }
+
+export enum DimensionType {


@manasars , we have similar definition in cvat-core/src/enums.js. What is the reason for that?

As mentioned in earlier review inputs, we have similar implementation of ReviewStatus in both the places, hence we implemented similarly.

@nmanovic Need to rewrite cvat-core in typescript to avoid this.

nmanovic · 2020-12-24T17:20:27Z

cvat/apps/engine/media_extractors.py

@@ -179,7 +183,8 @@ def _make_name():

 class ZipReader(ImageListReader):
    def __init__(self, source_path, step=1, start=0, stop=None):
-        self._zip_source = zipfile.ZipFile(source_path[0], mode='r')
+        self._dimension = DimensionType.DIM_2D


Andrey and I asked you to reimplement the class. It is much better to create a class like LidarDataReader inherited from ZipReader and redefine all corresponding methods. Thus you will not need to have if self._dimension == DimensionType.DIM_3D: mostly in every method. Let me know if you disagree by a reason.

As mentioned in the comments earlier, We distinguish the mime types only after we initialise the zipWriter class as initially we only enquire on what is the extension of the main file, which here is a zip (MediaFile). After the extraction along with picking the names of possible image files, the actual zip is deleted by keeping file pointer open in memory.

Upon deleting of zipExtractor object, the del would close the open file pointer, which is being actively
utilized further in the process. Thus introduction of LidarData Class would disrupt the flow and make it required to
create another zip file or abandon the deletion in the ZipExtractor.
Also if the 2 if are removed, then we need to introduce if in places such as FrameProvider, task.py and possibly when
import export is being carried based on dimension.
Kindly let us know your comments on the same.

nmanovic · 2020-12-24T17:49:13Z

cvat/apps/engine/media_extractors.py

@@ -283,8 +312,9 @@ def get_image_size(self, i):
        return image.width, image.height

 class IChunkWriter(ABC):
-    def __init__(self, quality):
+    def __init__(self, quality, dimension=DimensionType.DIM_2D):


Again better to inherit here. For example, IChunkWriter for common code, IChunk2dWriter(IChunkWriter) and IChunk3dWriter(IChunkWriter) or something like that.

The only parameter we require for distinguishing 2D and 3D task is _dimension which by default is always 2D.
Only in the case of zipCompressedChunkWriter we require the same and only at this point we have an if.
If another class is created for the same, such as zipCompressed3DChunkWriter along with IChunk3dWriter, then we require to place multiple if in frameProvider and task.py. Please let me know if you have any different opinion.

Does an additional type that is defined here https://github.com/openvinotoolkit/cvat/blob/develop/cvat/apps/engine/models.py#L34-L37 solve this problem?

Hi @azhavoro,

The additional type mentioned here will not help since the ext type 'zip' can have both images and pcd data and the additional type list accepts list as input whereas we have zip as input.

nmanovic · 2020-12-24T17:56:24Z

@manasars , an extra level of directories + temporary files inside archives. Could you please fix that?

test_pointcloud_pcd.zip\test_pointcloud_pcd\test_pointcloud_pcd\related_images\000001_pcd

cvat/apps/engine/tests/test_rest_api.py

nmanovic · 2020-12-24T18:10:19Z

@manasars , an extra level of directories + temporary files inside archives. Could you please fix that?

test_pointcloud_pcd.zip\test_pointcloud_pcd\test_pointcloud_pcd\related_images\000001_pcd

The same problem with another archive.

coveralls · 2020-12-25T11:23:57Z

Coverage increased (+0.2%) to 62.948% when pulling 4b61eb4 on manasars:cvat-3D-M1 into 01d35c8 on openvinotoolkit:develop.

nmanovic · 2020-12-29T12:08:58Z

@bsekachev , do you have any questions to the client or server part? Could you please review?

bsekachev · 2020-12-29T15:56:45Z

@nmanovic

I do not have critical comments about the patch. Looks like my comments and the most comments of other guys have been resolved. There are several unresolved conversations above opened not by me.
Also I've noticed newly appeared client-side code issues (not critical) and the most of them are expected to be highlighted or even fixed automatically by static linters on precommit hook. Apparently it doesn't work or just ignored. And I believe we should clarify this moment with @manasars because getting rid of such the issues on code review was exact purpose of the linters integration.

bsekachev · 2020-12-29T15:58:23Z

cvat-core/src/annotation-formats.js

+                dimension: {
+                    /**
+                     * @name dimension
+                     * @type {string}


probably should be module:API.cvat.enums.TaskDimension

Made changes accordingly.

bsekachev · 2020-12-29T15:58:27Z

cvat-core/src/annotation-formats.js

+                dimension: {
+                    /**
+                     * @name dimension
+                     * @type {string}


probably should be module:API.cvat.enums.TaskDimension

Made changes accordingly.

bsekachev · 2020-12-29T15:59:02Z

cvat-core/src/annotation-formats.js

@@ -15,6 +15,7 @@
                format: initialData.ext,
                version: initialData.version,
                enabled: initialData.enabled,
+                dimension: initialData.dimension


Missed trailing comma

Made changes accordingly.

bsekachev · 2020-12-29T16:00:04Z

cvat-core/src/session.js

@@ -974,6 +974,7 @@
                use_zip_chunks: undefined,
                use_cache: undefined,
                copy_data: undefined,
+                dimension: undefined


Missed trailing comma

Made changes accordingly.

bsekachev · 2020-12-29T16:00:25Z

cvat-core/src/session.js

+                    dimension: {
+                    /**
+                     * @name enabled
+                     * @type {string}


probably should be module:API.cvat.enums.TaskDimension

Made changes accordingly.

bsekachev · 2020-12-29T16:05:00Z

cvat-ui/src/components/annotation-page/top-bar/annotation-menu.tsx

@@ -161,6 +161,7 @@ export default function AnnotationMenuComponent(props: Props): JSX.Element {
                dumpers,
                dumpActivities,
                menuKey: Actions.DUMP_TASK_ANNO,
+                taskDimension:jobInstance.task.dimension


Missed space

Made changes accordingly.

bsekachev · 2020-12-29T16:05:04Z

cvat-ui/src/components/annotation-page/top-bar/annotation-menu.tsx

@@ -169,11 +170,13 @@ export default function AnnotationMenuComponent(props: Props): JSX.Element {
                    onClickMenuWrapper(null, file);
                },
                menuKey: Actions.LOAD_JOB_ANNO,
+                taskDimension:jobInstance.task.dimension


Missed space

Made changes accordingly.

bsekachev · 2020-12-29T16:05:07Z

cvat-ui/src/components/annotation-page/top-bar/annotation-menu.tsx

            })}
            {ExportSubmenu({
                exporters: dumpers,
                exportActivities,
                menuKey: Actions.EXPORT_TASK_DATASET,
+                taskDimension:jobInstance.task.dimension


Missed space

Made changes accordingly.

bsekachev · 2020-12-29T16:05:12Z

cvat-ui/src/components/model-runner-modal/detector-runner.tsx

@@ -14,6 +14,7 @@ import Text from 'antd/lib/typography/Text';
 import InputNumber from 'antd/lib/input-number';
 import Button from 'antd/lib/button';
 import notification from 'antd/lib/notification';
+import {DimensionType} from '../../reducers/interfaces'


Missed space

Made changes accordingly.

bsekachev · 2020-12-29T16:06:19Z

cvat-ui/src/reducers/interfaces.ts

@@ -569,3 +569,8 @@ export interface CombinedState {
    shortcuts: ShortcutsState;
    review: ReviewState;
 }
+
+export enum DimensionType {


@nmanovic Need to rewrite cvat-core in typescript to avoid this.

manasars · 2020-12-30T08:48:29Z

Hi @bsekachev / @nmanovic ,
The pre-commit hook had stopped working earlier due to some git config.
We have modified the same and now the lint changes are checked.
Fixed all the mentioned lint errors.

cdp added 5 commits December 7, 2020 13:11

CVAT-3D Updated the Mime Types with Bin Support, added dependency of …

a40d8dd

…open3D

CVAT-3D Added additional column as Dimension for engine_task table an…

6bcac4a

…d created a relatedfiles table for PCD to Image mapping.

Added Support for 3D file Upload in BIN and PCD.

db27800

Added Dimension attribute defaulting to 2D for importer and exporter.

7f89251

Added props passing for dimension attribute, filtering of import, Mig…

0fd3742

…ration Scripts and Dimension attribute for MpegChunk Writers

manasars requested review from bsekachev, nmanovic and zhiltsov-max as code owners December 7, 2020 12:56

zhiltsov-max reviewed Dec 8, 2020

View reviewed changes

cvat/apps/engine/media_extractors.py Show resolved Hide resolved

cvat/apps/engine/media_extractors.py Outdated Show resolved Hide resolved

cvat/apps/engine/media_extractors.py Outdated Show resolved Hide resolved

cvat/apps/engine/media_extractors.py Show resolved Hide resolved

nmanovic reviewed Dec 9, 2020

View reviewed changes

cvat/apps/dataset_manager/serializers.py Outdated Show resolved Hide resolved

nmanovic reviewed Dec 9, 2020

View reviewed changes

cvat/apps/dataset_manager/formats/registry.py Outdated Show resolved Hide resolved

nmanovic reviewed Dec 9, 2020

View reviewed changes

cvat/apps/engine/media.mimetypes Outdated Show resolved Hide resolved

nmanovic reviewed Dec 9, 2020

View reviewed changes

cvat/apps/engine/media_extractors.py Outdated Show resolved Hide resolved

nmanovic reviewed Dec 9, 2020

View reviewed changes

cvat/apps/engine/media_extractors.py Outdated Show resolved Hide resolved

nmanovic reviewed Dec 9, 2020

View reviewed changes

cvat/apps/engine/models.py Outdated Show resolved Hide resolved

nmanovic reviewed Dec 9, 2020

View reviewed changes

cvat/apps/engine/models.py Show resolved Hide resolved

nmanovic reviewed Dec 9, 2020

View reviewed changes

vnishukov reviewed Dec 10, 2020

View reviewed changes

cvat-core/src/api-implementation.js Outdated Show resolved Hide resolved

vnishukov reviewed Dec 10, 2020

View reviewed changes

cvat-ui/src/components/actions-menu/actions-menu.tsx Outdated Show resolved Hide resolved

vnishukov reviewed Dec 10, 2020

View reviewed changes

cvat-ui/src/components/actions-menu/load-submenu.tsx Outdated Show resolved Hide resolved

azhavoro reviewed Dec 10, 2020

View reviewed changes

bsekachev requested changes Dec 10, 2020

View reviewed changes

cvat/apps/engine/models.py Outdated Show resolved Hide resolved

cdp added 2 commits December 14, 2020 21:18

Modified code as per review comments

3be8ef9

Updated Unit test cases for 3D task creation

6357751

vnishukov suggested changes Dec 15, 2020

View reviewed changes

nmanovic reviewed Dec 15, 2020

View reviewed changes

cvat/apps/engine/models.py Outdated Show resolved Hide resolved

cdp added 2 commits December 24, 2020 13:15

Modified test file name

733e4bb

Merge branch 'develop' into cvat-3D-M1

fc5cf64

azhavoro reviewed Dec 24, 2020

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

cdp added 3 commits December 24, 2020 14:25

Trigger travis ci

45d276f

Modified test case to ignore 3D preview images

6c0d765

Trigger notification

86c7494

nmanovic reviewed Dec 24, 2020

View reviewed changes

cvat/apps/engine/tests/test_rest_api.py Outdated Show resolved Hide resolved

nmanovic reviewed Dec 24, 2020

View reviewed changes

cvat/apps/engine/tests/test_rest_api.py Outdated Show resolved Hide resolved

nmanovic reviewed Dec 24, 2020

View reviewed changes

cvat/apps/engine/tests/test_rest_api.py Outdated Show resolved Hide resolved

nmanovic reviewed Dec 24, 2020

View reviewed changes

cvat/apps/engine/tests/test_rest_api.py Outdated Show resolved Hide resolved

cdp added 3 commits December 25, 2020 14:55

Merge branch 'develop' into cvat-3D-M1

7c3017d

Deleting DS Store files

768a17e

Modified test cases as per review comments

6879381

bsekachev reviewed Dec 29, 2020

View reviewed changes

cdp added 3 commits December 30, 2020 11:24

Merge branch 'develop' into cvat-3D-M1

f0217ca

Checking pre-commit hook

2af7627

Fixed Lint issues - precommit hook verification

4b61eb4

bsekachev approved these changes Dec 31, 2020

View reviewed changes

nmanovic approved these changes Jan 8, 2021

View reviewed changes

nmanovic merged commit 069fadc into cvat-ai:develop Jan 8, 2021

CVAT-3D Milestone1 #2534

CVAT-3D Milestone1 #2534

Conversation

manasars commented Dec 7, 2020

zhiltsov-max left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnishukov left a comment

Choose a reason for hiding this comment

manasars commented Dec 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmanovic commented Dec 24, 2020

nmanovic commented Dec 24, 2020

coveralls commented Dec 25, 2020 • edited Loading

nmanovic commented Dec 29, 2020

bsekachev commented Dec 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manasars commented Dec 30, 2020 • edited Loading

coveralls commented Dec 25, 2020 •

edited

Loading

manasars commented Dec 30, 2020 •

edited

Loading