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

[HuidaeCho]

Describe the bug
When we run a display command (d.*) from the terminal to add a layer to a wx* monitor, it runs twice:

1. Original process executed by the user without GRASS_REGION (display extent): This process adds a new command line to the cmd file. If the command takes a long time to finish for the entire raster region, we have to wait even if the display extent is very small.
2. Another process executed by the monitor with GRASS_REGION: The monitor runs the above command line saved in the cmd file, but this time with the current display extent. The same command can run quickly if the display extent is very small. Ultimately, we will only see this result.

To Reproduce
Steps to reproduce the behavior:

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

See it takes long even though the display extent is very small

Expected behavior
Any display commands should always honor the current display extent.

System description (please complete the following information):

• Operating System: Linux
• GRASS GIS version: main branch

Additional context
GRASS_REGION is internal to the monitor and the terminal doesn't have access to it. Even if GRASS_REGION was exposed to the terminal, display commands would still run twice.

I changed

grass/display/d.rast.arrow/main.c

Lines 222 to 223 in b45319c

	/* Setup driver and check important information */
	D_open_driver();

to

    /* Setup driver and check important information */
    D_open_driver();

    if (!getenv("GRASS_REGION")) {
        D_save_command(G_recreate_command());
        D_close_driver();
        exit(EXIT_SUCCESS);
    }

and it worked. Basically, d.rast.arrow just adds a new command line to the cmd file and exits. Then, it'll be executed by the monitor for actual rendering. This solution is not very elegant and takes the module developer's effort.

Maybe, we add to D_open_driver():

    if (!getenv("GRASS_REGION")) {
        D_save_command(G_recreate_command());
        D_close_driver();
        return 1;
    }

and in each display module:

    /* Setup driver and check important information */
    if (D_open_driver() == 1) exit(EXIT_SUCCESS);

if rendering for the entire computational region can be expensive.

Still, I'm not 100% sure that GRASS_REGION must be required to render anything on a display. For example, render this entire raster on a PNG. I think we need a solution.

Created #3482.

[HuidaeCho]

Or something like this... In D_open_driver()

    if (**running in a tty OR not in GUI**) { /* Or we still need to check GRASS_REGION
                                                 in case we use non-interactive monitors. */
        D_save_command(G_recreate_command());
        D_close_driver();
        return 1; /* it would be easier to just kill the process here, so we don't have to change anything
                     in display modules, but I'm not sure if that would be a better design */
    }

and in each display module:

    /* Setup driver and check important information */
    if (D_open_driver() == 1) exit(EXIT_SUCCESS);

[HuidaeCho]

OK, D_open_driver() already exit() on adding a new command line, but the command is spawned before that.

[HuidaeCho]

I did more testing. Without any changes to display modules, changing this:

grass/lib/display/r_raster.c

Lines 148 to 155 in b45319c

	if (p && G_strcasecmp(drv->name, p) != 0)
	G_warning(_("Unknown display driver <%s>"), p);
	G_verbose_message(_("Using display driver <%s>..."), drv->name);
	LIB_init(drv);
	init();
	return 0;

to

    if (p && G_strcasecmp(drv->name, p) != 0)
        G_warning(_("Unknown display driver <%s>"), p);
    G_verbose_message(_("Using display driver <%s>..."), drv->name);
    LIB_init(drv);

    init();

    /* don't run display commands if GRASS_REGION (display extent) is not defined;
       rendering will be done by the display driver */
    if (!getenv("GRASS_REGION")) {
        D_close_driver();
        exit(0);
    }

worked.

[HuidaeCho]

#3482 now uses a less invasive approach by returning -1 and letting individual modules decide what to do when GRASS_REGION is not defined (D_open_driver() == -1). Without any code changes in display modules (not checking D_open_driver() return value), everything should be the same as before.

[wenzeslaus]

I can confirm the issue, but the solution with GRASS_REGION does not look right to me because GRASS_REGION is what happens to be used in this context and is used in wxGUI and grass.jupyter, but that's not the only way to render.

Additionally, I actually tested the most bare bones way of rendering and that does not work with the new code (your two relevant PRs combined). The following takes ages with the current code, but with the proposed code, it does not render anything:

export GRASS_RENDER_IMMEDIATE=cairo
export GRASS_RENDER_FILE_READ=TRUE
g.region rows=50 cols=50
rm map.png 
d.rast nidp
d.rast.arrow -a drain_tx type=drainage
echo $?  # 0 reported, but map.png contains only nidp

I must say I don't understand how GRASS_REGION fixes that since g.region does not fix it for me here. Are there two issues? Double rendering going on or double init and the region not being respected? Is that the case?

[HuidaeCho]

I can confirm the issue, but the solution with GRASS_REGION does not look right to me because GRASS_REGION is what happens to be used in this context and is used in wxGUI and grass.jupyter, but that's not the only way to render.

Additionally, I actually tested the most bare bones way of rendering and that does not work with the new code (your two relevant PRs combined). The following takes ages with the current code, but with the proposed code, it does not render anything:

export GRASS_RENDER_IMMEDIATE=cairo
export GRASS_RENDER_FILE_READ=TRUE
g.region rows=50 cols=50
rm map.png 
d.rast nidp
d.rast.arrow -a drain_tx type=drainage
echo $?  # 0 reported, but map.png contains only nidp

I must say I don't understand how GRASS_REGION fixes that since g.region does not fix it for me here. Are there two issues? Double rendering going on or double init and the region not being respected? Is that the case?

The -a flag for d.rast.(arrow|num) currently means draw arrows and numbers aligned with original raster cells.

I think you meant:

export GRASS_RENDER_IMMEDIATE=cairo
export GRASS_RENDER_FILE_READ=TRUE
g.region rows=50 cols=50
rm map.png 
d.rast nidp
d.rast.arrow drain_tx type=drainage

which worked for me with 2fa11a2.

[wenzeslaus]

I can confirm that with the latest change the basic ("immediate") rendering works again.

Still, does the rendering actually happen twice? If yes, #3482 seems like fixing this double rendering issue using an implementation detail of the monitor-driven rendering somewhere deep in the monitor system, namely usage of GRASS_REGION. If user defines GRASS_REGION in the terminal, the GRASS_REGION-based test will pass and the situation is the same as what these PRs are trying to address. Is there a different way to tell that a d-command is being used as an interface in d.mon monitors rather than being used for actual rendering of data to a file? Or we say GRASS_REGION is the way and ignore the edge cases?

[HuidaeCho]

Without any of my related PRs,

• With a monitor, yes, it does double-rendering
  • Once by the monitor's render.py (computational region, this was the original issue) and one more by the GUI's watcher (GRASS_REGION)
  • The display module is called three times: 1) by the user (not rendering anything because D_open_driver() exits), 2) by spawned render.py and 3) by the GUI
• Without a monitor (your test case above), no, it doesn't do double-rendering
  • render.py is not spawned (no attached monitor) and
  • the GUI has nothing to do with it because there is no cmd file to watch at all.
• Within g.gui, I don't think it does double-rendering because it always does immediate rendering (no spawning of any external rendering script) with GRASS_REGION (no slow rendering for a small region) if I'm not wrong.

With my related PRs

• With a monitor, no, it won't do double-rendering anymore if the module developer follows the suggested pattern
  • The process by the user just spawns render.py and exits in D_open_driver()
  • D_open_driver() from a new process by render.py returns -1
  • If the suggested pattern is used, this new process just exits without rendering
  • The GUI triggers this module again with GRASS_REGION and D_open_driver() returns 0
  • Now, the module renders only once

If the return value of D_open_driver() isn't checked, the module's behavior will remain the same as before these PRs (double rendering).

• Without a monitor (your test case above), the behavior should be the same as before (single rendering)
  • render.py is not spawned (no attached monitor) and
  • the GUI has nothing to do with it because there is no cmd file to watch at all.

Please try this. Defining GRASS_REGION from the terminal will render once per module because MONITOR check fails (no render.py spawning) and D_open_driver() returns 0.

export GRASS_REGION="proj: 99; zone: 0; north: 842288.7298103943; south: 842056.1497003906; east: -297381.45183609176; west: -297756.32556617697; cols: 1420; rows: 881; e-w resol: 0.2639955846; n-s resol: 0.2639955846; top: 1.000000000000000; bottom: 0.000000000000000; cols3: 44251; rows3: 41262; depths: 1; e-w resol3: 30; n-s resol3: 30; t-b resol: 1;"
export GRASS_RENDER_IMMEDIATE=cairo
export GRASS_RENDER_FILE_READ=TRUE
rm map.png
d.rast nidp
d.rast.arrow -a drain_tx type=drainage

I also tested GRASS_RENDER_COMMAND:

cat<<EOT>cmd.py
#!/usr/bin/env python3
import os
import sys
from grass.script import core as grass
from grass.script import task as gtask

os.environ["GRASS_RENDER_IMMEDIATE"] = "cairo"
os.environ["GRASS_RENDER_FILE_READ"] = "TRUE"

cmd = gtask.cmdstring_to_tuple(sys.argv[1])
grass.run_command(cmd[0], **cmd[1])
EOT
export GRASS_RENDER_COMMAND=cmd.py
rm -f map.png
g.region rows=50 cols=50
d.rast nidp
d.rast.arrow drain_tx type=drainage

It rendered once.

GRASS_RENDER_COMMAND with GRASS_REGION

cat<<EOT>cmd.py
#!/usr/bin/env python3
import os
import sys
from grass.script import core as grass
from grass.script import task as gtask

os.environ["GRASS_RENDER_IMMEDIATE"] = "cairo"
os.environ["GRASS_RENDER_FILE_READ"] = "TRUE"

cmd = gtask.cmdstring_to_tuple(sys.argv[1])
grass.run_command(cmd[0], **cmd[1])
EOT
export GRASS_RENDER_COMMAND=cmd.py
export GRASS_REGION="proj: 99; zone: 0; north: 842288.7298103943; south: 842056.1497003906; east: -297381.45183609176; west: -297756.32556617697; cols: 1420; rows: 881; e-w resol: 0.2639955846; n-s resol: 0.2639955846; top: 1.000000000000000; bottom: 0.000000000000000; cols3: 44251; rows3: 41262; depths: 1; e-w resol3: 30; n-s resol3: 30; t-b resol: 1;"
rm -f map.png
g.region rast=nidp
d.rast nidp
d.rast.arrow -a drain_tx type=drainage

Worked as expected.

Initially, I thought we can simply exit() from D_open_driver() if called from the terminal so we don't have to make any changes to display modules, but not all of them render. For example, d.info doesn't render anything. That's why I changed exit() to return -1. Its (good?) side effect is, no behavioral changes if no code changes.

Is there a different way to tell that a d-command is being used as an interface in d.mon monitors rather than being used for actual rendering of data to a file? Or we say GRASS_REGION is the way and ignore the edge cases?

I think checking MONITOR and GRASS_REGION should be good enough because GRASS_REGION seems to be mainly designed for this purpose (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.)

It would be convenient to have IPC (we used to have one many many years ago for UN*X) so the display library can communicate with the GUI, but now we have non-GUI display drivers and immediate rendering.

[wenzeslaus]

I would be be very careful in limiting GRASS_REGION to displays.

I think checking MONITOR and GRASS_REGION should be good enough because GRASS_REGION seems to be mainly designed for this purpose (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.)

That might have been the original motivation for adding that functionality, but it is compared to the WIND_OVERRIDE mechanism which is general and it has a general name (it is not called GRASS_DISPLAY_REGION or GRASS_RENDER_REGION).

GRASS_REGION is used in r.import, gui/wxpython/psmap, gui/wxpython/iscatt, and many other places in gui/wxpython besides standard 2D rendering. It is also crucial for workflow(script)-level parallelization and scripting in general. It enables controlling computational region without need to deal with files which brings issues such as cleaning temporary files and race conditions. Since GRASS_REGION is a environment variable, it is passed to subprocesses just like GISRC and the runtime environment needed to run GRASS tools, so it is easy to integrate into scripting and complex workflows. (It actually works so well that I want mask to work the same: #2392.)