Bug report #7225

QGIS crashes after failed intersect removal

Added by Duncan Golicher over 6 years ago. Updated over 6 years ago.

Status:Closed
Priority:High
Assignee:-
Category:Vectors
Affected QGIS version:1.8.0 Regression?:No
Operating System:Windows/Ubuntu Easy fix?:No
Pull Request or Patch supplied:No Resolution:fixed
Crashes QGIS or corrupts data:Yes Copied to github as #:16256

Description

If a user attempts to use the avoid intersection feature when digitising over a polygon with topological errors the new polygon vanishes. This is frustrating for new users, as no explanation message is provided. The issue is mentioned here.
http://www.faunalia.com/content/bad-bad-polygon-fixing-it-quantum-gis-1
However it us rather more serious than suggested by this post.

If the user attempts to save the layer, with the phantom polygon, QGIS closes ungraciously under Windows or freezes up completely under Ubuntu. The bug has a major impact on usability, as it can lead to a lot of lost work and major frustrations.
Although it is of course avoidable by careful digitising, it is quite easy to introduce a minor error, such as clicking a vertex twice. This really should never lead to the program closing in this way. I link screenshots in which I explained the problem to the students affected and show how to work around it by cleaning up a layer using GRASS.
https://dl.dropbox.com/u/2703650/links/CleaningwithGRASS.pdf
The issue is serious enough to have led students to give up on QGIS and go back to Arc.

History

#1 Updated by Giovanni Manghi over 6 years ago

  • Status changed from Open to Feedback

This happens on QGIS 1.8 and master or just 1.8? Can you attach sample data? Can you post the error message you get in the terminal when qgis crashes on Linux?

#2 Updated by Giovanni Manghi over 6 years ago

  • Affected QGIS version changed from master to 1.8.0

From what I see in the PDF you are using qgis 1.8. Please try if with master the crash still happen, of yes please attach sample data. Beware that in qgis master we have a few regressions due to the many changes (for the best) being made, and right now GRASS vectors are affected by one of this regressions. Anyway you should be able to test if the standard qgis editing tools still cause the crash.

#3 Updated by Giovanni Manghi over 6 years ago

if this issue was caused by this

#5584

then it has been fixed in master, please test and report back.

#4 Updated by Duncan Golicher over 6 years ago

No, it is not fixed in the master, unless there are changes that are
not yet included in the nightly build for Ubuntu. I get exactly the
same behaviour with

QGIS version 1.9.0-Master QGIS code revision exported
Compiled against Qt 4.8.1 Running against Qt 4.8.1
Compiled against GDAL/OGR 1.9.2 Running against GDAL/OGR 1.10dev
GEOS Version 3.3.3 PostgreSQL Client Version 9.1.8
SpatiaLite Version 3.1.0-RC2 QWT Version 5.2.2
PROJ.4 Version 480 QScintilla2 Version

I could post an example layer, but it is not really necessary. To
reproduce the behaviour simply try digitising a new polygon layer in
any way that leads to an invalid topology of any type (for example
crossing back over the line). Then, with avoid intersection switched on
draw a second polygon. This vanishes. Then toggle off editing and save
the edits. The program closes. Happens every time with any layer.

The error messages are

(qgis.bin:28183): Gtk-CRITICAL **: IA__gtk_progress_configure:
assertion `value >= min && value <= max' failed

(qgis.bin:28183): Gtk-CRITICAL **: IA__gtk_progress_configure:
assertion `value >= min && value <= max' failed

(qgis.bin:28183): Gtk-CRITICAL **: IA__gtk_progress_configure:
assertion `value >= min && value <= max' failed
Warning: QMainWindow::saveState(): 'objectName' not set for QToolBar
0xa5936f8 'topoedit'

#5 Updated by Giovanni Manghi over 6 years ago

I could post an example layer,

please do, because with my own tests I get no crash (qgis master on ubuntu). Indeed the polygon do not show, but this is the result of this

#5584

fix, and probably the behavior needs to be changed (an error message should pop up).

#6 Updated by Giovanni Manghi over 6 years ago

Duncan Golicher wrote:

No, it is not fixed in the master, unless there are changes that are
not yet included in the nightly build for Ubuntu. I get exactly the
same behaviour with

my hunch is that you not testing with a master that includes that fix, but I may be wrong. Have you updated recently?

#7 Updated by Duncan Golicher over 6 years ago

I am using the latest (nightly) build that I installed this morning at 11 am on Ubuntu.
The close down/freeze up behaviour is in fact rather inconsistent. It is possible (sometimes) to save a layer with polygons that show up in the attribute table but have no geometries.
This was suggested as a resolution here.
#5584
However, this is dangerous behaviour and I think that it needs changing. Attempts to clean up the layer in GRASS leads to a crash, and any other procedure run on the layer will cause highly unpredictable results.
IMHO, ideally QGIS should warn users about invalid topologies when they are first produced (and have an optional option to prevent any invalid topologies being added, if the user knows that they need to ensure validity). It should also be made impossible to turn on the remove intersection feature for any layer containing topologically invalid polygons. This would be the safest option and would save a lot of error checking along the road.

Duncan

#8 Updated by Giovanni Manghi over 6 years ago

Duncan Golicher wrote:

I am using the latest (nightly) build that I installed this morning at 11 am on Ubuntu.
The close down/freeze up behaviour is in fact rather inconsistent. It is possible (sometimes) to save a layer with polygons that show up in the attribute table but have no geometries.

can you please attach a sample where it is possible to get consistently the crash?

This was suggested as a resolution here.
#5584
However, this is dangerous behaviour and I think that it needs changing.

Before this fix QGIS crashes always when attempting save a vector with an empty geometry. Now it does not crash anywmore, but the result is a vector with a record with no geometry. While the model allow this, I also think that the right behavior is to discard both the geometry and the record.

Attempts to clean up the layer in GRASS leads to a crash, and any other procedure run on the layer will cause highly unpredictable results.

is master? if yes then it is a known issue, after all is the development, release and regressions are expected. You should be able to use GRASS vectors in 1.8 with no major issues.

IMHO, ideally QGIS should warn users about invalid topologies when they are first produced (and have an optional option to prevent any invalid topologies being added, if the user knows that they need to ensure validity).

I agree it would be a nice option, feel free to file a feature request. it would be even better if you would be available to support its development.

It should also be made impossible to turn on the remove intersection feature for any layer containing topologically invalid polygons. This would be the safest option and would save a lot of error checking along the road.

how this would work?

#9 Updated by Duncan Golicher over 6 years ago

This layer causes a crash (sometimes) when another polygon is drawn. The close down is unpredictable with the latest master, but it does occur sporadically.

https://dl.dropbox.com/u/2703650/links/example.zip

I wish I could contribute more to QGIS. I use the program a lot. As a user I don't have any problems myself that I can't fix or work around. However my own C programming is not up to helping directly with development. I can support QGIS to some extent by using the program in my teaching. This broadens the user base. If I could be confident that QGIS were stable enough to be a viable replacement for ESRI licenses I could possible provide more support. However crashes are never tolerated by students working on an assignment. I am very much more concerned about instability issues than adding new features. Unexpected behaviour makes teaching a huge challenge. This extends to any odd behaviour, such as adding features in the attribute table with no geometries. These will produce incorrect results upon analysis, so they lead to a loss in confidence.

My suggestions are to make the upstream error checking more robust in order to save work down the line. If you can prevent users from being able to do things that are never going to work properly (i.e. avoiding intersects with invalid topologies) the problems can be stopped at source. QGIS already contains code to validate geometries. So, I would have thought that it would not be too much work to run checks that prevent "naive" users from making mistakes. These default checks could be turned off by users that know what they are doing.

#10 Updated by Giovanni Manghi over 6 years ago

Duncan Golicher wrote:

This layer causes a crash (sometimes) when another polygon is drawn. The close down is unpredictable with the latest master, but it does occur sporadically.

https://dl.dropbox.com/u/2703650/links/example.zip

no crashes here with the latest master, tested many times. Anyway, the reason of the crash in such situation (trying commit a record with no geometry) is really fixed in master. Anyway I reopened the ticket because it is really needed a warning and (in my opinion) to reject the record all together.

If I could be confident that QGIS were stable enough to be a viable replacement for ESRI licenses I could possible provide more support. However crashes are never tolerated by students working on an assignment. I am very much more concerned about instability issues than adding new features. Unexpected behaviour makes teaching a huge challenge. This extends to any odd behaviour, such as adding features in the attribute table with no geometries. These will produce incorrect results upon analysis, so they lead to a loss in confidence.

I'm able to crash Arcgis 10.1 in less than 1 minute after opening it, but this is not the point. QGIS is becoming more and more stable at each release, but as any other software is not perfect, and if you considered is not supported by a multi-billion company... well that's not bad at all. People and students in particular must understand that is not just a matter of open software be free of charge, freedom and openness count much more. And as the software is made by its own users, the bugs can be fixed very quickly, by a common effort.

My suggestions are to make the upstream error checking more robust in order to save work down the line. If you can prevent users from being able to do things that are never going to work properly (i.e. avoiding intersects with invalid topologies)

sure, please file a ticket with a feature request. Check before if it exist already avoiding a duplicate.

cheers!

#11 Updated by Giovanni Manghi over 6 years ago

possible duplicate of #5584

see also #7229

#12 Updated by Duncan Golicher over 6 years ago

Ideally (and I do so wish that I could offer to do the work on this) I think QGIS should reject an addition to the attribute table with no geometry. I can see no reason to ever permit them. They must surely violate the data model. How can a shapefile be used if there is no geometry for a row in the attribute table? How can the layer be imported to PostGIS? It makes no sense (to me) to allow this. Users should be warned, the polygon should be eliminated completely and the invalid polygons on the original layer should also be flagged up with a suggestion to fix all errors before trying to use the avoid intersection function again. I would have thought that this could be added fairly easily without any major changes.

However, it would be so much safer if users were digitising by default with an option switched on that rejects all invalid topologies without adding them to the layer in the first place. This would mean that if they draw any invalid polygon they would be provided with an error message at that point, and the offending polygon would not be added to the layer until the errors are fixed. I haven't looked through the code, but off hand I would have thought that this would be fairly straightforward to implement.

This would make QGIS easier to learn (providing the checks could be easily turned off when needed), and it would make digitising in QGIS a more professional alternative to using the default Arc simple features model, which itself causes many problems due to its lack of checks on topology.

I agree that Arc also crashes, but students don't necessarily know that. It is all about confidence. Because Arc costs so much students assume that it must be the best. If Arc does crash it is easy to tell students that they are responsible for the problem by doing something wrong themselves. In contrast, if QGIS crashes they tend to blame the program and ask why they are not using the "industry standard" under the assumption that Arc would some how know what to do and would combine the invalid geometries for them.

I know that this attitude is very unfair on all the hard working OS developers, but unless the students are intrinsically sympathetic to the OS philosophy it can be hard to explain to them.

#13 Updated by Giovanni Manghi over 6 years ago

However, it would be so much safer if users were digitising by default with an option switched on that rejects all invalid topologies without adding them to the layer in the first place. This would mean that if they draw any invalid polygon they would be provided with an error message at that point, and the offending polygon would not be added to the layer until the errors are fixed. I haven't looked through the code, but off hand I would have thought that this would be fairly straightforward to implement.

please file a ticket with a feature request.

cheers!

#14 Updated by Marco Hugentobler over 6 years ago

  • Status changed from Feedback to Closed

QgsMapToolAddFeature now rejects features with empty geometry and displays an error message (208c9206ad972b96aef0fdfdebd8012ffa120fe7)

#15 Updated by Giovanni Manghi over 6 years ago

  • Resolution set to fixed

Marco Hugentobler wrote:

QgsMapToolAddFeature now rejects features with empty geometry and displays an error message (208c9206ad972b96aef0fdfdebd8012ffa120fe7)

Hi Marco, thanks for the fix. I have just one note to add: it seems that in master is not possible to add a polygon when this is digitized "against" an existing one and the latter has errors in it. So in this cases the users will receive a "The feature cannot be added because it contains an empty geometry" message even when they think is doing the right thing (and it really seems like that). So I guess that in this cases a different error message should be thrown. But more important seems the necessity to add an option that would not allow to commit a geometry if this is not valid, see #7235 .

Also available in: Atom PDF