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

Further improve 3D Map View performance #60672

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

Conversation

dvdkon
Copy link
Contributor

@dvdkon dvdkon commented Feb 19, 2025

Description

This PR comprises two changes to the 3D view:

  • Decoupling entity visibility from the currently set near/far planes: Currently, which entities are visible depends on the near/far planes. However, adding/removing entities will likely change the near/far planes that are used as input. Currently, we just re-run the scene update once, but to do this properly we'd need to loop until the planes stabilise.

    I instead propose not caring about the planes when selecting entities (by just choosing small/large enough constants), which is where the current process would converge to after enough frames anyway. This way, we get rid of the second (expensive) updateScene() call per frame, almost doubling frame rate, and reduce complexity as a bonus.

  • Early culling in QgsVirtualPointCloudEntity: On each scene update, the VPC entity iterates over its constituent point clouds and calls their handleSceneUpdate() method. That will eventually do frustum culling somewhere in QgsChunkedEntity, but we can get a significant speedup if we do the frustum culling on whole point clouds already in the VPC entity.

@github-actions github-actions bot added this to the 3.42.0 milestone Feb 19, 2025
Copy link

github-actions bot commented Feb 20, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 7be4280)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 7be4280)

Copy link

github-actions bot commented Feb 20, 2025

Tests failed for Qt 6

One or more tests failed using the build from commit f702cb4

4978_line_rendering_1 (testEpsg4978LineRendering)

4978_line_rendering_1

Test failed at testEpsg4978LineRendering at tests/src/3d/testqgs3drendering.cpp:1828

Rendered image did not match tests/testdata/control_images/3d/expected_4978_line_rendering_1/expected_4978_line_rendering_1.png (found 100268 pixels different)

The full test report (included comparison of rendered vs expected images) can be found here.

Further documentation on the QGIS test infrastructure can be found in the Developer's Guide.

Copy link

github-actions bot commented Feb 20, 2025

Tests failed for Qt 5

One or more tests failed using the build from commit f702cb4

4978_line_rendering_1 (testEpsg4978LineRendering)

4978_line_rendering_1

Test failed at testEpsg4978LineRendering at tests/src/3d/testqgs3drendering.cpp:1828

Rendered image did not match tests/testdata/control_images/3d/expected_4978_line_rendering_1/expected_4978_line_rendering_1.png (found 100268 pixels different)

The full test report (included comparison of rendered vs expected images) can be found here.

Further documentation on the QGIS test infrastructure can be found in the Developer's Guide.

@nyalldawson
Copy link
Collaborator

@dvdkon i'm not sure if it's related to the previous set of optimisations, but on current master there's a regression in 3d movement when using the mouse wheel to zoom in -- if a small zoom is performed quickly, the map will zoom in a HUGE amount. The same amount of movement zooming out works correctly.

@nyalldawson nyalldawson added the Frozen Feature freeze - Do not merge! label Feb 20, 2025
@uclaros
Copy link
Contributor

uclaros commented Feb 21, 2025

I can confirm that without #60246 zooming in works as usual

@nyalldawson
Copy link
Collaborator

@uclaros should we revert #60246 before today's release? It's an optimisation only, and causes a regression

@uclaros
Copy link
Contributor

uclaros commented Feb 21, 2025

Yes, I think we should. We can reintroduce it later with a fix

@nyalldawson nyalldawson removed the Frozen Feature freeze - Do not merge! label Feb 21, 2025
Done by adding frustum culling early in the loop, results in a ~10x
speedup in one project.
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