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

[Feature] Faster Point Cloud Visualizers #534

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

Conversation

TomaszTB
Copy link
Contributor

This PR introduces an RDXRawImagePointCloudVisualizer which uses the RDXRawImagePointCloudRenderer. This visualizer also matches up color and depth images based on acquisition time to eliminate de-sync between the two.

This PR also replaces the old RDXROS2ColoredPointCloudVisualizer with a new RDXROS2RawImagePointCloudVisualizer.

Some other helpful methods are added to TimeTools and ImageMessageDecoder.

Video of the new visualizer:

2024-12-13.15-36-00.mp4

Comment on lines 112 to 114
colorToDepthTransform.setAndInvert(colorImage.getPose());
colorToDepthTransform.multiply(depthPose);
LibGDXTools.toLibGDX(colorToDepthTransform, tempColorTransform, libGDXColorTransform);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes a transform issue. Seems to me that we're using a color to depth transform, not depth to color.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This guy replaces the RDXROS2ColoredPointCloudVisualizer. I'm not 100% certain about the name. What do we think?

Copy link
Member

Choose a reason for hiding this comment

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

What about removing the RawImage part of the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's what I was thinking too. It uses the RDXRawImagePointCloudVisualizer, but the API doesn't expose any RawImages so I think it makes sense to remove that part of the name. I'll do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh but RDXROS2PointCloudVisualizer already exists. Should I just steal the RDXROS2ColoredPointCloudVisualizer name instead?

Copy link
Member

Choose a reason for hiding this comment

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

lol sure

@@ -31,6 +39,30 @@ public ImageMessageDecoder()
}
}

public RawImage decodeMessage(ImageMessage messageToDecode)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why I didn't make a method that returns a RawImage in the first place. Only dilemma I have is whether to use a CPU or GPU mat. I went with CPU since this class is used on the UI side. Should I add a method that uses GPU mat too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think GPU is needed

Copy link
Member

Choose a reason for hiding this comment

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

I guess in the method name you should put CPU

@@ -101,7 +101,7 @@ void main()
#ifdef INPUT_COLOR_IMAGE
else if (u_coloringMethod == COLOR_FROM_IMAGE)
{
vec3 colorFramePoint = transformPoint3D(depthFramePoint, u_depthToColorTransform);
vec3 colorFramePoint = transformPoint3D(depthFramePoint, u_colorToDepthTransform);
Copy link
Member

Choose a reason for hiding this comment

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

I think you've got it backwards. You're converting the point from depth frame to color frame. The transform that does that should be called depthToColorTransform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm that's true. I'm bad at this stuff so I really don't understand what I'm doing here hahaha. I'll change the names back

Copy link
Member

Choose a reason for hiding this comment

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

I can explain on the whiteboard whenever

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.

3 participants