Don't keep pointer to QgsPointCloudLayer in QgsPointCloudLayerRenderer and QgsPointCloudLayerProfileGenerator

[dvdkon]

Description

This is a quick change that refactors a small problem in QgsPointCloudLayerRenderer. Previously render() used a saved QgsPointCloudLayer object (and called methods on it and QgsPointCloudDataProvider, generating warnings), but that object is not thread safe. Instead we now get everything we need up-front and only keep a refcounted pointer to the thread-safe layer index.

EDIT: I've also made a similar change to QgsPointCloudLayerProfileGenerator, since it was pointed out it had the same issue.

[github-actions]

🪟 Windows builds

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

🪟 Windows Qt6 builds

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

[github-actions]

Tests failed for Qt 5

One or more tests failed using the build from commit a3866f4

classified_render_edit_1 (testModifyAttributeValue)

classified_render_edit_1

Test failed at testModifyAttributeValue at tests/src/core/testqgspointcloudediting.cpp:193

Rendered image did not match tests/testdata/control_images/pointcloud_editing/expected_classified_render_edit_1/expected_classified_render_edit_1.png (found 490 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.

[github-actions]

Tests failed for Qt 6

One or more tests failed using the build from commit a3866f4

classified_render_edit_1 (testModifyAttributeValue)

classified_render_edit_1

Test failed at testModifyAttributeValue at tests/src/core/testqgspointcloudediting.cpp:193

Rendered image did not match tests/testdata/control_images/pointcloud_editing/expected_classified_render_edit_1/expected_classified_render_edit_1.png (found 490 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.

[uclaros]

Same change needs to be applied to the profile renderer too.
And probably then we can switch from QGIS_PROTECT_QOBJECT_THREAD_ACCESS_NON_FATAL to QGIS_PROTECT_QOBJECT_THREAD_ACCESS in QgsPointCloudLayer::dataProvider()

[uclaros]

Just curious, is there a benefit of using std::optional instead of just a QgsPointCloudIndex() / QgsPointCloudIndex( nullptr ), which evaluates as false?

[dvdkon]

Good catch. There isn't, I just personally default to using std::optional when a member can be explicitly left out, and don't much like putting nullptr in smart pointers.

It is redundant (and probably inconsistent with the rest of the codebase), so if you think it's better, I'll change it to the bare index object.

[uclaros]

That's a nice habit I guess!
It would be ever slightly more readable without it, as optional is barely used in the codebase, but I don't have a strong opinion :)

[nyalldawson]

I'd personally love to see optional used more often -- it's quite an elegant solution to some tricky API considerations... just so long as we don't need to worry about exposure to Python 🤣

[dvdkon]

Same change needs to be applied to the profile renderer too. And probably then we can switch from QGIS_PROTECT_QOBJECT_THREAD_ACCESS_NON_FATAL to QGIS_PROTECT_QOBJECT_THREAD_ACCESS in QgsPointCloudLayer::dataProvider()

Do you mean QgsPointCloudLayerProfileGenerator? As far as I can see, QgsProfilePlotRenderer doesn't keep any layer object. And in the generator it's more complicated, because it calls identify() on the data provider, which is also not thread-safe. So we can't really fix the issue in full by just shuffling objects around like in this PR.

[uclaros]

Do you mean QgsPointCloudLayerProfileGenerator? As far as I can see, QgsProfilePlotRenderer doesn't keep any layer object. And in the generator it's more complicated, because it calls identify() on the data provider, which is also not thread-safe. So we can't really fix the issue in full by just shuffling objects around like in this PR.

Yes, I mean QgsPointCloudLayerProfileGenerator::generateProfile(), where I believe the main issue lies, as it is called in separate threads for rendering (the comment there pin points the issue as the logic was copied from the 2d renderer).

We can ignore the identify() part for now, it is probably called from the main thread.

[dvdkon]

Yes, I mean QgsPointCloudLayerProfileGenerator::generateProfile(), where I believe the main issue lies, as it is called in separate threads for rendering (the comment there pin points the issue as the logic was copied from the 2d renderer).

Thanks, I see it now. I've fixed this method as well and made the dataProvider() method fail on non-main-thread access.

[uclaros]

Thanks, I see it now. I've fixed this method as well and made the dataProvider() method fail on non-main-thread access.

Nice!
You will also need to change layer->dataProvider()->index() to the new method layer->index(), which returns a different index (editing index) when layer is in editing mode.
Then that method can also change to the FATAL variant!

[dvdkon]

You will also need to change layer->dataProvider()->index() to the new method layer->index(), which returns a different index (editing index) when layer is in editing mode.

Thanks, I didn't notice that. The test now passes on my machine.

Then that method can also change to the FATAL variant!

Done.