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

Fix point cloud performance issue when points are filtered via "show" #12317

Merged
merged 4 commits into from
Nov 21, 2024

Conversation

connormanning
Copy link
Contributor

@connormanning connormanning commented Nov 19, 2024

Discard hidden points in the fragment shader, rather than moving them to 0 in the vertex shader. Fixes #11270.

Description

The problem

I was running into an issue when points from a 3D Tiles point cloud dataset were being filtered out via a show condition. For example showing only Ground points or only Noise points - the more points that were filtered away, the worse the performance got. Some investigation showed that this was only an issue on Apple silicon, and is most noticeable in Firefox. On my M2 MacBook with Firefox, the second link in #12140 slows my entire computer to a crawl until the tab is closed. I submitted that issue which was found to be a duplicate of #11270, which has some investigation and suggestions. Unfortunately the one-line fix described here had no effect for me.

Investigation details

The offending line is here, which multiplies the point position by show, in order to zero out the position when when the point should be hidden due to show == 0.

It seems that when the point position is set to 0 though, that something in the rendering pipeline degenerates significantly on Apple silicon.

Changes

  • Remove the repositioning of hidden points, which causes the issue (see note below)
  • Add a varying float v_pointCloudShow, populated in the vertex shader
  • When v_pointCloudShow is 0, discard the point in the fragment shader

It's not quite clear to my why the line gl_Position = show * positionClip still causes the issue even with the other changes applied. I'd have thought that since the point will be discarded immediately in the fragment shader, that this line could be left as-is. But as mentioned in the code comment there, this position adjustment must be removed to fix the performance issue.

That said, I just started learning about shaders a few hours ago to look into this issue, so I don't really know what any of this means. Suggestions are very welcome from anyone who knows what they're doing.

For example, is this the best place to perform the discard? Maybe somewhere around here would be more appropriate? There may easily be tens of thousands of points hidden in common scenarios, so I tried to discard them as early as possible.

Issue number and link

Testing plan

The fix should be tested both on hardware that is affected as well as hardware that is not affected by the original issue to make sure this doesn't have any side-effects on non-affected hardware.

I've built a bundle containing these changes, so people can compare their performance, making sure that the "fix" both fixes the issue and does not cause new ones. Open the below links, with only one open at a time, and try to drag the point cloud around in little circles to see the performance.

Here is an example of what it looks like if you are experiencing the issue (yes, it's a recording of a screen, but I can't capture it via screen recording since the computer itself is lagging):

cesium-lag-480.mov
  • Before - laggy (maybe) - this is a link to the official Cesium Sandcastle - if you click this and your computer lags, maybe to the point where it can be difficult to close the tab, then you are experiencing the issue. Note that you might not see any issues at all if you do not have the affected hardware, and it may vary per browser. Firefox seems to be the most noticeable.
  • After - fixed (hopefully) - the same Cesium setup but with this fix applied. It should show a pink point cloud that can be dragged around easily and should not lag your computer. If you were experiencing the issue in the first link, then this one should be noticeably better.
CPU/GPU Browser Before lags? After lags?
Apple M2 Max (integrated GPU) Firefox Yes No

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @connormanning! Welcome to the Cesium community!

In order for us to review your PR, please complete the following steps:

Review Pull Request Guidelines to make sure your PR gets accepted quickly.

@ggetz
Copy link
Contributor

ggetz commented Nov 20, 2024

Hi @connormanning, thank you for the proposed fix!

I can confirm we have a CLA on file for you, and we can proceed with reviewing this PR. 🎉

@ggetz
Copy link
Contributor

ggetz commented Nov 20, 2024

@lilleyse Would you mind taking a look and verifying the approach used here?

@lilleyse
Copy link
Contributor

lilleyse commented Nov 20, 2024

This looks great @connormanning.

For performance testing I ran a modified version of the sandcastle and didn't see any noticeable difference between this branch and main.

EDIT: tested on a non M1/M2 machine.

@lilleyse lilleyse added this pull request to the merge queue Nov 21, 2024
Merged via the queue into CesiumGS:main with commit b17154b Nov 21, 2024
4 checks passed
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.

Performance degradation with large point clouds and show condition on M1 Mac
3 participants