[GRASS-dev] suggested temporary feature freeze for wxGUI

Seeing all the recent bug reports coming in for the new wxPython GUI is frustrating...not because of the testing or reports, which are very good to have, but because almost all of these features worked previously.

This is a function of the complexity of the GUI. Unlike command modules, it involves a very large amount of code that is very tightly interlinked in sometimes not very obvious ways--even to those of us who are writing it. A feature or even a bug fix added in one place can cause a cascade of effects elsewhere and break something that seems completely unrelated.

At the moment, the GUI is pretty complete in giving a user interface to all of GRASS. So I suggest that this is a good time to stop adding new things and try to make sure all the things we already have work well. Obviously we should keep accumulating ideas for enhancements and new features, but simply try to resist the temptation to try to implement them.

It seems to me that we need to focus especially on 3 areas at the moment:

1) basic bug fixing. These are things that throw an error or do not work as designed.

2) increasing robusticity. This is trickier, but I'm thinking of situations where a feature works fine under normal circumstances, but can fail or give an error under abnormal circumstances like repeated mouse clicking, odd combinations of actions, etc. These are harder to find and fix. But we need to make the GUI as unbreakable as possible. Some of this is adding more error traps so that abnormal circumstances may break something, but will do so gracefully.

3) multi-platform support. Somethings worked (and still work) fine on my Mac or Martin's Linux flavor, but don't work so well on other systems. Some important pieces like the digitizer and new nviz modules do not even compile on Mac and Windows. It's been an amazing piece of work to get to the point where we have advanced capabilities like these functional at all. But we now need to make them available to all GRASS users.

Doing these things will give us a dependable and very usable UI for GRASS. This is very important if we want to make this a viable alternative GUI for GRASS 6.4 and the only one for GRASS 7.

After this work, I suggest that we take a look at some of the underlying algorithms for getting maps rendered and displayed, given the rewrite of the display architecture going on now for GRASS 7. Some of this has already been started and it may turn out that only tweaks are needed. But we should take a hard look at this before going on to make sure that this is a solid system down the line.

Then we can start in on the backlog of enhancement and feature requests. But we should implement them very carefully and with a lot of testing from now on to avoid breaking this increasingly complex code.

With a busy academic year already starting, I'm finding much less time to work on the GUI and I suspect that Martin's term is nearing beginning too. So the less developer time available, also makes this a good time for getting caught up with where we are now.

I'd like to appeal for some help from you folks especially for the cross-platform implementation issues. Most of these are C, C++, or compilation related. These are areas where I do not have expertise to do much. Martin has done a fantastic job of adding some sophisticated new modules--especially for digitizing and multi-dimensional visualization. It would be great if someone(s) could help get the cross-platform issues of these modules worked out.

Cheers
Michael

Hi,

2008/9/6 Michael Barton <michael.barton@asu.edu>:

At the moment, the GUI is pretty complete in giving a user interface to all
of GRASS. So I suggest that this is a good time to stop adding new things
and try to make sure all the things we already have work well. Obviously we
should keep accumulating ideas for enhancements and new features, but simply
try to resist the temptation to try to implement them.

I agree, proposal:

* in next weeks create releasebranch_64
* fixing bugs in the release branch
* development goes to develbranch_6 (leading to 6.5) and *especially*
trunk (grass7)
* make wxGUI working under MS Windows (including vector digitizer)
* release RC1 testable on MS Windows (including installer, Marco?) --
middle of October could be reasonable time
* till the end of the year release 6.4.0

With a busy academic year already starting, I'm finding much less time to
work on the GUI and I suspect that Martin's term is nearing beginning too.
So the less developer time available, also makes this a good time for
getting caught up with where we are now.

Yes, in next week I am going back to Prague after almost two years to
restart working at the university, so probably I will have not so much
time for wxGUI.

I'd like to appeal for some help from you folks especially for the
cross-platform implementation issues. Most of these are C, C++, or
compilation related. These are areas where I do not have expertise to do

We definitely need more people to be actively involved.

Martin

--
Martin Landa <landa.martin gmail.com> * http://gama.fsv.cvut.cz/~landa *

This all sounds like a good plan. See below for a couple comments.
On Sep 6, 2008, at 2:45 PM, Martin Landa wrote:

Hi,

2008/9/6 Michael Barton <michael.barton@asu.edu>:

At the moment, the GUI is pretty complete in giving a user interface to all
of GRASS. So I suggest that this is a good time to stop adding new things
and try to make sure all the things we already have work well. Obviously we
should keep accumulating ideas for enhancements and new features, but simply
try to resist the temptation to try to implement them.

I agree, proposal:

* in next weeks create releasebranch_64
* fixing bugs in the release branch
* development goes to develbranch_6 (leading to 6.5) and *especially*
trunk (grass7)
* make wxGUI working under MS Windows (including vector digitizer)
* release RC1 testable on MS Windows (including installer, Marco?) --

Don't forget Mac too. Maybe William Kyngesburye can help with the compilation issues. Neither vdigit nor the new ndim visualizer will compile with Mac--even though it is on a Unix platform.

middle of October could be reasonable time
* till the end of the year release 6.4.0

With a busy academic year already starting, I'm finding much less time to
work on the GUI and I suspect that Martin's term is nearing beginning too.
So the less developer time available, also makes this a good time for
getting caught up with where we are now.

Yes, in next week I am going back to Prague after almost two years to
restart working at the university, so probably I will have not so much
time for wxGUI.

I'd like to appeal for some help from you folks especially for the
cross-platform implementation issues. Most of these are C, C++, or
compilation related. These are areas where I do not have expertise to do

We definitely need more people to be actively involved.

Martin

--
Martin Landa <landa.martin gmail.com> * http://gama.fsv.cvut.cz/~landa *

Hi,

2008/9/7 Michael Barton <michael.barton@asu.edu>:

[...]

* in next weeks create releasebranch_64
* fixing bugs in the release branch
* development goes to develbranch_6 (leading to 6.5) and *especially*
trunk (grass7)
* make wxGUI working under MS Windows (including vector digitizer)
* release RC1 testable on MS Windows (including installer, Marco?) --

Don't forget Mac too. Maybe William Kyngesburye can help with the
compilation issues. Neither vdigit nor the new ndim visualizer will compile
with Mac--even though it is on a Unix platform.

Vector digitizer should work on Mac even without lgdi link. For
wxNviz, there is still unsolved issue with direct rendering (which is
needed for nviz_cmd).

Martin

--
Martin Landa <landa.martin gmail.com> * http://gama.fsv.cvut.cz/~landa *

On Sep 7, 2008, at 5:06 AM, Martin Landa wrote:

Hi,

2008/9/7 Michael Barton <michael.barton@asu.edu>:

Don't forget Mac too. Maybe William Kyngesburye can help with the
compilation issues. Neither vdigit nor the new ndim visualizer will compile
with Mac--even though it is on a Unix platform.

Vector digitizer should work on Mac even without lgdi link. For
wxNviz, there is still unsolved issue with direct rendering (which is
needed for nviz_cmd).

Martin

I haven't tried compiling GRASS in a while, busy with other stuff. I thought vdigit compiled - the makefile has the correct compile flags to handle libgdi.

Yes, nviz has the problem where you need to disable render.c (and lose nviz_cmd). The OpenGL programming is too deep for me.

There is a separate issue (not critical) that I'd like to see fixed someday - Python linking. Currently any binary code that uses python (vdigit, nviz) links the python binary. At least on OSX, this is not the proper way to do it. It's more like the dynamic lookup flags used in vdigit to handle libgdi, but it requires knowing the full path to the python binary, which is tricky in OSX.

It was discussed a while back, but Glynn was reluctant to put Python path-detecting code in configure.

The main benefit of all this is that whichever python binary was used to compile GRASS, *any* compatible python binary can be used to run GRASS. ie, compile using the system python 2.5.1 on Leopard, but run using the python.org Python 2.5.2. I'm seeing a lot more people preferring the up-to-date python over the system python, though I personally prefer to stick with the system python.

-----
William Kyngesburye <kyngchaos*at*kyngchaos*dot*com>
http://www.kyngchaos.com/

"This is a question about the past, is it? ... How can I tell that the past isn't a fiction designed to account for the discrepancy between my immediate physical sensations and my state of mind?"

- The Ruler of the Universe

On Sep 7, 2008, at 11:23 AM, William Kyngesburye wrote:

I haven't tried compiling GRASS in a while, busy with other stuff. I thought vdigit compiled - the makefile has the correct compile flags to handle libgdi.

Yes, nviz has the problem where you need to disable render.c (and lose nviz_cmd). The OpenGL programming is too deep for me.

Ah. libnviz has a problem, in nviz.h, for the AGL types. This should fix it (and gets a step closer to fixing the direct rendering problem):

--- include/nviz.h.orig 2008-08-12 22:19:32.000000000 -0500
+++ include/nviz.h 2008-08-12 13:03:31.000000000 -0500
@@ -128,9 +128,9 @@
      GLXPixmap windowId;
      Pixmap pixmap;
  #elif defined(OPENGL_AQUA)
- AGLPixelFmtID pixelFmtId;
+ AGLPixelFormat pixelFmtId;
      AGLContext contextId;
- AGLPixmap windowId;
+ AGLPbuffer windowId;
      GWorldPtr pixmap;
  #elif defined(OPENGL_WINDOWS)
      HDC displayId; /* display context */

Now there is another error to deal with in wxpython/nviz - gstypes.h defines X, Y and Z for (internal use only), but this causes problems for one of the OSX application services headers. The fix I figured out was to include ApplicationServices.h and agl.h before the grass headers. This is done in include/nviz.h, but not in wxpython/nviz/nviz.h. Change the #include order to:

#include <grass/gis.h>
#include <grass/nviz.h>
#include <grass/gsurf.h>
#include <grass/gstypes.h>

And in nviz.i change to:

#include "nviz.h"
#include <grass/gsurf.h>
#include <grass/gstypes.h>

Now we're left with some errors in grass6_wxnviz_wrap.cpp:

grass6_wxnviz_wrap.cpp:3165: error: expected unqualified-id before ‘{’ token
grass6_wxnviz_wrap.cpp:3173: error: expected unqualified-id before ‘{’ token
grass6_wxnviz_wrap.cpp:3180: error: expected unqualified-id before ‘{’ token
...

These are all

static bool check(PyObject *obj) {

I seem to recall seeing this before somewhere with swig-generated code, and I don't remember what the solution was.

-----
William Kyngesburye <kyngchaos*at*kyngchaos*dot*com>
http://www.kyngchaos.com/

[Trillian] What are you supposed to do WITH a maniacally depressed robot?

[Marvin] You think you have problems? What are you supposed to do if you ARE a maniacally depressed robot? No, don't try and answer, I'm 50,000 times more intelligent than you and even I don't know the answer...

- HitchHiker's Guide to the Galaxy

Hi,

2008/9/7 William Kyngesburye <woklist@kyngchaos.com>:

Ah. libnviz has a problem, in nviz.h, for the AGL types. This should fix
it (and gets a step closer to fixing the direct rendering problem):

[...]

Committed, r33321.

Martin

--
Martin Landa <landa.martin gmail.com> * http://gama.fsv.cvut.cz/~landa *

Michael Barton wrote:

Seeing all the recent bug reports coming in for the new wxPython GUI
is frustrating...not because of the testing or reports, which are very
good to have, but because almost all of these features worked
previously.

This is a function of the complexity of the GUI. Unlike command
modules, it involves a very large amount of code that is very tightly
interlinked in sometimes not very obvious ways--even to those of us
who are writing it. A feature or even a bug fix added in one place can
cause a cascade of effects elsewhere and break something that seems
completely unrelated.

In which case, it's probably time to start thinking about refactoring
the code.

E.g. class BufferedWindow in mapdisp.py is ~2500 lines. That includes
some massive "case" (i.e. if/elif/...) statements in OnLeftUp and
OnRightUp which should probably be replaced with e.g.
self.currentTool.OnLeftUp() etc, with a separate class for each tool.

2) increasing robusticity. This is trickier, but I'm thinking of
situations where a feature works fine under normal circumstances, but
can fail or give an error under abnormal circumstances like repeated
mouse clicking, odd combinations of actions, etc.

I would guess that most of this is due to the use of threads. Writing
concurrent code is hard, and performing meaningful testing is all but
impossible (textbooks on the subject often include extensive sections
on formal proofs using temporal logic).

[We have seen this issue on a smaller scale with NVIZ calling Tcl's
"update" during rendering.]

As a general rule, auxilliary threads (i.e. everything except for the
main event-handling thread) should avoid accessing any shared data. If
necessary, data should be (deep-) copied within the main thread and
the copy passed to the auxilliary thread when it is created. Any
interaction with the rest of the process should be via wx.CallAfter.

If shared data has to be read or written directly by auxilliary
threads, it needs to be protected against concurrent access with
locks. Code which performs a read-modify-write cycle on data needs to
retain the lock for the duration of the cycle.

Also, bear in mind that GRASS itself isn't particularly thread-safe.
Many library functions aren't thread-safe, and anything which modifies
fixed files (WIND, CURGROUP, etc) is risky. The safest solution is to
ensure that you never run multiple GRASS commands concurrently. If
necessary, just block the main thread until any other threads have
finished.

--
Glynn Clements <glynn@gclements.plus.com>

Hi,

2008/9/7 Glynn Clements <glynn@gclements.plus.com>:

In which case, it's probably time to start thinking about refactoring
the code.

E.g. class BufferedWindow in mapdisp.py is ~2500 lines. That includes
some massive "case" (i.e. if/elif/...) statements in OnLeftUp and
OnRightUp which should probably be replaced with e.g.
self.currentTool.OnLeftUp() etc, with a separate class for each tool.

definitely true for trunk. Probably develbranch_6 we can leave as it
is now, or to backport relevant changes from trunk, but before that we
need releasebranch_64.

[...]

Martin

--
Martin Landa <landa.martin gmail.com> * http://gama.fsv.cvut.cz/~landa *

Thanks for the suggestions.

Refactoring is certainly in order. We've started some of this already, but there is a lot more to do. I don't think we are overtly using multithreading. However, Python may be doing this on its own I suppose.

Michael

Michael

On Sep 7, 2008, at 2:44 PM, Glynn Clements wrote:

Michael Barton wrote:

Seeing all the recent bug reports coming in for the new wxPython GUI
is frustrating...not because of the testing or reports, which are very
good to have, but because almost all of these features worked
previously.

This is a function of the complexity of the GUI. Unlike command
modules, it involves a very large amount of code that is very tightly
interlinked in sometimes not very obvious ways--even to those of us
who are writing it. A feature or even a bug fix added in one place can
cause a cascade of effects elsewhere and break something that seems
completely unrelated.

In which case, it's probably time to start thinking about refactoring
the code.

E.g. class BufferedWindow in mapdisp.py is ~2500 lines. That includes
some massive "case" (i.e. if/elif/...) statements in OnLeftUp and
OnRightUp which should probably be replaced with e.g.
self.currentTool.OnLeftUp() etc, with a separate class for each tool.

2) increasing robusticity. This is trickier, but I'm thinking of
situations where a feature works fine under normal circumstances, but
can fail or give an error under abnormal circumstances like repeated
mouse clicking, odd combinations of actions, etc.

I would guess that most of this is due to the use of threads. Writing
concurrent code is hard, and performing meaningful testing is all but
impossible (textbooks on the subject often include extensive sections
on formal proofs using temporal logic).

[We have seen this issue on a smaller scale with NVIZ calling Tcl's
"update" during rendering.]

As a general rule, auxilliary threads (i.e. everything except for the
main event-handling thread) should avoid accessing any shared data. If
necessary, data should be (deep-) copied within the main thread and
the copy passed to the auxilliary thread when it is created. Any
interaction with the rest of the process should be via wx.CallAfter.

If shared data has to be read or written directly by auxilliary
threads, it needs to be protected against concurrent access with
locks. Code which performs a read-modify-write cycle on data needs to
retain the lock for the duration of the cycle.

Also, bear in mind that GRASS itself isn't particularly thread-safe.
Many library functions aren't thread-safe, and anything which modifies
fixed files (WIND, CURGROUP, etc) is risky. The safest solution is to
ensure that you never run multiple GRASS commands concurrently. If
necessary, just block the main thread until any other threads have
finished.

--
Glynn Clements <glynn@gclements.plus.com>

Michael Barton wrote:

Refactoring is certainly in order. We've started some of this already,
but there is a lot more to do. I don't think we are overtly using
multithreading.

Yep:
  gcmd.py:477:class CommandThread(Thread):
  goutput.py:48:class CmdThread(threading.Thread):
  mapdisp.py:84:class Command(Thread):
  nviz_mapdisp.py:45:class NvizThread(Thread):

histogram.py and vdigit.py also import the Thread class, but they
don't actually use it.

--
Glynn Clements <glynn@gclements.plus.com>

Hi,

2008/9/8 Glynn Clements <glynn@gclements.plus.com>:

       gcmd.py:477:class CommandThread(Thread):
       goutput.py:48:class CmdThread(threading.Thread):

It's used for running GRASS command in own thread to update GUI
widgets properly (print module messages to log widget when the command
is running).

       mapdisp.py:84:class Command(Thread):

This not used currently.

       nviz_mapdisp.py:45:class NvizThread(Thread):

Not fully working, reason is the same as for CommandThread.

histogram.py and vdigit.py also import the Thread class, but they
don't actually use it.

For vdigit.py I was planning to implement same as for nviz_mapdisp.

--
Martin Landa <landa.martin gmail.com> * http://gama.fsv.cvut.cz/~landa *

On Sep 7, 2008, at 1:14 PM, William Kyngesburye wrote:

Now we're left with some errors in grass6_wxnviz_wrap.cpp:

grass6_wxnviz_wrap.cpp:3165: error: expected unqualified-id before ‘{’ token
grass6_wxnviz_wrap.cpp:3173: error: expected unqualified-id before ‘{’ token
grass6_wxnviz_wrap.cpp:3180: error: expected unqualified-id before ‘{’ token
...

These are all

static bool check(PyObject *obj) {

I seem to recall seeing this before somewhere with swig-generated code, and I don't remember what the solution was.

I think I got it - it's a similar problem to the X, Y and Z defines. I dug around in the OSX system and found that "check" is defined in AssertMacros.h (which looks like is mainly used for debugging). I didn't see any direct includes of that, but I suspect it comes eventually from the Python.h include.

This seems to work - add this to the end of the %module section in nviz.i:

#undef check

-----
William Kyngesburye <kyngchaos*at*kyngchaos*dot*com>
http://www.kyngchaos.com/

The equator is so long, it could encircle the earth completely once.