Skip to content

feat: add color to point cloud display#961

Merged
samsja merged 26 commits intomainfrom
feat-display-colored-point-cloud
Jan 6, 2023
Merged

feat: add color to point cloud display#961
samsja merged 26 commits intomainfrom
feat-display-colored-point-cloud

Conversation

@anna-charlotte
Copy link
Copy Markdown
Contributor

@anna-charlotte anna-charlotte commented Dec 27, 2022

Signed-off-by: anna-charlotte [email protected]

When displaying a point cloud, we want to additionally consider its colors, if those are available.
The point cloud tensor is stored in a Documents .tensor field. If colors are available store them in a chunk Document with tags['name'] = 'point_cloud_colors'. The chunks tensor must be of shape (n_points, 3) or (n_points, 4), with a corresponding color for each point.

  • check for point_colors in display_point_cloud_tensor()
  • check and update documentation, if required. See guide

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 27, 2022

Codecov Report

Base: 83.50% // Head: 85.18% // Increases project coverage by +1.67% 🎉

Coverage data is based on head (1ec6896) compared to base (b5977a2).
Patch coverage: 57.14% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #961      +/-   ##
==========================================
+ Coverage   83.50%   85.18%   +1.67%     
==========================================
  Files         155      155              
  Lines        8051     8057       +6     
==========================================
+ Hits         6723     6863     +140     
+ Misses       1328     1194     -134     
Flag Coverage Δ
docarray 85.18% <57.14%> (+1.67%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
docarray/document/mixins/plot.py 65.48% <25.00%> (-0.89%) ⬇️
docarray/document/mixins/mesh.py 98.36% <100.00%> (+0.08%) ⬆️
docarray/array/mixins/io/binary.py 99.35% <0.00%> (+1.29%) ⬆️
docarray/array/mixins/io/pushpull.py 93.16% <0.00%> (+1.70%) ⬆️
docarray/array/mixins/getattr.py 81.81% <0.00%> (+3.03%) ⬆️
docarray/helper.py 82.75% <0.00%> (+3.06%) ⬆️
docarray/array/mixins/io/csv.py 89.74% <0.00%> (+17.94%) ⬆️
docarray/array/mixins/plot.py 67.96% <0.00%> (+39.82%) ⬆️
docarray/document/mixins/text.py 98.00% <0.00%> (+42.00%) ⬆️
docarray/array/mixins/text.py 100.00% <0.00%> (+50.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: anna-charlotte <[email protected]>
anna-charlotte added 3 commits December 27, 2022 11:02
Signed-off-by: anna-charlotte <[email protected]>
@anna-charlotte anna-charlotte force-pushed the feat-display-colored-point-cloud branch from ba8801e to 5c03bd7 Compare December 27, 2022 10:42
Signed-off-by: anna-charlotte <[email protected]>
@anna-charlotte anna-charlotte marked this pull request as ready for review December 27, 2022 13:00
Copy link
Copy Markdown
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

add a test to show that at least it does not break

Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
@anna-charlotte anna-charlotte force-pushed the feat-display-colored-point-cloud branch from d2eb4b4 to e6fd102 Compare December 28, 2022 14:50
anna-charlotte added 2 commits December 28, 2022 20:21
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
anna-charlotte added 2 commits December 28, 2022 21:01
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
@anna-charlotte
Copy link
Copy Markdown
Contributor Author

add a test to show that at least it does not break

@Joan I added a smoke test for the display function but I am getting this pyglet.canvas.xlib.NoSuchDisplayException, see here. Locally it works. I guess its because the .display() wants to open an extra display window with the interactive point cloud. Do you have an idea on how to solve this, such as suppressing such windows from opening?

anna-charlotte added 2 commits December 29, 2022 08:32
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
@anna-charlotte
Copy link
Copy Markdown
Contributor Author

anna-charlotte commented Jan 2, 2023

Update on test: removed test for display function again, because it always failed in the CI due to the pyglet window not being able to open when running in the CI, while passing locally

Copy link
Copy Markdown
Member

@JohannesMessner JohannesMessner left a comment

Choose a reason for hiding this comment

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

Did you remove the test that you mention in the comment here?

colors = np.tile(np.array([0, 0, 0]), (len(self.tensor), 1))
for chunk in self.chunks:
if (
'name' in chunk.tags.keys()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is 'name' a key that is already established in other parts of the code / other features? If not I think I would prefer a more descriptive key here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We used 'name' for the vertices and faces chunks, too. I think it makes sense to use the same for both of them, to keep it simple for the user. Do you have a suggestion for what you would prefer over 'name'?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no sounds good, let's keep it consistent

anna-charlotte and others added 4 commits January 2, 2023 11:17
Co-authored-by: samsja <[email protected]>
Signed-off-by: Charlotte Gerhaher <[email protected]>
@anna-charlotte
Copy link
Copy Markdown
Contributor Author

Did you remove the test that you mention in the comment here?

@JohannesMessner Yes, removed it

Signed-off-by: anna-charlotte <[email protected]>
@anna-charlotte anna-charlotte force-pushed the feat-display-colored-point-cloud branch from e514334 to 1ec6896 Compare January 6, 2023 09:15
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 6, 2023

📝 Docs are deployed on https://ft-feat-display-colored-point-cloud--jina-docs.netlify.app 🎉

@samsja samsja merged commit 3e7f514 into main Jan 6, 2023
@samsja samsja deleted the feat-display-colored-point-cloud branch January 6, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants