schema: add KeyPoint3D, KeyLine3D, Polygon3D #45

pd-nisse · 2021-11-02T18:09:31Z

While KeyPoint2D, KeyLine2D, Polygon2D allow for point/line geomtries on the image plane, we would like to store the the 3D equivalents in carthesian coordinates in sensor space.

This change is

While KeyPoint2D, KeyLine2D, Polygon2D allow for point/line geomtries on the image plane, we would like to store the the 3D equivalents in carthesian coordinates in sensor space.

quincy-kh-chen

@pd-nisse In this DGP repo, we also push the compiled protos. Please run make build-proto and stage the changes for the compile ones. Thank you.

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @pd-nisse and @quincy-kh-chen)

-- commits, line 2 at r1:
Please modify the commit message to comply with commitlintrc. This PR failing the CI.

pre-merge failure: https://github.com/TRI-ML/dgp/runs/4084803939?check_suite_focus=true
contribution guidelines: https://github.com/TRI-ML/dgp/blob/master/docs/CONTRIBUTING.md#making-a-commit

dgp/proto/annotations.proto, line 59 at r1 (raw file):

Quoted 22 lines of code…

  // 2D Key Point on the image
  KEY_POINT_2D = 10;               // key_point_2d

  // 2D Key Line on the image
  KEY_LINE_2D = 11;                // key_line_2d

  // 2D Polygon on the image
  POLYGON_2D = 12;                 // polygon_2d

  // Agent behavior
  AGENT_BEHAVIOR = 14;             // agent_behavior

  // 3D Key Point in sensor space
  KEY_POINT_3D = 15;               // key_point_3d

  // 3D Key Line in sensor space
  KEY_LINE_3D = 16;                // key_line_3d

  // 3D Polygon in sensor space
  POLYGON_3D = 17;                 // polygon_3d
}

Let's reorder the field number as follow to be consistent:
FYI @ChaoFang-TRI

  KEY_POINT_2D = 10;               // key_point_2d

  // 3D Key Point on the point cloud
  KEY_POINT_3D = 11;               // key_point_3d

  // 2D Key Line on the image
  KEY_LINE_2D = 12;                // key_line_2d

  // 3D Key Line on the point cloud
  KEY_LINE_3D = 13;                // key_line_3d

  // 2D Polygon on the image
  POLYGON_2D = 14;                 // polygon_2d

  // 3D Polygon on the point cloud
  POLYGON_3D = 15;                 // polygon_3d

  // Agent behavior
  AGENT_BEHAVIOR = 16;             // agent_behavior

dgp/proto/annotations.proto, line 226 at r1 (raw file):

// 3D point.
message KeyPoint3D {

Please move KeyPoint3D under the existing KeyPoint2D to be consistent

dgp/proto/annotations.proto, line 250 at r1 (raw file):

// 3D line annotation.
message KeyLine3DAnnotation{

ditto

dgp/proto/annotations.proto, line 265 at r1 (raw file):

}

message PolygonPoint3D {

ditto

dgp/proto/annotations.proto, line 313 at r1 (raw file):

// List of KeyPoint3DAnnotation
message KeyPoint3DAnnotations {

ditto

dgp/proto/annotations.proto, line 318 at r1 (raw file):

// List of KeyLine3DAnnotation
message KeyLine3DAnnotations {

ditto

dgp/proto/annotations.proto, line 323 at r1 (raw file):

// List of Polygon3DAnnotation
message Polygon3DAnnotations {

ditto

quincy-kh-chen

+@quincy-kh-chen +@yuta-tsuzuki-woven for schema review FYI @ChaoFang-TRI @chrisochoatri @kuanleetri

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @pd-nisse, @quincy-kh-chen, and @yuta-tsuzuki-woven)

yuta-tsuzuki-woven

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @pd-nisse, @quincy-kh-chen, and @yuta-tsuzuki-woven)

dgp/proto/annotations.proto, line 59 at r1 (raw file):

Previously, quincy-kh-chen (quincy.chen) wrote…

  // 2D Key Point on the image
  KEY_POINT_2D = 10;               // key_point_2d

  // 2D Key Line on the image
  KEY_LINE_2D = 11;                // key_line_2d

  // 2D Polygon on the image
  POLYGON_2D = 12;                 // polygon_2d

  // Agent behavior
  AGENT_BEHAVIOR = 14;             // agent_behavior

  // 3D Key Point in sensor space
  KEY_POINT_3D = 15;               // key_point_3d

  // 3D Key Line in sensor space
  KEY_LINE_3D = 16;                // key_line_3d

  // 3D Polygon in sensor space
  POLYGON_3D = 17;                 // polygon_3d
}

Let's reorder the field number as follow to be consistent:
FYI @ChaoFang-TRI

  KEY_POINT_2D = 10;               // key_point_2d

  // 3D Key Point on the point cloud
  KEY_POINT_3D = 11;               // key_point_3d

  // 2D Key Line on the image
  KEY_LINE_2D = 12;                // key_line_2d

  // 3D Key Line on the point cloud
  KEY_LINE_3D = 13;                // key_line_3d

  // 2D Polygon on the image
  POLYGON_2D = 14;                 // polygon_2d

  // 3D Polygon on the point cloud
  POLYGON_3D = 15;                 // polygon_3d

  // Agent behavior
  AGENT_BEHAVIOR = 16;             // agent_behavior

@quincy-kh-chen
BTW, if we change the order, will it lose backward compatibility with existing datasets?

pd-nisse · 2021-11-05T09:58:28Z

@quincy-kh-chen : Thanks for the comments! I re-ordered per your draft, but surface_normals_2D inhibits the index 13 already. How would you like to see this resolved?

quincy-kh-chen

@pd-nisse good catch! Let's do a one-time correction on the indices.
@yuta-tsuzuki-woven this re-ordered is backward-compatible if we serialize/deserialize data via JSONs. We (TRI, Woven, PD) are all using DGP JSONs. We might switch to binaries in the future to boost the performance. Hence, it's better to correct the indices before we make the move.

FYI @ChaoFang-TRI @chrisochoatri
How about we re-order as follow:

  SURFACE_NORMALS_2D = 7;          // surface_normals_2d

  // 3D Surface normals on the point cloud
  SURFACE_NORMALS_3D = 8;          // surface_normals_3d

  // 2D Motion vectors on the image
  // (i.e. 2D optical flow)
  MOTION_VECTORS_2D = 9;           // motion_vectors_2d

  // 3D Motion vectors on the point cloud
  // (i.e. 3D flow on the point cloud)
  MOTION_VECTORS_3D = 10;           // motion_vectors_2d

  // 2D Key Point on the image
  KEY_POINT_2D = 11;               // key_point_2d

  // 3D Key Point on the point cloud
  KEY_POINT_3D = 12;               // key_point_3d

  // 2D Key Line on the image
  KEY_LINE_2D = 13;                // key_line_2d

  // 3D Key Line on the point cloud
  KEY_LINE_3D = 14;                // key_line_3d

  // 2D Polygon on the image
  POLYGON_2D = 15;                 // polygon_2d

  // 3D Polygon on the point cloud
  POLYGON_3D = 16;                 // polygon_3d

  // Agent behavior
  AGENT_BEHAVIOR = 17;             // agent_behavior

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @pd-nisse, @quincy-kh-chen, and @yuta-tsuzuki-woven)

yuta-tsuzuki-woven

@quincy-kh-chen In my understanding, these annotation type numbers are populated in scene JSON as a key of annotations in datum and ontology like following and do we need to fix the number in existing datasets if we re-order them?

Sorry, if I misunderstood somthing.

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @pd-nisse, @quincy-kh-chen, and @yuta-tsuzuki-woven)

ChaoFang-TRI

I think we shall not reorder, only append new anno types.

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @pd-nisse, @quincy-kh-chen, and @yuta-tsuzuki-woven)

quincy-kh-chen

Hello @yuta-tsuzuki-woven @ChaoFang-TRI @pd-nisse yes you're right, it's not backward compatible even if we're using JSONs. Some of our existing datasets already use these AnnotationType numbers. Let's just append the new ones.

Reviewable status: 0 of 1 files reviewed, 8 unresolved discussions (waiting on @pd-nisse, @quincy-kh-chen, and @yuta-tsuzuki-woven)

- added 3D equivalents to new 2D geometries

pd-nisse · 2021-11-19T10:24:01Z

@quincy-kh-chen : I left the order as appending and built the proto files.

quincy-kh-chen

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @pd-nisse)

dgp/proto/annotations.proto, line 227 at r2 (raw file):

 z) point (in 3D carthesian coordinates).

/carthesian/cartesian/

dgp/proto/annotations.proto, line 228 at r2 (raw file):

message KeyPoint3D {
  // (x, y, z) point (in 3D carthesian coordinates).
  double x = 1;

Do we need double? Is float sufficient? cc @chrisochoatri what's your thoughts?

dgp/proto/annotations.proto, line 254 at r2 (raw file):

  uint32 class_id = 1;

  // 3D line.

What order vertices should follow? e.g. clockwise or counterclockwise, with respect to what axis?

dgp/proto/annotations.proto, line 265 at r2 (raw file):

}

message PolygonPoint3D {

Let's add a simple docstring to explain what's PolygonPoint3D. Isn't this duplicated with KeyPoint3D?

dgp/proto/annotations.proto, line 266 at r2 (raw file):

message PolygonPoint3D {
  // (x, y, z) point (in 3D carthesian coordinates).

/carthesian/cartesian/

dgp/proto/annotations.proto, line 267 at r2 (raw file):

message PolygonPoint3D {
  // (x, y, z) point (in 3D carthesian coordinates).
  double x = 1;

Do we need double? Is float sufficient?

dgp/proto/annotations.proto, line 278 at r2 (raw file):

  // 3D polygon.
  // Points should be put into this field with counter-clockwise order.

Let's try elaborating counter-clockwise order e.g. with respect to what axis?

dgp/proto/annotations.proto, line 312 at r2 (raw file):

}

// List of KeyPoint3DAnnotation

Please rebase. Now we always end with a period.

dgp/proto/annotations.proto, line 317 at r2 (raw file):

}

// List of KeyLine3DAnnotation

Please rebase. Now we always end with a period.

dgp/proto/annotations.proto, line 322 at r2 (raw file):

}

// List of Polygon3DAnnotation

Please rebase. Now we always end with a period.

quincy-kh-chen

@pd-nisse thank you. Please rebase.

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @pd-nisse)

chrisochoatri

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @pd-nisse and @quincy-kh-chen)

dgp/proto/annotations.proto, line 228 at r2 (raw file):

Previously, quincy-kh-chen (quincy.chen) wrote…

Do we need double? Is float sufficient? cc @chrisochoatri what's your thoughts?

I'm not sure we have a need to double precision, maybe float is enough. But unless we are storing a very large number of keypoints I don't see the harm in double (just don't see a use case).

dgp/proto/annotations.proto, line 278 at r2 (raw file):

Previously, quincy-kh-chen (quincy.chen) wrote…

Let's try elaborating counter-clockwise order e.g. with respect to what axis?

Similar curiosity, I assume these vertices all need to be on the same plane? If so maybe add a note since that will need to be enforced by whoever saves the this data.

akira-wakatsuki · 2022-02-02T09:55:38Z

Hi @pd-nisse @quincy-kh-chen @yuta-tsuzuki-woven

I'm Akira from Woven Alpha. We are also interested in this PR and looking forward to getting this PR merged. All comments look resolved and let me ask what is remaining.

Thanks in advance.

nehalmamgain · 2022-04-25T03:58:35Z

Will get this moving.

tk-woven

Thanks for this :) My review was requested from the Woven JP side, so I left some comments.

tk-woven · 2022-07-29T00:51:15Z

dgp/proto/annotations.proto

+
+  // An associative map stores arbitrary attributes, where the key is attribute name
+  // and the value is attribute value. Both key_type and value_type are string.
+  map<string, string> attributes = 3;


Could you please share where we can find documentation of keys and value semantics to expect here?

tk-woven · 2022-07-29T00:51:31Z

dgp/proto/annotations.proto

