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.

Sign up for GitHub

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

Jump to bottom

Update to read intensity and visualization #3834

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Update to read intensity and visualization #3834

wants to merge 2 commits into from

Conversation

Leo-B-Y
Copy link

@Leo-B-Y Leo-B-Y commented Nov 24, 2023

What changes are proposed in this pull request?

  1. update three to support intensity field parse in pcd
  2. Determine if intensity field exists, if not then use color.r for ShadeByIntensity

How is this patch tested? If it is not, please explain why.

test local with pcd file which has intensity field

# .PCD v0.7 - Point Cloud Data file format
VERSION 0.7
FIELDS x y z intensity timestamp ring
SIZE 4 4 4 4 8 2
TYPE F F F F F U
COUNT 1 1 1 1 1 1
WIDTH 201191
HEIGHT 1
VIEWPOINT 0 0 0 1 0 0 0
POINTS 201191
DATA binary_compressed

image

Release Notes

Is this a user-facing change that should be mentioned in the release notes?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release
    notes for FiftyOne users.

There is almost no impact on existing users, if pcd doesn't have an intensity field it will continue to use color.r for rendering, maybe the documentation section needs to be updated a bit

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

@ritch ritch added the app Issues related to App features label Nov 27, 2023
ritch
ritch requested changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@ritch ritch left a comment

Choose a reason for hiding this comment

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

A couple things are needed for us to the land this change and release it. I think we can help with some of these:

  1. if you can fix the issue with remapping the intensity (or propose an alternative)
  2. we need to add this new support to the documentation, we can help with this
  3. regression test the upgrade of three.js
  4. regression test this change with other PCD files

Thanks for the PR!

@ritch
Copy link
Contributor

ritch commented Nov 27, 2023

We might be able to help with all the items I've mentioned in my comments. Thanks again for the contribution.

@Leo-B-Y
Copy link
Author

Leo-B-Y commented Nov 28, 2023

I apologize for the build failure, I didn't run the yarn command in codespace, the yarn.lock file has been updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ritch ritch ritch requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
app Issues related to App features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Leo-B-Y @ritch