Skip to content

Remove QgsAbstractGeometry deleteVertex implementations - #67307

Draft
ViperMiniQ wants to merge 1 commit into
qgis:masterfrom
ViperMiniQ:delete-deletevertex
Draft

ViperMiniQ wants to merge 1 commit into
qgis:masterfrom
ViperMiniQ:delete-deletevertex

Conversation

@ViperMiniQ

@ViperMiniQ ViperMiniQ commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

deleteVertex now uses deleteVertices( [vertex] )

needs #67306 and #67294

@github-actions github-actions Bot added this to the 4.4.0 milestone Sep 6, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

🍎 MacOS Qt6 builds

Download MacOS Qt6 builds of this PR for testing.
This app is not notarized, run sudo xattr -d com.apple.quarantine /Applications/QGIS*.app to avoid the warning
(Built from commit be0f8f6)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
To execute locally, unzip the downloaded zip file and run bin\qgis-bin.exe in the extracted directory.
You might be prompted by Windows Defender click "Run anyway"
(Built from commit be0f8f6)

geom = QgsGeometry.fromWkt(wkt)
assert geom.deleteVertex(3)
expected_wkt = "CurvePolygon (CompoundCurve (CircularString (0 0, 1 1, 2 0),(2 0, 1 -1),(1 -1, 0 0)))"
expected_wkt = "CurvePolygon (CompoundCurve (CircularString (0 0, 1 1, 2 0),(2 0, 1 -1, 0 0)))"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because neighboring same type curves get merged:

condenseCurves(); // merge consecutive LineStrings and CircularStrings

@ViperMiniQ

ViperMiniQ commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

I need help defining this.

There are a couple test cases to adjust, which concern the behavior of deleteVertices.
So what is different between it and deleteVertex and why do tests fail?

deleteVertices() calls condenseCurves() unconditionally, while deleteVertex() called that method only when a shared vertex of 2 curves was being deleted.
What that method does, it just merges neighboring curves of the same type.
So if you have a CC( LS, LS, CS, CS, CS, LS ), calling it would produce CC( LS, CS, LS ).

I can only take a guess why this was introduced in deleteVertex(). At the time, I just copied it over, not being overly familiar with CC at the time.

Deleting a shared vertex of 2 CSes would produces a LS consisting of endpoints of those two CCes. My guess is condenseCurves() was being called at this point to get rid of that new curve and merge it with the rest. Trouble is, it was never merging just that curve, but all. This was introduced in #57628

I see a few options here:
1.) we call condenseCurves() unconditionally (current deleteVertices behavior)
2.) we call condenseCurves() only after a shared vertex deletion (current deleteVertex behavior)
3.) we never call condenseCurves()
4.) we handle the cases when we are creating and adding new curves and we merge them with the curve that comes prior (only if of the same type)

Taking a better look at this, my vote is for 3 and that is not because 4 is extra work.
With 4, adding vertices to existing curves in a method called delete vertices does not make sense. We cannot go around about addition of new curves, but we preserve the deletion behavior with this.

Example:
Consider CompoundCurve( CircularString( 0 0, 1 1, 2 0), CircularString( 2 0, 3 1, 4 0 ), LineString( 4 0, 5 0, 6 0 )).

If the first action is to delete the shared vertex of the first two curves (at 2 0), they would collapse into a LS and be merged with the LS at the end. From there, there are no changes in behavior, LS vertex deletion is simple.
But if we start by deleting a vertex of the LS at the end (at 6 0), curves are condensed and now there is no deletion of a shared vertex if we want to delete the one at 2 0, because then it is just one vertex of a bigger CS. See screencasts for the above WKT:

Screencast_20260912_183354.webm
Screencast_20260912_183421.webm

@uclaros

uclaros commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

I can only take a guess why this was introduced in deleteVertex(). At the time, I just copied it over, not being overly familiar with CC at the time.

Deleting a shared vertex of 2 CSes would produces a LS consisting of endpoints of those two CCes. My guess is condenseCurves() was being called at this point to get rid of that new curve and merge it with the rest. Trouble is, it was never merging just that curve, but all. This was introduced in #57628

I agree, completely skipping the condenseCurves() makes more sense, now that the new deletion logic can handle it.
Maybe @lbartoletti or @Djedouas can confirm that adding condenseCurves() was an artifact of fixing #57628 and not really required behavior.

@Djedouas

Copy link
Copy Markdown
Member

Skipping the condenseCurves() makes more sense to me too

@ViperMiniQ

Copy link
Copy Markdown
Contributor Author

Thanks @Djedouas!

I'll do a separate PR for this and then cleanup this branch, so that it is just removing deleteVertex implementations and not adjusting any tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants