Clean up point cloud index and improve its thread safety

[wonder-sk]

QGIS Enhancement: Clean up point cloud index and improve its thread safety

Date 2024/03/14

Author Martin Dobias (@wonder-sk)

Contact wonder dot sk at gmail dot com

Version QGIS 3.38

Summary

Handling of hierarchies is one of the fundamental pieces of the point cloud implementation that makes sure that datasets with millions or billions of points can be visualized quickly. The time has shown that some parts of the code has weak points - to be more specific, QgsPointCloudIndex and its derived classes. It would be good to address these problems to make sure that the code is robust and maintainable in the future. This includes thread safety, better interface to fetch hierarchies and less code duplication.

Proposed Solution

These are the bits of code that need attention:

• thread safety - the QgsPointCloudIndex object may be simultaneously accessed both from the main thread and from worker threads. While some work has been done to use mutexes to prevent race conditions between threads, thread safety issues may still arise (e.g. map layer gets deleted in the main thread while another thread is accessing its index). We could adopt the same approach as designed for tiled scene index, where the index object QgsTiledSceneIndex is thread safe, implicitly shared object that forwards calls to QgsAbstractTiledSceneIndex implementations.

• clean up QgsPointCloudIndex class API

  • the class derives from QObject, although there is no need for that
  • match more closely the tiled scene index API (QgsTiledSceneIndex) calls - these two classes have very similar purpose (the main difference is that tiled scene layers allow more general hierarchies while point cloud layers only allow octree hierarchies), yet the APIs are quite different. This makes it more difficult to navigate and understand the code.
  • clean up access to statistics - there is now metadataStatistics() function to access all stats, but there are still other means to access stats that should be removed (metadataStatistic(), metadataClasses() and more)

• unify local/remote implementation EPT provider - the data provider for EPT point cloud format has two implementations - a "local" and a "remote" - based on whether data are accessed locally or from a remote server. The local implementation has different handling of hierarchy - it fetches the whole hierarchy at once, rather than doing lazy loading like the "remote" implementation does. This should be unified to use lazy loading in both cases, to make sure we have consistent behavior and less code duplication.

• clean up hierarchy fetching logic - the implementation is more complex than it needs to be, addressing also impossible cases and making the code complicated to follow (e.g. allow fetching of hierarchy for nodes where not even their parent nodes have hierarchy available yet)

If the time will allow, I would like to also make it possible to fetch hierarchy of point clouds in 3D views without freezing the user interface - this is already available for tiled scene layers (implementation of canCreateChildren() and prepareChildren() in QgsChunkLoaderFactory).

Deliverables

This work would be done in a series of pull requests that implement the proposed solution as mentioned above.

Risks

This is a low-risk task - all the major pieces of code are in place and they just need fixing to improve stability and code quality.

Performance Implications

There should be no performance implications.

Backwards Compatibility

There will be changes in the QgsPointCloudIndex interface, but this should be acceptable given that the class was intentionally marked as experimental.

[nyalldawson]

Sounds great to me!

[anitagraser]

Thank you for submitting your proposal to the 2024 QGIS Grant Programme. The 2 week discussion period starts today. At the end of the discussion, the proposal author has to provide a 3-line pitch of their proposal for the voter information material. (For an example from last year check https://github.com/qgis/PSC/issues/58#issuecomment-1567892412)