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.

Sign up for GitHub

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

Jump to bottom

wxGUI/vdigit: clean zero-length vector map boundaries before start editing #911

Closed
wants to merge 1 commit into from
Closed

wxGUI/vdigit: clean zero-length vector map boundaries before start editing #911

wants to merge 1 commit into from

Conversation

tmszi
Copy link
Member

@tmszi tmszi commented Aug 17, 2020

Fix bug reported in the issue #488.

petrasovaa
petrasovaa reviewed Aug 18, 2020
View reviewed changes
Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

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

I didn't actually test this, so this is not a full review yet, but I would be concerned here that the extensive topology checks which would be called every time you start editing would take a lot of time. Ideally we could figure out (somehow, I don't know how at this point) something is wrong and then try v.build -e.
It would be good to find out why the zero-length boundaries actually crash the GUI.

This should in any case go into separate function for readability.

g.remove is not needed, g.rename with overwrite will do the job.

@tmszi tmszi force-pushed the fix_wxgui_vdigit_zero_length_boundary branch 2 times, most recently from f74ae39 to b22859f Compare August 18, 2020 11:52
@tmszi
Copy link
Member Author

tmszi commented Aug 18, 2020 •
edited
Loading

I didn't actually test this, so this is not a full review yet, but I would be concerned here that the extensive topology checks which would be called every time you start editing would take a lot of time. Ideally we could figure out (somehow, I don't know how at this point) something is wrong and then try v.build -e.

Yes of course. I have seen how the check of zero-length lines is solved in the C function Vect_topo_check() and I use this functionality in my _topoZeroLengthCheck() method.

It would be good to find out why the zero-length boundaries actually crash the GUI.

wxWidgets StrokeLines() function complain:

Traceback (most recent call last):
  File "/usr/lib64/grass79/gui/wxpython/mapwin/buffered.py",
line 593, in OnPaint

self.pdcVector.DrawToDCClipped(gcdc, rgn)
wx._core
.
wxAssertionError
:
C++ assertion "n > 1" failed at /var/tmp/portage/x11-libs/wx
GTK-3.0.4-r302/work/wxWidgets-3.0.4/src/common/graphcmn.cpp(
742) in StrokeLines():

Zero length line/boundary has only one point (start == end) and therefore it is raise C++ assertion "n > 1 (n -> number of points) error.

wxGUI doesn't always crash. Map rendering during editing is incorrect, due to wxWidgets StrokeLines() function assertion error (screenshot from editing #488). Rendering start working during editing if you zoom in the region which doesn't contains zero length lines/boundaries.

This should in any case go into separate function for readability.

All right.

g.remove is not needed, g.rename with overwrite will do the job.

Thanks. I fixed it.

@tmszi tmszi force-pushed the fix_wxgui_vdigit_zero_length_boundary branch from b22859f to 602d210 Compare August 18, 2020 12:44
@tmszi tmszi force-pushed the fix_wxgui_vdigit_zero_length_boundary branch from 602d210 to 2a1e4b1 Compare August 18, 2020 13:48
@tmszi tmszi added backport_needed bug Something isn't working GUI wxGUI related labels Aug 19, 2020
@neteler
Copy link
Member

neteler commented Sep 21, 2020

@tmszi @petrasovaa can this be merged?

@petrasovaa
Copy link
Contributor

@tmszi @petrasovaa can this be merged?

I haven't had time to review it. Did you test it? In any case, I am not sure it's a good idea to backport it, the problem it fixes is rather obscure.

@neteler
Copy link
Member

neteler commented Sep 22, 2020

@tmszi @petrasovaa can this be merged?

I haven't had time to review it. Did you test it?

I think I did but it is too long time ago.
A test dataset is available here: #488 (comment)

In any case, I am not sure it's a good idea to backport it, the problem it fixes is rather obscure.

ok I see.

@neteler neteler added this to the 8.0.0/7.10 milestone Sep 22, 2020
@neteler
Copy link
Member

neteler commented Nov 24, 2021

I finally have been able to test this PR. It works nicely, see also the new screenshots in issue #488.

Thanks a lot, @tmszi. Please merge if there are no further objections.

@neteler neteler mentioned this pull request Nov 24, 2021
Closed
@petrasovaa petrasovaa self-requested a review November 24, 2021 14:31
wenzeslaus
wenzeslaus requested changes Nov 25, 2021
View reviewed changes
Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

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

@tmszi and @neteler I might be late to the party, but here is my take on it.

The original report by @neteler describes the crash being caused by previous crash. Notably, this does not resolve either, but it mitigates the problem by requiring and ensuring clean topology (at least for that particular invalid messy topology case). Please, note this in the merge commit (and perhaps the PR description too). In other words, to require some cleaning before editing is probably fair enough (although messy data might be the very reason I want to edit the vector manually, no?), however it needs to be clear that the code would still crash if the data is not as expected.

There are some minor issues with the code, but the major issues I see are adding this type of code to already overloaded, and possibly misused, toolbars.py, adding analytical code to GUI, and adding another direct access to ctypes. I try to discuss in the comments what I mean.

gui/wxpython/vdigit/toolbars.py
Comment on lines +869 to +872
def _topoZeroLengthCheck(self, map_name):
"""Topology check for lines or boundaries of zero length

:param str map_name: full vector map name
Copy link
Member

Choose a reason for hiding this comment

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

The code in this function should probably be a separate module. It should not be part of GUI because it is data processing. In general, we should be moving any processing and analytical code from GUI. The idea is to keep the GUI as small as possible while making all functionality available in GUI also available for scripting ideally in command line or at least in Python. This also allows for easier testing of the functionality both in terms of writing tests and manual testing in case of debugging an issue.

gui/wxpython/vdigit/toolbars.py
Comment on lines +18 to +26
from ctypes import c_char_p, pointer

import wx

from grass import script as grass
from grass.lib.vector import GV_BOUNDARY, GV_LINE, GV_LINES, Map_info, \
Vect_close, Vect_get_line_type, Vect_get_num_lines, Vect_line_alive, \
Vect_line_length, Vect_new_cats_struct, Vect_new_line_struct, \
Vect_open_old, Vect_read_line
Copy link
Member

Choose a reason for hiding this comment

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

The use of ctypes and the generated ctypes interface to libraries is tricky already at the import level. Other places in GUI which do this are (or should be) very careful about import. It is best to completely avoid this in GUI and solve it in a separate module (in Python or C).

gui/wxpython/vdigit/toolbars.py
Comment on lines +911 to +912
def _cleanZeroLength(self, map_name):
"""Clean lines or boundaries of zero length
Copy link
Member

Choose a reason for hiding this comment

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

This is kind of border line case here. This calls modules, so it is okay in GUI in general and there is already v.build call here, but on the other hand, this file is toolbars.py. Something to consider. Maybe refactoring is needed if there is more stuff needed than just one v.build call.

gui/wxpython/vdigit/toolbars.py
Comment on lines +957 to +975
ltype = None
if zero_length[0] > 0 and zero_length[1] == 0:
ltype = 'line(s)'
elif zero_length[1] > 0 and zero_length[0] == 0:
ltype = 'boundary(ies)'
elif zero_length[0] > 0 and zero_length[1] > 0:
ltype = 'line(s)/boundary(ies)'

if ltype:
clean_dlg = wx.MessageDialog(
self.GetParent(),
message=_(
"The vector map contains zero-length {ltype}, "
"to continue editing, you need to clean them. "
"Do you want to clean them?".format(ltype=ltype)
),
caption=_('Clean?'),
style=wx.YES_NO | wx.YES_DEFAULT | wx.ICON_QUESTION,
)
Copy link
Member

Choose a reason for hiding this comment

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

Translatable strings can't have sentences constructed from words. You need multiple messages or part of the message which is not a sentence but some {x}: {y} type of report or table. There is also a way how to change a message with support of the gettext package based on the number, but I don't know how to easily use it in Python with our suboptimal translation handling (that is _ added to buildins).

gui/wxpython/vdigit/toolbars.py
@@ -882,6 +946,43 @@ def StartEditing(self, mapLayer):
else:
return

# check and clean zero length lines, boundaries
Copy link
Member

Choose a reason for hiding this comment

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

This function is already long, but if we add all these lines with two new return statements and at least six branches, it will be only harder to maintain later. If you stay with this solution, I suggest refactoring or shortening the code needed right here.

gui/wxpython/vdigit/toolbars.py
)

if zero_length is None:
return
Copy link
Member

Choose a reason for hiding this comment

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

The one return statement above returns None, but the three down below return boolean. Pylint should give a warning about that.

@petrasovaa petrasovaa mentioned this pull request Nov 25, 2021
Merged
@petrasovaa
Copy link
Contributor

#1989 fixes the crash described in #488 (merged and backported to 8.0). This PR offers an automatic check and fixing before editing, so it may be nice from user point of view, but it adds a lot of code in GUI for a very minor case and I wonder how it would perform with large vector maps.

@petrasovaa petrasovaa modified the milestones: 8.0.0, 8.2.0 Dec 7, 2021
@wenzeslaus wenzeslaus modified the milestones: 8.2.0, 8.4.0 Feb 27, 2022
@wenzeslaus wenzeslaus modified the milestones: 8.3.0, 8.4.0 Feb 10, 2023
@petrasovaa petrasovaa modified the milestones: 8.4.0, Future Apr 30, 2024
@echoix
Copy link
Member

echoix commented Oct 31, 2024

The original issue was already fixed in another PR, and this PR has multiple issues where some data processing is added in GUI, and also introduces ctypes in GUI; it isn’t in the direction intended.

Thus, this PR will be closed, after almost 4 years without reaching consensus

@echoix echoix closed this Oct 31, 2024
@echoix echoix removed this from the Future milestone Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wenzeslaus wenzeslaus wenzeslaus requested changes

@petrasovaa petrasovaa Awaiting requested review from petrasovaa

Assignees
No one assigned
Labels
bug Something isn't working GUI wxGUI related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tmszi @neteler @petrasovaa @echoix @wenzeslaus