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

Putting glsl 1.50 resources back in RenderSystem #668

Merged
merged 4 commits into from
May 5, 2021

Conversation

pijaro
Copy link
Contributor

@pijaro pijaro commented Apr 8, 2021

GLSL version 1.50 resources were left commented out in RenderSystem class which resulted in performance hit in pointcloud rendering. #46 mentions this but was left behind for a long time now (2017).

I tested putting back 1.50 locally and it worked fine on ubuntu 20.04. Therefore I think it is safe to assume that this is no longer an issue.

Presented PR puts back glsl 1.50 resources and updates corresponding unit tests. This improves pointcloud rendering a lot (on my machine it is about 200% for 21mln pointcloud while rendering with primitives).

This may also fix #662

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
@pijaro pijaro changed the title Putting back glsl 1.50 resources back in RenderSystem Putting glsl 1.50 resources back in RenderSystem Apr 8, 2021
@pijaro
Copy link
Contributor Author

pijaro commented Apr 14, 2021

@jacobperron can you take a look at this PR, please?

@adamdbrw
Copy link

@mjeronimo this one improves performance by a lot with point clouds and should be a quick one to review, could you take a look?

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
Copy link
Contributor

@mjeronimo mjeronimo left a comment

Choose a reason for hiding this comment

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

I think this is OK as a first step. Going forward, I'd rather see these differences encapsulated into an object with methods such as get_size_of_single_vertex and get_num_vertices_per_box, for example.

@jacobperron
Copy link
Member

@pijaro Does this resolve #670, or is that a separate issue?

@adamdbrw
Copy link

@jacobperron @pijaro AFAIK this is a separate issue

@pijaro
Copy link
Contributor Author

pijaro commented Apr 26, 2021

@jacobperron @adamdbrw That's correct, #670 is a separate issue.

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron
Copy link
Member

@ros-pull-request-builder retest this please

Signed-off-by: Piotr Jaroszek <piotr.jaroszek@robotec.ai>
@pijaro
Copy link
Contributor Author

pijaro commented May 5, 2021

I just committed a fix for windows.

@ros-pull-request-builder retest this please

@jacobperron
Copy link
Member

@pijaro Thanks! Sorry, I meant to follow-up on that.

Windows: Build Status

@jacobperron jacobperron merged commit c74ca56 into ros2:ros2 May 5, 2021
@adamdbrw
Copy link

adamdbrw commented May 6, 2021

@jacobperron Would this be a good candidate for a Foxy backport?

@pijaro pijaro mentioned this pull request May 6, 2021
@jacobperron
Copy link
Member

@adamdbrw It looks like there is an issue with this patch (#681). I think it'd be better to let this soak on the ros2 branch, and then backport to Galactic. After it's had some time to soak in Galactic, I'd be open to backporting to Foxy.

@jacobperron jacobperron added this to In progress in Galactic via automation May 7, 2021
@jacobperron jacobperron removed this from In progress in Galactic May 7, 2021
@jacobperron jacobperron added this to Proposed in Galactic Patch Release 1 via automation May 7, 2021
@clalancette clalancette moved this from Proposed to Needs backport in Galactic Patch Release 1 Jun 1, 2021
@cottsay cottsay removed this from Needs backport in Galactic Patch Release 1 Sep 21, 2021
@cottsay cottsay added this to Needs backport in Galactic Patch Release 2 Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Low FPS in PointCloud2 visualization
4 participants