You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
The text was updated successfully, but these errors were encountered:
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 qgis/PSC#58 (comment))
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 toQgsAbstractTiledSceneIndex
implementations.clean up
QgsPointCloudIndex
class APIQgsTiledSceneIndex
) 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.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.The text was updated successfully, but these errors were encountered: