-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Point cloud editing fixes #60237
base: master
Are you sure you want to change the base?
Point cloud editing fixes #60237
Conversation
8b0cdfe
to
395652a
Compare
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add auto test for the ignoreIndexFilter flag, and for change attribute value API when there's a subset string applied?
@@ -44,6 +44,7 @@ QgsPointCloudEditingIndex::QgsPointCloudEditingIndex( QgsPointCloudLayer *layer | |||
mZMax = mIndex.zMax(); | |||
mRootBounds = mIndex.rootNodeBounds(); | |||
mSpan = mIndex.span(); | |||
mFilterExpression = mIndex.subsetString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the filter expression changes while we're in editing mode - is this going to be refreshed? I would prefer not to have mFilterExpression here and rather get it when needed from mIndex
@@ -166,7 +166,7 @@ std::unique_ptr<QgsPointCloudBlock> QgsCopcPointCloudIndex::nodeData( const QgsP | |||
// we need to create a copy of the expression to pass to the decoder | |||
// as the same QgsPointCloudExpression object mighgt be concurrently | |||
// used on another thread, for example in a 3d view | |||
QgsPointCloudExpression filterExpression = mFilterExpression; | |||
QgsPointCloudExpression filterExpression = request.ignoreIndexFilterEnabled() ? QgsPointCloudExpression() : mFilterExpression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's implement this for ept point cloud index (just to be sure it respects the api)
// TODO: apply filtering (if any) | ||
request.setAttributes( pcIndex.attributes() ); | ||
request.setAttributes( attributes ); | ||
request.setIgnoreIndexFilterEnabled( true ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add a comment why we are ignoring the index filter? to get all points so we have the correct point indexes within the node (and it could also go to docstring for setIgnoreIndexFilterEnabled() to clarify when one would want to use it)
Description
This PR fixes point selection for the following cases: