add deleteVertices to QgsAbstractGeometry#63257
add deleteVertices to QgsAbstractGeometry#63257ViperMiniQ wants to merge 15 commits intoqgis:masterfrom
Conversation
🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. 🍎 MacOS Qt6 buildsDownload MacOS Qt6 builds of this PR for testing. |
|
I'll check on this shortly. I believe only some test cases need to be covered and apart from formatting issues, the rest should be good. |
|
The purpose of this PR is to clearly define the behavior when multiple vertices are deleted from a geometry, since not all geometries can be modified as easily, there are some rules we need/want to follow. This change also provides much better result when multiple vertices are deleted through the vertex tool. Currently, the vertex tool filters out the start and end points of polygons and removes selected points if the geometry would otherwise be deleted before reaching them. Another important aspect is the To test the vertex tool modifications, you will need the following commit (you can also review the parts of the code that will become obsolete in the vertex tool, as they will no longer be needed): If this PR is accepted and merged, that commit will also become a PR. With the vertex tool updated to use the Testing dataset: ne_10m_land.zip Test 1: Time: Test 2: Time: Apart from speed, we will save enough energy to train better AIs to replace us :) Creating a geometry in Python without registering it (this is important because this just modifies the geometry in place, so functions should have similar times).
Creating a layer and iterating its features calling
In current master, using the vertex tool is best being done on a single vertex, otherwise the tool might end up deleting more vertices than selected, there is no support for multiple vertices deletion on CompoundCurve geometries, for example (till now). Here is how the tool behaves on master vs the behavior with deleteVertices: Screencast_20251102_203328.webm Screencast_20251102_202711.webm That weird geometry if you want to play with it: :) |
|
Waiting for review. I see merge conflicts, but those are in auto generated .py files. |
|
Woah, we're halfway there... |
uclaros
left a comment
There was a problem hiding this comment.
This is a partial review, only for QgsAbstractGeometry, QgsGeometry and QgsLinestring.
Let's first decide on the final behavior regarding passing invalid vertex ids and what happens when a degenerate geometry is produced after a vertex deletion (fail vs clear geom).
| /** | ||
| * Deletes vertices within the geometry | ||
| * \param positions list of vertex ids for vertices to delete | ||
| * \returns TRUE if delete was successful |
There was a problem hiding this comment.
Can you clarify what makes a successful delete?
How are invalid vertex ids handled?
How are degenerate geometry results handled (ie deleting one vertex of a 2 vertex linestring will result in an empty geometry or will return false?
Also needs \since QGIS 4.0
There was a problem hiding this comment.
A successful deletion is when all the requested vertices are actually deleted (maybe some extra as well, to keep the geometry valid), but there is a difference between QgsAbstractGeometry and QgsGeometry.
If we reach a state where less than required vertices are present to form a valid geometry (linestring with less than 2 vertices, a polygon with less than 3), we clear the geometry at that point and return TRUE, same as in deleteVertex.
If any of the QgsAbstractGeometry vertices fails to be deleted, the method should return FALSE. This can potentially leave the geometry in an invalid state.
Invalid VertexIds are not handled at all in QgsAbstractGeometry, the same way they are not handled in the deleteVertex method. Were you thinking of a check to see if the requested VertexIds actually belong to the the geometry? For example, right now, neither deleteVertex or deleteVertices care for the part_id or ring_id in case of linestring, they only use the vertex bit of the VertexId.
Calling deleteVertices on a QgsGeometry should always produce a valid geometry – it internally calls the QgsAbstractGeometry::deleteVertices and if it gets a FALSE from it, it resets the geometry to whatever it was before the call (in QgsGeomtry::deleteVertices, we copy the geometry so we can reset to it if the deletion of any vertices fails).
eea069e to
f660167
Compare
…an one circularstring arc
fa6d5c9 to
b1ad58b
Compare
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
This PR implements
deleteVerticesforQgsAbstractGeometryclasses and is the first part of #62669.Unfortunately, some types of geometries can hold other types of geometries, so the method in QgsGeometry as tried in #62669 cannot properly handle all and that is why I decided for this approach.
Follow up PR will modify the QgsVertexTool to use the newly created functions which will avoid constant calls to detach and speed up vertices deletion on complex geometries as well as simplify the vertex tool delete function.
I need to add more tests for CoumpundCurve and CurvePolygon types before setting this PR as ready. Suggestions are welcome.
Even the special case of a "middle vertex" of a circularstring as added in #53960 is checked for.
(ping @uclaros and @3nids)