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

[3D] Fix reset view button #60413

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

Conversation

Withalion
Copy link
Contributor

Description

This PR fixes reset view button giving wrong camera positions for virtual point clouds, and also fixes a bug where resetting view multiple times would set near & far planes incorrectly. Also it fixes initial position setting of camera after opening 3D view, which should be set to top down view of whole scene.

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

github-actions bot commented Feb 3, 2025

🪟 Windows builds

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

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 0995c8d)

Copy link

github-actions bot commented Feb 3, 2025

Tests failed for Qt 6

One or more tests failed using the build from commit 7524667

virtual_pointcloud_3d_overview (testPointCloud3DOverview)

virtual_pointcloud_3d_overview

Test failed at testPointCloud3DOverview at tests/src/3d/testqgspointcloud3drendering.cpp:603

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 3, 2025

Tests failed for Qt 5

One or more tests failed using the build from commit 7524667

virtual_pointcloud_3d_overview (testPointCloud3DOverview)

virtual_pointcloud_3d_overview

Test failed at testPointCloud3DOverview at tests/src/3d/testqgspointcloud3drendering.cpp:603

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.

mCamera->setFarPlane( distance * 2 );
// we force the updateCameraNearFarPlanes() in Qgs3DMapScene to properly set the planes
// by making sure the cameraPose is never the same, which will emit cameraChanged()
mCameraPose.setDistanceFromCenterPoint( camPose.distanceFromCenterPoint() + 1 );
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just emit cameraChanged() directly here, after calling setCameraPos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to call updateCameraFromPose() directly, which updates QCameraand emits the signal

@Withalion
Copy link
Contributor Author

Withalion commented Feb 4, 2025

To correct myself, the feature to open the 3D scene in the same place as 2D scene is currently looking at stays. It just fixes the bug for smaller scenes where this "zoom" would move camera to point to empty space.
EDIT: fixes only reset view button not the initial position bug

@Withalion Withalion changed the title [3D] Fix reset view button & initial position [3D] Fix reset view button Feb 5, 2025
Comment on lines +183 to +184
mCameraPose = camPose;
updateCameraFromPose();
Copy link
Contributor

Choose a reason for hiding this comment

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

why not calling setCameraPose( camPose ); and add a call to updateCameraFromPose() in it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setCameraPose( camPose ) already calls updateCameraFromPose(), however the issue was that the setCameraPose( camPose ) would return early as camPose was the same and the near & far plane would not be recalculated according to layers

Comment on lines +143 to +144
zMin = virtualProvider->subIndexes()[0].zRange().lower();
zMax = virtualProvider->subIndexes()[0].zRange().upper();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why check against [0] first? all sub indexes will be checked in the iteration below.
([0] could also be out of range for some weird bad vpc file!)

zMax = virtualProvider->subIndexes()[0].zRange().upper();
for ( QgsPointCloudSubIndex subIndex : virtualProvider->subIndexes() )
{
const QgsDoubleRange newRange = subIndex.zRange();
Copy link
Contributor

Choose a reason for hiding this comment

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

While not very probable, subIndex.zRange() may be also a default constructed infinite range if the vpc has 4 coordinate instead of 6 coordinate bboxes.
These infinite range subindexes should be skipped or the zMin/zMax doubles will overflow after the multiplication later on.

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.

4 participants