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

[HuidaeCho]

This PR uses #3482 to allow displaying raster numbers for a smaller display extent (not computational region) than the suggested 200x200. Do we still want this limitation when d.rast.arrow doesn't have any or should we add the same limit to d.rast.arrow?

[echoix]

It seems the same pattern as #3491 and #3492 that I understood, but here the logic is done a bit earlier, but after calling the parser. I don't see an obvious reason why it should shouldn't work: it is shortcircuiting extra validation steps if it will be entered again in a second pass.

[wenzeslaus]

Do we still want this limitation when d.rast.arrow doesn't have any or should we add the same limit to d.rast.arrow?

The limitation works very poorly in GUI and it does not makes sense in number of cells because what really matters is the size of a cell in rendered pixes. We probably don't what to go that direction and we don't want to keep increasing the limit based on available hardware. Overall, it would be great if you can remove it as part of this update.

[HuidaeCho]

It seems the same pattern as #3491 and #3492 that I understood, but here the logic is done a bit earlier, but after calling the parser. I don't see an obvious reason why it should work: it is shortcircuiting extra validation steps if it will be entered again in a second pass.

All three PRs have the same pattern: D_open_driver() after G_parser() and before G_get_window(), in d.rast.(arrow|num), to grab GRASS_REGION.

1. d.* modules run from the terminal (no GRASS_REGION defined)
2. D_open_driver() spawns render.py and exits if GRASS_RENDER_IMMEDIATE is not defined (not defined in the terminal) and either (not both) GRASS_RENDER_COMMAND or MONITOR is defined. In this case, MONITOR is defined so render.py will be spawned, which will run the same display module in its render().
3. That same display module still cannot see GRASS_REGION because its parent render.py was spawned from the terminal, but this time, the monitor's render.py read in env file which contains GRASS_RENDER_IMMEDIATE. Now because GRASS_RENDER_IMMEDIATE and MONITOR are defined, D_open_driver() doesn't spawn render.py again and goes on to check GRASS_REGION and return -1 (GRASS_REGION not available in the terminal yet)
4. In step 3, when render.py was called, it added the command line for the display module to the cmd file, which the monitor GUI (gui/wxpython/mapdisp/main.py) checks in its watcher every 0.5 second and triggers an event for rendering all layers in GetLayersFromCmdFile().
5. GetLayersFromCmdFile() above executes all display commands in the cmd file, but now with the display extent defined in GRASS_REGION.
6. Every display module calls D_open_driver(), which will now return 0 because GRASS_REGION is defined from the GUI.
7. Rendering occurs happily and quickly only within GRASS_REGION.
8. Any further changes to GRASS_REGION within the monitor GUI will move to step 5.

[echoix]

It seems the same pattern as #3491 and #3492 that I understood, but here the logic is done a bit earlier, but after calling the parser. I don't see an obvious reason why it should work: it is shortcircuiting extra validation steps if it will be entered again in a second pass.

I'm so sorry, I thought I wrote:

I don't see an obvious reason why it shouldn't work. Probably an autocorrect thing.

[HuidaeCho]

It seems the same pattern as #3491 and #3492 that I understood, but here the logic is done a bit earlier, but after calling the parser. I don't see an obvious reason why it should work: it is shortcircuiting extra validation steps if it will be entered again in a second pass.

I'm so sorry, I thought I wrote:

I don't see an obvious reason why it shouldn't work. Probably an autocorrect thing.

No problem. Now we have more detail documentation about how D_open_driver() works. ;-).

[echoix]

Thanks for the very detailed and simple explanations! It'll take some time until I master a bit of every part of GRASS, but it's a good start, and way easier in words like this, I'm lucky for it!

My initial comment when "approving" was to highlight the way I thought it was ok, from the two previous, a bit like a proof by mathematical induction, since there was something that prevented me to say it's exactly the same.

[neteler]

Now we have more detail documentation about how D_open_driver() works. ;-).

Thanks for writing it up! Would it make sense to move it to the doxygen section as it is easily lost in a PR discussion?
Like here: https://grass.osgeo.org/programming8/displaylib.html

[HuidaeCho]

Now we have more detail documentation about how D_open_driver() works. ;-).

Thanks for writing it up! Would it make sense to move it to the doxygen section as it is easily lost in a PR discussion? Like here: https://grass.osgeo.org/programming8/displaylib.html

Now with #3500, it's much simpler from the developer's perspective. No special consideration is necessary.

[wenzeslaus]

I agree. Opening the driver should happen right after the parser given the monitor architecture.

[HuidaeCho]

Do we still want this limitation when d.rast.arrow doesn't have any or should we add the same limit to d.rast.arrow?

The limitation works very poorly in GUI and it does not makes sense in number of cells because what really matters is the size of a cell in rendered pixes. We probably don't what to go that direction and we don't want to keep increasing the limit based on available hardware. Overall, it would be great if you can remove it as part of this update.

Actually, the -a flag uses the number of raster cells always and "Constrain display resolution to computational settings" does a similar thing with computational resolution without this flag. Based on my test, the only time it uses the number of pixels is without these two, but then you'll just see a black (or green) rectangle because d.rast.num (or d.rast.arrow) just draws numbers (or arrows) per pixel, which doesn't make sense.

Without

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

Lines 207 to 222 in 9c3b67e

	if ((nrows > 75) || (ncols > 75)) {
	G_asprintf(&tmpstr1, n_("%d row", "%d rows", nrows), nrows);
	G_asprintf(&tmpstr2, n_("%d col", "%d cols", ncols), ncols);
	/* GTC %s will be replaced by strings "X rows" and "Y cols" */
	G_warning(_("Current region size: %s X %s\n"
	"Your current region setting may be too large. "
	"Cells displayed on your graphics window may be too "
	"small for cell category number to be visible."),
	tmpstr1, tmpstr2);
	G_free(tmpstr1);
	G_free(tmpstr2);
	}
	if ((nrows > 200) || (ncols > 200)) {
	G_fatal_error(_("Aborting (region larger then 200 rows X 200 cols is "
	"not allowed)"));
	}

# in NC sample dataset
d.mon wx0
g.region rast=elevation
d.rast elevation
d.rast.num elevation

produces

and is slow.

I think this limitation can be useful if it is customizable using either a module parameter or an environment variable. For example, if a raster map is large with a lot of cells, it will automatically not render at a scale closer to its full region and will start rendering when the map is zoomed in closely. Like min and max visibility scales. It'll help with accidental zoom out and this has happened a lot to me in g.gui with d.rast.arrow (I had to kill it manually from the terminal).

[wenzeslaus]

Some "smart" behavior (aka heuristic) as default combined with a way to pick behavior manually by parameter is a solution I have seen working.