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

Mesh Layer Rendering with Updated Canvas Fix #59987

Merged
merged 14 commits into from
Jan 17, 2025

Conversation

JanCaha
Copy link
Contributor

@JanCaha JanCaha commented Dec 20, 2024

Description

There was a regression in Mesh Layer rendering for scalar datasets in case of "Updated Canvas" extent. This PR fixes it and also fixes fixes issue with style setting panel that could cause QGIS crash (test case was for this was added).

cc @uclaros

@github-actions github-actions bot added this to the 3.42.0 milestone Dec 20, 2024
Copy link

github-actions bot commented Dec 20, 2024

🪟 Windows builds

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

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 018cc53)

…on (avoids double rendering of the mesh layer)
@@ -17184,12 +17184,6 @@ void QgisApp::handleRenderedLayerStatistics() const
QgsMeshLayer *meshLayer = qobject_cast<QgsMeshLayer *>( QgsProject::instance()->mapLayer( layerStatistics->layerId() ) );
if ( meshLayer )
{
QgsMeshRendererSettings rendererSettings = meshLayer->rendererSettings();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this logic applied now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is applied in step of renderer creation. Basically, this part of the logic was applied twice, which caused issue (double rendering).

There is significant difference between mesh and raster renderer - mesh renderer is not stored at all, only the settings and mesh renderer is always created when it is needed. Thus there is no refresh of it.

I am not 100% sure that I properly understand the approach that was introduced with QgsRenderedLayerStatistics. Should the layer statistics be only applied on the renderer from this part of the code? And if rendered from anywhere else, just stick with data that are already set in mesh render settings?

@JanCaha
Copy link
Contributor Author

JanCaha commented Dec 31, 2024

@uclaros

I updated this to follow the use of QgsRasterLayerRenderer as closely as possible.

On creation of the renderer the QgsRenderedLayerStatistics is calculated but renderer settings are not updated with min max values. The update of renderer is only done from handleRenderedLayerStatistics() in QgisApp. This called from QgisApp::onRenderComplete(), which means that two rendering passes happen - first with old min max settings and after that second one, that is triggered from handleRenderedLayerStatistics(). This exactly follows steps for rendering Raster Layer.

This should fix the rendering pipeline, to be correct.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 15, 2025
@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 15, 2025
@qgis qgis deleted a comment from github-actions bot Jan 15, 2025
triggerRepaint();
if ( repaint )
{
emit rendererChanged();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to skip emitting the signal too?
If we keep it we can skip the emit meshLayer->legendChanged(); from qgisapp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that changed those. This signal is raised and the legendChanged does not

@uclaros uclaros merged commit 5fc5503 into qgis:master Jan 17, 2025
31 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.

3 participants