+
+// 3D point annotation.
+message KeyPoint3DAnnotation {
+  // Class identifier (should be in [0, num_classes - 1])


Could you please share where num_classes defined?

tk-woven · 2022-07-29T00:52:31Z

dgp/proto/annotations.proto

+  // and the value is attribute value. Both key_type and value_type are string.
+  map<string, string> attributes = 3;
+
+  // An identifier key. Used to link with other annotations.


Can we expand on the semantics here please? What does a proper value look like to make a correct link?

nehalmamgain · 2022-08-01T04:39:05Z

Continuing discussion here

schema: add KeyPoint3D, KeyLine3D, Polygon3D

05c3af0

While KeyPoint2D, KeyLine2D, Polygon2D allow for point/line geomtries on the image plane, we would like to store the the 3D equivalents in carthesian coordinates in sensor space.

quincy-kh-chen suggested changes Nov 5, 2021

View reviewed changes

quincy-kh-chen assigned quincy-kh-chen and yuta-tsuzuki-woven Nov 5, 2021

quincy-kh-chen reviewed Nov 5, 2021

View reviewed changes

yuta-tsuzuki-woven reviewed Nov 5, 2021

View reviewed changes

Merge branch 'TRI-ML:master' into pd/nisse/point-line-polygon-3d-proto

7022c47

quincy-kh-chen reviewed Nov 5, 2021

View reviewed changes

yuta-tsuzuki-woven reviewed Nov 7, 2021

View reviewed changes

ChaoFang-TRI reviewed Nov 8, 2021

View reviewed changes

quincy-kh-chen reviewed Nov 9, 2021

View reviewed changes

sshusainTRI force-pushed the master branch from 5794634 to 1d0c267 Compare November 18, 2021 17:02

pd-nisse and others added 2 commits November 19, 2021 11:14

Merge branch 'TRI-ML:master' into pd/nisse/point-line-polygon-3d-proto

f878a1a

schema: add KeyPoint3D, KeyLine3D, Polygon3D

6a8aa98

- added 3D equivalents to new 2D geometries

quincy-kh-chen suggested changes Dec 14, 2021

View reviewed changes

quincy-kh-chen reviewed Dec 14, 2021

View reviewed changes

chrisochoatri reviewed Dec 20, 2021

View reviewed changes

tk-woven requested changes Jul 29, 2022

View reviewed changes

nehalmamgain mentioned this pull request Aug 1, 2022

schema: add KeyPoint3D, KeyLine3D, Polygon3D #101

Merged

nehalmamgain mentioned this pull request Aug 1, 2022

style: modify parallel domain metadata with make build-proto #102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schema: add KeyPoint3D, KeyLine3D, Polygon3D #45

schema: add KeyPoint3D, KeyLine3D, Polygon3D #45

pd-nisse commented Nov 2, 2021 •

edited by quincy-kh-chen

Loading

quincy-kh-chen left a comment

quincy-kh-chen left a comment

yuta-tsuzuki-woven left a comment

pd-nisse commented Nov 5, 2021

quincy-kh-chen left a comment

yuta-tsuzuki-woven left a comment

ChaoFang-TRI left a comment

quincy-kh-chen left a comment

pd-nisse commented Nov 19, 2021

quincy-kh-chen left a comment

quincy-kh-chen left a comment

chrisochoatri left a comment

akira-wakatsuki commented Feb 2, 2022

nehalmamgain commented Apr 25, 2022

tk-woven left a comment

tk-woven Jul 29, 2022

tk-woven Jul 29, 2022

tk-woven Jul 29, 2022

nehalmamgain commented Aug 1, 2022

schema: add KeyPoint3D, KeyLine3D, Polygon3D #45

Are you sure you want to change the base?

schema: add KeyPoint3D, KeyLine3D, Polygon3D #45

Conversation

pd-nisse commented Nov 2, 2021 • edited by quincy-kh-chen Loading

quincy-kh-chen left a comment

Choose a reason for hiding this comment

quincy-kh-chen left a comment

Choose a reason for hiding this comment

yuta-tsuzuki-woven left a comment

Choose a reason for hiding this comment

pd-nisse commented Nov 5, 2021

quincy-kh-chen left a comment

Choose a reason for hiding this comment

yuta-tsuzuki-woven left a comment

Choose a reason for hiding this comment

ChaoFang-TRI left a comment

Choose a reason for hiding this comment

quincy-kh-chen left a comment

Choose a reason for hiding this comment

pd-nisse commented Nov 19, 2021

quincy-kh-chen left a comment

Choose a reason for hiding this comment

quincy-kh-chen left a comment

Choose a reason for hiding this comment

chrisochoatri left a comment

Choose a reason for hiding this comment

akira-wakatsuki commented Feb 2, 2022

nehalmamgain commented Apr 25, 2022

tk-woven left a comment

Choose a reason for hiding this comment

tk-woven Jul 29, 2022

Choose a reason for hiding this comment

tk-woven Jul 29, 2022

Choose a reason for hiding this comment

tk-woven Jul 29, 2022

Choose a reason for hiding this comment

nehalmamgain commented Aug 1, 2022

pd-nisse commented Nov 2, 2021 •

edited by quincy-kh-chen

Loading