libdisplay: `D_open_driver()` returns `-1` if `GRASS_REGION` is not defined from the terminal #3482

HuidaeCho · 2024-03-10T05:25:53Z

This PR attempts to address #3481. Display commands can be very slow for a small display extent in the monitor because the process executed by the user from the terminal doesn't have access to that display extent through GRASS_REGION.

wget https://data.isnew.info/grass/nidp.pack https://data.isnew.info/grass/drain_tx.pack
grass -c epsg:5070 /tmp/grass_test
r.unpack -o nidp.pack
r.unpack -o drain_tx.pack
g.region rast=nidp -ap
d.mon wx0
d.rast nidp
# zoom to a very small region in the wx0 monitor (e.g., 50x50 grid)
d.rast.arrow -a drain_tx type=drainage # takes very long for a small region

HuidaeCho · 2024-03-10T06:24:42Z

Hmm... It doesn't work with d.redraw because d.redraw has no access to GRASS_REGION in the monitor. #3484 fixes this.

HuidaeCho · 2024-03-11T04:43:41Z

Testing other display modules...

d.info and d.rast.leg (requires d.info) don't work. d.info doesn't display anything on the monitor.

Now, D_open_driver() returns -1 on no GRASS_REGION and d.info/d.rast.leg work. This PR won't affect any other modules if no changes are made to utilize this return value. Before this PR, D_open_driver() always returned 0 and no display modules checked its return value.

…he terminal

… on no GRASS_REGION and individual modules decide what to do (e.g., d.info does not render any layers)

wenzeslaus

More explanation or different implementation needed. Please see my comment in #3481 (comment)

wenzeslaus · 2024-03-14T03:55:30Z

lib/display/r_raster.c

+  \return -1 if GRASS_REGION is not defined; use this return value to decide
+  what to do when rendering for the entire computational region can be slow
+  without constraining to the display extent in GRASS_REGION from the monitor


Is the point that it is too slow or that it is rendered twice? I would say, it should not be rendered twice regardless of the speed. Where (on the disk) it is even rendered the first time in case of d.mon wx0?

It's actually both. It can be slow and renders twice. It's rendered the first time in the same file, which will be overwritten the second time. You can see that first render using this patch to the monitor render.py:

--- MONITORS/wx0/render.py.orig 2024-03-14 01:46:02.172981610 -0600 +++ MONITORS/wx0/render.py 2024-03-14 01:45:37.172088784 -0600 @@ -2,6 +2,7 @@ import os import sys import tempfile +import shutil from grass.script import core as grass from grass.script import task as gtask @@ -163,6 +164,7 @@ read_stdin(cmd) render(cmd, mapfile) + shutil.copy(mapfile, "/tmp/first_render.ppm"); update_cmd_file(os.path.join(path, "cmd"), cmd, mapfile) if cmd[0] == "d.erase" and os.path.exists(legfile): os.remove(legfile)

and run d.rast nidp for a small region. Check /tmp/first_render.ppm to see the entire Texas, not the small region.

wenzeslaus · 2024-03-14T04:00:35Z

lib/display/r_raster.c

+    /* if GRASS_REGION is not defined, a display module can choose to render
+     * for the entire computational region or wait for the monitor to call it
+     * with GRASS_REGION set to the current display extent (what is shown) in
+     * the monitor; the monitor defines GRASS_REGION to the display extent


Usage of GRASS_REGION just happens to be the case in d.mon wx0. I don't know much about the display library, but I think the current API allows implementation of a monitor which does not use GRASS_REGION. This makes GRASS_REGION integral part of the monitor system. That's perhaps fine, but it is a change we should be sure about.

GRASS_REGION is not specific to the wx0 monitor. d.mon's render_cmd.py already uses it to adjust the region:

grass/display/d.mon/render_cmd.py

Line 115 in 9c3b67e

os.environ["GRASS_REGION"] = grass_region

and I think GRASS_REGION was mainly designed for this purpose. Otherwise, the monitor system would have to change the computational region every time it wants to change the display extent.

From variables.html: GRASS_REGION ... use is the same as WIND_OVERRIDE. ... This allows programs such as the GUI to run external commands on an alternate region without having to modify the WIND file then change it back afterwards.

How does g.gui do it?

wenzeslaus · 2024-03-14T04:01:58Z

lib/display/r_raster.c

+     * dynamically, but it's the display module who is responsible to contrain
+     * its rendering within this region to save computational time; if the
+     * display module does not check the return value of D_open_driver(), it
+     * will have to render the same content twice (once by the command line and


"render twice" - It seems to me that should never happen and the display modules should be required to work properly rather than opting in it.

I completely agree with you. However, at the moment, not all display modules display (e.g., d.info). Obviously, with my first exit() method, this module and any other modules relying on it (e.g. d.rast.leg) didn't work.

Maybe, it would be better to add a new argument to D_open_driver(exit_on_no_region) to not exit and use D_open_driver(1) in displaying modules and D_open_driver(0) in non-displaying ones (will still run the entire module twice).

wenzeslaus · 2024-03-14T04:06:47Z

lib/display/r_raster.c

+     * computationally to render for the entire computational region, so it is
+     * recommended to exit from the module if D_open_driver() returns -1; upon


Rendering according to computation region seems like plain wrong when the expectation is only to pass the command line deeper to the rendering backend. Is it possible that this GRASS_REGION issue is a symptom of a deeper issue in the monitor system?

Like I said in #3481 (comment), we used to have an IPC-based (and socket-based) monitor system for UN*X where we didn't have to call display modules three times, IIRC, like we do now. Since we moved away from UN*X only systems and to support non-interactive file-based monitors, we don't have a server-client model anymore. Is it a deeper issue in the monitor system? Yes and no... Yes because it's very annoying to deal with this kind of multiple calls to one command and maybe no because it's a necessary evil within the current architecture when you deal with the terminal and GUI using only environment variables.

wenzeslaus · 2024-03-14T04:07:32Z

lib/display/r_raster.c

+       if (D_open_driver() == -1) {
+           D_close_driver();
+           exit(EXIT_SUCCESS);
+       }


An example usage should be in the Doxygen comment.

HuidaeCho · 2024-03-14T09:17:28Z

I think I found a cleaner and better solution. Testing... Please check #3500.

github-actions bot added C Related code is in C libraries display labels Mar 10, 2024

HuidaeCho added the enhancement New feature or request label Mar 10, 2024

HuidaeCho self-assigned this Mar 10, 2024

HuidaeCho mentioned this pull request Mar 10, 2024

[Bug] Display commands for d.mon can take a long time *unexpectedly* to update the cmd file when first run. #3481

Closed

HuidaeCho linked an issue Mar 10, 2024 that may be closed by this pull request

[Bug] Display commands for d.mon can take a long time *unexpectedly* to update the cmd file when first run. #3481

Closed

HuidaeCho force-pushed the D_open_driver_exit_if_no_region branch from 99b0f5a to 3b41a66 Compare March 10, 2024 06:04

HuidaeCho mentioned this pull request Mar 10, 2024

d.redraw: Rewritten in C #3484

Closed

HuidaeCho added a commit to HuidaeCho/grass that referenced this pull request Mar 10, 2024

Remove lines conflicting with OSGeo#3482

91c73cd

HuidaeCho added the GUI wxGUI related label Mar 10, 2024

HuidaeCho added a commit to HuidaeCho/grass that referenced this pull request Mar 10, 2024

Remove lines conflicting with OSGeo#3482

0d59f3a

HuidaeCho marked this pull request as draft March 11, 2024 04:59

github-actions bot added module and removed GUI wxGUI related labels Mar 11, 2024

HuidaeCho added 3 commits March 11, 2024 02:58

libdisplay: D_open_driver exits if GRASS_REGION is not defined from t…

830dd3b

…he terminal

init() only once

b9439ae

Do not exit from D_open_driver(); Instead, D_open_driver() returns -1…

df0bdf1

… on no GRASS_REGION and individual modules decide what to do (e.g., d.info does not render any layers)

HuidaeCho force-pushed the D_open_driver_exit_if_no_region branch from bbac60f to df0bdf1 Compare March 11, 2024 08:58

HuidaeCho changed the title ~~libdisplay: D_open_driver exits if GRASS_REGION is not defined from the terminal~~ libdisplay: D_open_driver() return -1 if GRASS_REGION is not defined from the terminal Mar 11, 2024

HuidaeCho changed the title ~~libdisplay: D_open_driver() return -1 if GRASS_REGION is not defined from the terminal~~ libdisplay: D_open_driver() returns -1 if GRASS_REGION is not defined from the terminal Mar 11, 2024

HuidaeCho marked this pull request as ready for review March 11, 2024 23:48

HuidaeCho mentioned this pull request Mar 12, 2024

d.rast.num: Constrain rendering within the current display extent in the wx monitor #3489

Merged

neteler added this to the 8.4.0 milestone Mar 12, 2024

Revert d.rast and d.rast.arrow; Will create separate PRs

5217a22

github-actions bot removed the module label Mar 12, 2024

This was referenced Mar 12, 2024

d.rast: Use new D_open_driver() return value to constrain rendering #3491

Closed

d.rast.arrow: Use new D_open_driver() return value to constrain rendering #3492

Closed

echoix previously approved these changes Mar 13, 2024

View reviewed changes

echoix added the GUI wxGUI related label Mar 13, 2024

wenzeslaus requested changes Mar 14, 2024

View reviewed changes

Add more details about the suggested pattern

7be8894

HuidaeCho dismissed echoix’s stale review via 7be8894 March 14, 2024 02:50

Return -1 only if a monitor is used

2fa11a2

wenzeslaus requested changes Mar 14, 2024

View reviewed changes

HuidaeCho mentioned this pull request Mar 14, 2024

d.mon: Delegate rendering to wx monitors #3500

Merged

HuidaeCho closed this Mar 14, 2024

HuidaeCho deleted the D_open_driver_exit_if_no_region branch March 14, 2024 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libdisplay: `D_open_driver()` returns `-1` if `GRASS_REGION` is not defined from the terminal #3482

libdisplay: `D_open_driver()` returns `-1` if `GRASS_REGION` is not defined from the terminal #3482

HuidaeCho commented Mar 10, 2024 •

edited

Loading

HuidaeCho commented Mar 10, 2024 •

edited

Loading

HuidaeCho commented Mar 11, 2024 •

edited

Loading

wenzeslaus left a comment

wenzeslaus Mar 14, 2024

HuidaeCho Mar 14, 2024

wenzeslaus Mar 14, 2024

HuidaeCho Mar 14, 2024 •

edited

Loading

wenzeslaus Mar 14, 2024

HuidaeCho Mar 14, 2024 •

edited

Loading

wenzeslaus Mar 14, 2024

HuidaeCho Mar 14, 2024 •

edited

Loading

wenzeslaus Mar 14, 2024

HuidaeCho commented Mar 14, 2024 •

edited

Loading

		* computationally to render for the entire computational region, so it is
		* recommended to exit from the module if D_open_driver() returns -1; upon

libdisplay: D_open_driver() returns -1 if GRASS_REGION is not defined from the terminal #3482

libdisplay: D_open_driver() returns -1 if GRASS_REGION is not defined from the terminal #3482

Conversation

HuidaeCho commented Mar 10, 2024 • edited Loading

HuidaeCho commented Mar 10, 2024 • edited Loading

HuidaeCho commented Mar 11, 2024 • edited Loading

wenzeslaus left a comment

Choose a reason for hiding this comment

wenzeslaus Mar 14, 2024

Choose a reason for hiding this comment

HuidaeCho Mar 14, 2024

Choose a reason for hiding this comment

wenzeslaus Mar 14, 2024

Choose a reason for hiding this comment

HuidaeCho Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

wenzeslaus Mar 14, 2024

Choose a reason for hiding this comment

HuidaeCho Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

wenzeslaus Mar 14, 2024

Choose a reason for hiding this comment

HuidaeCho Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

wenzeslaus Mar 14, 2024

Choose a reason for hiding this comment

HuidaeCho commented Mar 14, 2024 • edited Loading

libdisplay: `D_open_driver()` returns `-1` if `GRASS_REGION` is not defined from the terminal #3482

libdisplay: `D_open_driver()` returns `-1` if `GRASS_REGION` is not defined from the terminal #3482

HuidaeCho commented Mar 10, 2024 •

edited

Loading

HuidaeCho commented Mar 10, 2024 •

edited

Loading

HuidaeCho commented Mar 11, 2024 •

edited

Loading

HuidaeCho Mar 14, 2024 •

edited

Loading

HuidaeCho Mar 14, 2024 •

edited

Loading

HuidaeCho Mar 14, 2024 •

edited

Loading

HuidaeCho commented Mar 14, 2024 •

edited

Loading