d.mon: Delegate rendering to wx monitors

[HuidaeCho]

This PR is another attempt to address #3481. It should replace

• libdisplay: D_open_driver() returns -1 if GRASS_REGION is not defined from the terminal #3482
• d.rast: Use new D_open_driver() return value to constrain rendering #3491
• d.rast.arrow: Use new D_open_driver() return value to constrain rendering #3492

How it works:

1. d.mon start=wx0
2. Run a display module from the terminal
3. D_open_setup() from the module spawns the monitor's render.py and exits
4. render.py touches a temp map file for the wx monitor so the monitor can force rendering non-existing layers; without this empty file, the monitor won't render them (Layer.IsRendered() and RenderMapMgr._checkRenderedSizes() in gui/wxpython/core/render.py)
5. render.py doesn't render() for the wx monitor because the monitor is watching the cmd file and will render any new layers; existing layers won't be re-rendered (exists in GetLayersFromCmdFile() in gui/wxpython/mapdisp/main.py
6. render.py adds the new command line to the cmd file
7. The monitor catches a change in the cmd file from step 6 and calls the display module for actual rendering (RenderLayerMgr.Render() in gui/wxpython/core/render.py)

A display module is called twice by the user from the terminal and by the wx monitor or render.py for non-wx monitors.

[HuidaeCho]

Unlike #3482, with this PR, no changes are needed in display modules.

[wenzeslaus]

While I'm missing some details here, this looks like a good and appropriate solution to the problem. It does special things for d.mon wx* which is where the issue is.

[wenzeslaus]

@landam It would be good if you can give this at least a quick look.

[wenzeslaus]

My comments on the explanation:

1. d.mon start=wx0
2. Run a display module from the terminal
3. D_open_setup() from the module spawns the monitor's render.py and exits

Seems like intended behavior.

4. render.py touches a temp map file for the wx monitor so the monitor can force rendering non-existing layers; without this empty file, the monitor won't render them (Layer.IsRendered() and RenderMapMgr._checkRenderedSizes() in gui/wxpython/core/render.py)

This is the part not clear to me. Why is the touch needed? The rendering happens based on the cmd file (not map file, no?) and than the rendering actually modifies the map file, so why do we need to touch it beforehand?

5. render.py doesn't render() for the wx monitor because the monitor is watching the cmd file and will render any new layers; existing layers won't be re-rendered (exists in GetLayersFromCmdFile() in gui/wxpython/mapdisp/main.py

Makes sense. The wxGUI code does its own rendering in the g.gui case, so doing it for the d.mon wx* case seems like the obvious choice. Commands from terminal then really work only as interface which needs to communicate. The idea (if it was there) to render sort of externally to wxGUI and then just use the file is not bad, but perhaps handling it in wxGUI is more straightforward.

6. render.py adds the new command line to the cmd file

7. The monitor catches a change in the cmd file from step 6 and calls the display module for actual rendering (RenderLayerMgr.Render() in gui/wxpython/core/render.py)

Seems like intended behavior.

A display module is called twice by the user from the terminal and by the wx monitor or render.py for non-wx monitors.

That sounds clear and straightforward.

[wenzeslaus]

I suggest closing the GRASS_REGION PRs.

[HuidaeCho]

My comments on the explanation:

1. d.mon start=wx0
2. Run a display module from the terminal
3. D_open_setup() from the module spawns the monitor's render.py and exits

Seems like intended behavior.

Yes

4. render.py touches a temp map file for the wx monitor so the monitor can force rendering non-existing layers; without this empty file, the monitor won't render them (Layer.IsRendered() and RenderMapMgr._checkRenderedSizes() in gui/wxpython/core/render.py)

This is the part not clear to me. Why is the touch needed? The rendering happens based on the cmd file (not map file, no?) and than the rendering actually modifies the map file, so why do we need to touch it beforehand?

This is only my guess; the original developer should know better. Maybe, the monitor code assumes that new layers are only added through the command line, which will create first images (map files) from the terminal (no more with this PR, only empty images) and it thinks checking the cmd file for a new layer without a map file should be enough? This behavior was also unexpected to me. g.gui is different because adding a new layer (well, everything) is done from the GUI.

5. render.py doesn't render() for the wx monitor because the monitor is watching the cmd file and will render any new layers; existing layers won't be re-rendered (exists in GetLayersFromCmdFile() in gui/wxpython/mapdisp/main.py

Makes sense. The wxGUI code does its own rendering in the g.gui case, so doing it for the d.mon wx* case seems like the obvious choice. Commands from terminal then really work only as interface which needs to communicate. The idea (if it was there) to render sort of externally to wxGUI and then just use the file is not bad, but perhaps handling it in wxGUI is more straightforward.

If there is a better way to let wxGUI know about commands from the terminal. To me, render.py is right between the terminal and wxGUI.

Just one thing. If someone more familiar with wxGUI could modify it to create missing map files, the touching line would not be necessary.

6. render.py adds the new command line to the cmd file
7. The monitor catches a change in the cmd file from step 6 and calls the display module for actual rendering (RenderLayerMgr.Render() in gui/wxpython/core/render.py)

Seems like intended behavior.

Yes

A display module is called twice by the user from the terminal and by the wx monitor or render.py for non-wx monitors.

That sounds clear and straightforward.

Great!

[echoix]

Is this PR ready or there was still some missing pieces?

[HuidaeCho]

Is this PR ready or there was still some missing pieces?

This PR is ready to go with #3484.

[HuidaeCho]

How to test this PR and #3484:

d.mon start=wx0
d.rast elevation
# zoom in
d.redraw
# 1. see how d.redraw *with* #3484 draws the full extent
#    because rendering is done from the command line and
#    only the monitor knows the current extent
# 2. pan and you'll see the previously zoomed-in and panned rendering

Now, with both this PR and #3484, d.redraw should render the correct display extent:

d.mon start=wx0
d.rast elevation
# zoom in
d.redraw
# 1. see how d.redraw with this PR *and* #3484 draws the correct extent
#    because it delegates rendering to the monitor, which knows the current extent

[echoix]

Does somebody else want to test these two out?

Then, once we have the go,
which one should be merged first? This one (#3500) probably, then #3484?

[HuidaeCho]

Does somebody else want to test these two out?

Then, once we have the go, which one should be merged first? This one (#3500) probably, then #3484?

Ideally, both at the same time. If only one first, I would merge this PR before #3484.

[echoix]

I'm not set up to be able to test this out. (I don't have a standard Linux desktop/gui installation, I only ever use Linux terminal only, or through WSL that even with now gui support, if I would see a UI problem it probably wouldn't be this PR's fault).

If someone is able to find something that would change the contents of these two PRs, they should.

Otherwise, they already had 2 months to object themselves. For these two I don't see any problems from what I see in the diffs, and from what I know. The problems would be on what I don't know (ie, impacts on parts of the code that I haven't read yet).

I would be almost ready to take your word for it, and simply have these changes merged in main for longer instead, in case any problem shows up.

[wenzeslaus]

Off-topic: I wanted to quickly check the new code again since I approved this in the past, but the rebase (force-push) prevents GitHub from showing changes since the last review. So, I guess that's a major drawback of rebase in a PR branch.

[neteler]

I tried to locally patch my main version and test but this PR is apparently out of sync:

wget https://github.com/OSGeo/grass/pull/3484.diff
wget https://github.com/OSGeo/grass/pull/3500.diff

✔ ~/software/grass_main [main|✚ 1…41⚑ 8] 
11:16 $ patch -p1 < 3484.diff
patching file display/Makefile
patching file display/d.mon/render_cmd.py
patching file display/d.redraw/Makefile
patching file display/d.redraw/d.redraw.html
patching file display/d.redraw/main.c
patching file scripts/Makefile
patching file scripts/d.redraw/Makefile
patching file scripts/d.redraw/d.redraw.html
patching file scripts/d.redraw/d.redraw.py

✔ ~/software/grass_main [main|✚ 7…42⚑ 8] 
11:17 $ patch -p1 < 3500.diff
patching file display/d.mon/render_cmd.py
Hunk #2 FAILED at 71.
Hunk #3 succeeded at 185 (offset -1 lines).
1 out of 3 hunks FAILED -- saving rejects to file display/d.mon/render_cmd.py.rej

Can it please be updated first (I hesitate to use the "Update branch" button)?

[HuidaeCho]

@neteler Please try it again (now #3727 and #3500):

$ git clone git@github.com:OSGeo/grass.git 
$ cd grass
$ wget https://github.com/OSGeo/grass/pull/3727.diff
$ wget https://github.com/OSGeo/grass/pull/3500.diff
$ patch -p1 < 3727.diff
patching file display/Makefile
patching file display/d.redraw/Makefile
patching file display/d.redraw/d.redraw.html
patching file display/d.redraw/main.c
patching file scripts/Makefile
patching file scripts/d.redraw/Makefile
patching file scripts/d.redraw/d.redraw.html
patching file scripts/d.redraw/d.redraw.py
$ patch -p1 < 3500.diff                    
patching file display/d.mon/render_cmd.py

[neteler]

I have tried #3727 and #3500 together. The updated monitor seems to behave as expected (the test case above runs as expected for me).