Skip to content
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

PyQGIS QgsPolygon creation crashes #51978

Closed
2 tasks done
moovida opened this issue Feb 22, 2023 · 8 comments
Closed
2 tasks done

PyQGIS QgsPolygon creation crashes #51978

moovida opened this issue Feb 22, 2023 · 8 comments
Assignees
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption PyQGIS Related to the PyQGIS API

Comments

@moovida
Copy link

moovida commented Feb 22, 2023

What is the bug or the crash?

When running the following pyQGIS script in the python editor, QGIS runs properly the first time, but then crashes if the script is re-run.

def listToPoints(coords):
    points = []
    for coord in coords:
        points.append(QgsPoint(coord[0], coord[1]))
    return points
    
exteriorPoints = listToPoints([[35,10],[10,20],[15,40],[45,45],[35,10]])
holePoints = listToPoints([[20,30],[35,35],[30,20],[20,30]])
exteriorRing = QgsLineString(exteriorPoints)
holeRing = QgsLineString(holePoints)
polygonWithHole = QgsPolygon(exteriorRing, rings=[holeRing])

vl = QgsVectorLayer("Polygon", "tmppoly", "memory")
f = QgsFeature()
f.setGeometry(polygonWithHole)
pr = vl.dataProvider()
pr.addFeatures([f])
QgsProject.instance().addMapLayer(vl)

Steps to reproduce the issue

  1. create a new empty project (epsg:4326)
  2. open the python editor
  3. paste the above script into the editor
  4. run the script
  5. remove the tmppoly layer that has just been created
  6. re-run the script
  7. QGIS freezes for about 2-3 seconds, then crashes
  8. no error messages of any kind are given

Versions

<style type="text/css"> p, li { white-space: pre-wrap; } </style>
QGIS version 3.28.3-Firenze QGIS code revision c12bcb2
Qt version 5.15.3
Python version 3.10.6
GDAL/OGR version 3.4.1
PROJ version 8.2.1
EPSG Registry database version v10.041 (2021-12-03)
GEOS version 3.10.2-CAPI-1.16.0
SQLite version 3.37.2
PDAL version 2.3.0
PostgreSQL client version unknown
SpatiaLite version 5.0.1
QWT version 6.1.4
QScintilla2 version 2.11.6
OS version Ubuntu 22.04.2 LTS
       
Active Python plugins
sagaprovider 2.12.99
db_manager 0.1.20
processing 2.12.99
grassprovider 2.12.99
MetaSearch 0.3.6
QGIS version 3.28.3-Firenze QGIS code revision [c12bcb2](https://github.com/qgis/QGIS/commit/c12bcb2f76) Qt version 5.15.3 Python version 3.10.6 GDAL/OGR version 3.4.1 PROJ version 8.2.1 EPSG Registry database version v10.041 (2021-12-03) GEOS version 3.10.2-CAPI-1.16.0 SQLite version 3.37.2 PDAL version 2.3.0 PostgreSQL client version unknown SpatiaLite version 5.0.1 QWT version 6.1.4 QScintilla2 version 2.11.6 OS version Ubuntu 22.04.2 LTS

Active Python plugins
sagaprovider
2.12.99
db_manager
0.1.20
processing
2.12.99
grassprovider
2.12.99
MetaSearch
0.3.6

Supported QGIS version

  • I'm running a supported QGIS version according to the roadmap.

New profile

  • I tried with a new QGIS profile

Additional context

No response

@moovida moovida added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Feb 22, 2023
@YoannQDQ YoannQDQ added PyQGIS Related to the PyQGIS API Crash/Data Corruption labels Feb 22, 2023
@nicogodet
Copy link
Member

nicogodet commented Feb 22, 2023

Can't reproduce on Linux
Read to fast, crash on windows too

@elpaso
Copy link
Contributor

elpaso commented Feb 23, 2023

Ownership of rings elements of the list is not transferred.

    def testPolygonRings(self):
        """Test rings ownership transfer"""

        ring = QgsLineString([0.5, 0.5, 0.9, 0.9], [0.5, 0.9, 0.9, 0.5])
        p = QgsPolygon(QgsLineString([0, 0, 1, 1], [0, 1, 1, 0]), rings=[ring])
        del ring
        p.asWkb()  # crashes because ring is gone

@elpaso elpaso self-assigned this Feb 23, 2023
elpaso added a commit to elpaso/QGIS that referenced this issue Feb 23, 2023
@elpaso elpaso changed the title memory layer creation with small pyQGIS script leads to crash PyQGIS QgsPolygon creation crashes Feb 23, 2023
@moovida
Copy link
Author

moovida commented Feb 24, 2023

@elpaso , is there a way to work around this? Since this affects also the previous LTS version, I assume I am choosing a less common way to do this?
And also, does this only affect polygon creation?
Thank you

@elpaso
Copy link
Contributor

elpaso commented Feb 24, 2023

@elpaso , is there a way to work around this?

You can keep a python reference the individual rings in the array or construct the geometry from WKT.

Since this affects also the previous LTS version, I assume I am choosing a less common way to do this?

No idea, sorry.

And also, does this only affect polygon creation?

I didn't check other constructors but there are probably other similar issues in the bindings, I don't really know if this issue has surfaced now due to some changes in the SIP bindings generator or if it has been here forever.

I would have expected that when constructing an array of wrapped instances SIP would have increased the ref count but apparently it doesn't.

@nyalldawson
Copy link
Collaborator

there a way to work around this

Construct the polygon with the exterior ring only, then add interior rings one by one. That should work, since the issue is with lists of rings only.

@moovida
Copy link
Author

moovida commented Mar 1, 2023

I will add another one here, because I have the feeling that it is related. If not, let me know and I will open a different issue.

This code makes QGIS crash for me:

p = QgsGeometry(QgsPoint(4, 1))
collection = QgsGeometryCollection()
collection.addGeometry(p.constGet())
convexhull = QgsGeometry(collection).convexHull()

canvas = QgsMapCanvas()
r = QgsRubberBand(canvas, QgsWkbTypes.LineGeometry)
r.addGeometry(p)

PS: again, if someone knows a workaround, it is appreciated.

@nyalldawson
Copy link
Collaborator

@moovida

there's a bug in your code:

collection.addGeometry(p.constGet())

Here you are borrowing a reference to the geometry stored in p and trying to add it to collection, which transfers ownership of that geometry. At that point two objects own the point, so you'll get a crash whenever one of them is deleted.

You could update your code to:

collection.addGeometry(p.constGet().clone())

So that you aren't trying to transfer the borrowed point to the collection, but instead are adding a copy of it.

I agree that none of this is particularly nice -- it exposes too much of the underlying c++ memory model to Python, and is not expected for regular Python developers. There's no easy way around this for now though -- just be very cautious of any method in the api which is noted as transferring ownership.

@moovida
Copy link
Author

moovida commented Mar 2, 2023

Hi @nyalldawson , thanks for the explanation. Now things are getting clearer. I agree that the exposed C++ memory model is a bit of a problem (and counterintuitive if you come from other languages that allow to keep references of objects in multiple locations). I am trying to create a course for students that are not programmers, so it is ok. But the normal thing that will happen is that they will break things. But in this case it might mean that QGIS crashes. :-/

Anyways, good to know and maybe even the course I am preparing can help to document things a bit. The most important thing is the transferred ownership. Will keep that clear in mind.

I have a few questions about the choice of some classes hierarchy. What would be the best place to ask them? The normal QGIS list? The devel list? I didn't see any pyQGIS list, right?

@elpaso elpaso closed this as completed May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption PyQGIS Related to the PyQGIS API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants