d.vect: Apply variable color only to area fill

[wenzeslaus]

Always use (respect) the color option for the area boundary and apply color table
and other variable color options only to the area fill (as fill_color).

Before

After

Previously, this was only possible by drawing the vector again using another layer (or vector map copy in case color table was used).

Trying to get old behavior with the new code

Unfortunately, color=none which disables drawing boundaries leaves thin white (bg color) lines in between the areas, so the old behavior cannot be exactly reproduced like that.

[wenzeslaus]

@landam Can you please comment both on the whether you agree with the change of behavior and on the correctness of the implementation? And/or suggest another reviewer.

Works for me as expected with the vector color table, RGB column, cat-based random colors, and without any of those but color and fill color specified.

[wenzeslaus]

Added images to the description and highlighted the controversy.

[HuidaeCho]

Hi @wenzeslaus. I'm against this PR. I think, with only one option for an RGB column, the current behavior (same color for both area and boundary) is more expected. Also, leaving thin gaps would not be acceptable in some case. Why don't we split rgb_column (currently, both area and boundary) into area and boundary?

• color and rgb_column for feature (border) color
• fill_color and fill_rgb_column (rgb_column if not given to keep the current behavior) for fill (area) color

Last, with this PR, zcolors seems to be ignored.

[HuidaeCho]

I did a little test. By default, the current d.vect only draws points, lines, areas, and faces from the command line. From the GUI, it draws boundaries as well. I created a red-only RGB column redcol for testing. To show you my data,

d.vect map=wps_hu12

To mimic your problem (no separate single boundary color),

d.erase
d.vect map=wps_hu12 rgb_col=redcol
# or
d.vect map=wps_hu12 rgb_col=redcol col=blue # Woops! col=blue doesn't work?

To overlay boundaries (what you want?),

d.erase
# it's a single run
d.vect map=wps_hu12 type=area,boundary rgb_col=redcol col=blue

In d.vect, area borders are drawn by display_area(), but boundaries by display_lines(). So, even now, you don't need to run d.vect twice to achieve what you want (single-color boundaries over RGB-filled areas) if I'm not missing anything.

From the GUI, it just works because boundary is drawn by default using the color option.

Now, do we need variable colors for area borders (separate from fill colors) or boundaries (new)? That's a different story.

[HuidaeCho]

@wenzeslaus Is this PR still relevant? I cannot reproduce this issue. From the NC sample mapset,

g.copy vect=boundary_county,tmp --o
v.db.addcolumn map=tmp col="rgb_color varchar(50)"
v.db.update tmp col=rgb_color querycol="abs(random()%255) || ':' || abs(random()%255) || ':' || abs(random()%255)"
d.mon start=wx0
g.region vect=tmp
d.vect map=tmp rgb_column=rgb_color color=black type=area,boundary

From the GUI, check boundary (default).

[wenzeslaus]

I was concerned with color table meaning the color rules saved with the map ($MAPSET_PATH/vector/country_boundaries/colr) which I created with:

v.colors map=country_boundaries color=inferno

It still seems to behave the same when I use simple d.vect without checking the boundary for type (default in GUI and CLI), color option is not used. Colors of boundaries and fill are the same. When I check it (d.vect...type=point,boundary,area...) I indeed get the desired result. Is this related to the changes you made?

The behavior seems strange since the boundaries are part of areas in GRASS and they are rendered with type=area already (test e.g. by setting line width high). It just happens that type=boundary is rendered after areas, so the originally rendered boundaries get replaced, or is there some smartness there now? How I'm going to explain this to someone?

[wenzeslaus]

@HuidaeCho Looking at comments here again, it seems your first and second test is the same, in both cases you ended up adding boundary, i.e., type=area,boundary, so I don't think anything changed. I think your suggestion is a good workaround for the original problem, but since the boundary rendering is already part of area rendering, I don't think this is resolved.

One use case for this would be the world map we show when GRASS GIS starts the first time. Using a workaround there would be okay-ish temporarily, but we don't want to keep using a workaround there. (Using RGB column there would be fine too in this case possibly lowering priority of this PR/problem.)

So, I think this PR (or another fix) is still relevant.

[HuidaeCho]

I was concerned with color table meaning the color rules saved with the map ($MAPSET_PATH/vector/country_boundaries/colr) which I created with:

v.colors map=country_boundaries color=inferno

It still seems to behave the same when I use simple d.vect without checking the boundary for type (default in GUI and CLI), color option is not used. Colors of boundaries and fill are the same. When I check it (d.vect...type=point,boundary,area...) I indeed get the desired result. Is this related to the changes you made?

The behavior seems strange since the boundaries are part of areas in GRASS and they are rendered with type=area already (test e.g. by setting line width high). It just happens that type=boundary is rendered after areas, so the originally rendered boundaries get replaced, or is there some smartness there now? How I'm going to explain this to someone?

I see. You were not talking about the rgb_column= and color= options of d.vect. Sorry for the confusion. Still, this PR changes the behavior of color= not only with v.colors, but also with rgb_column=. With this PR without a color table,

g.copy vect=boundary_county,tmp --o
v.db.addcolumn map=tmp col="rgb_color varchar(50)"
v.db.update tmp col=rgb_color querycol="abs(random()%255) || ':' || abs(random()%255) || ':' || abs(random()%255)"

d.vect map=tmp rgb_column=rgb_color type=area

doesn't draw the area-color boundaries anymore. It uses the default solid color now and we lose the area-color boundary feature, which I believe we have to keep supporting.

d.vect map=tmp rgb_column=rgb_color color=none type=area

behaves consistently with your v.colors example and leaves gaps, but I would say that is a feature because you explicitly requested no boundaries.

Let me summarize the current behavior without this PR.

v.colors map=country_boundaries color=inferno
d.vect map=country_boundaries       type=area  # draws the area-color boundaries
d.vect map=tmp rgb_column=rgb_color type=area  # draws the area-color boundaries

d.vect map=country_boundaries       color=blue type=area  # draws the area-color boundaries and ignores color=blue
d.vect map=tmp rgb_column=rgb_color color=blue type=area  # draws the area-color boundaries and ignores color=blue

v.colors -r map=country_boundaries 
d.vect map=country_boundaries type=area             # draws grey areas and black boundaries
d.vect map=country_boundaries color=blue type=area  # draws grey areas and blue boundaries

So the only time boundaries are drawn in their own single color is when areas are not assigned any colors either though rgb_column= or v.colors (custom_rgb in the code). That actually makes sense because without any boundary color and with a single area color, it would be just a single-color union polygon of all the features with no boundaries in-between. The last two behaviors are just necessary, but the other ones with either rgb_column= or v.colors, you do have area colors and by default boundaries use the same area color. Yes, boundaries are still drawn not because they are requested, but because we should fill the gaps. I would say this is a feature. If this PR is committed, not only we will lose this feature, but also it will change the current behavior of rgb_column= with column=.

What you need is again type=area,boundary with no code changes.

d.vect map=country_boundaries color=blue type=area,boundary

This is not a temporary workaround. It uses a feature of d.vect. Boundary drawing is separate from area drawing.

In the end, what would be the point of type=boundary if boundaries were drawn together with areas when type=boundary was not requested?

[HuidaeCho]

(test e.g. by setting line width high

I would say that is a bug. d.vect should not draw thick boundaries when no type=boundary is specified. I mean for type=area and maybe face only.

width_scale, default_width and nrec_width from area.c and lines 190-196.

Lines 441-442 in main.c, but then we need it for line features. So, it would be easier to use option checks to avoid thick boundaries for areas only. And move it to shape.c between display_area() and display_line().

[wenzeslaus]

First of all, I'm all for replacing this PR with something better. However, I have some concerns with what you are suggesting which will explain why I though it is only a workaround.

The last two commands in your bigger example miss type=area,boundary if I understand correctly, but assuming I got that right:

Your type=area versus type=area,boundary makes sense, but it seems to go against GRASS vector topological model and intention in d.vect (intention to follow it in the interface). In GRASS topology, an area is centroid and boundary, so one may reasonably expect that type=area includes boundaries too while type=boundary means only boundaries (isn't this the case for the type parameter for vector modules). Nobody wants to draw centroids (usually), so the d.vect doesn't draw them by default. However, it is common to draw areas/polygons/... with a boundary/outline/..., so I think most people would expect an area to be drawn including its boundary. This seems exactly like the intention in d.vect. Boundaries are rendered together with the fill and, perhaps most importantly, boundary is not included in defaults for the type parameter because it is already included in area.

In this perspective, the width behavior makes perfect sense. This PR works with this understanding too. It changes the d.vect behavior so that it assumes not only you don't want centroid to be rendered by default, but newly also assumes that you want boundaries rendered in a constant color.

Your approach seems to say that area for d.vect means skip not only centroids but also boundaries and render only what d.vect calls fill. In that case, I would say, boundary needs to be added to the defaults for type to achieve the expected behavior (and the boundaries with type=area need to ignore width and possibly render only sometimes). However, I have concerns on how this fits with what other modules do the with type parameter (which is perhaps less important because usefulness and intuitiveness may not always mean absolute consistency) and how d.vect is implemented and how it behaves, see, e.g., the following images where lines drawn by boundary and by area follow different rules.

Both figures: Same world map with color table as above, but with type=area,boundary and width=11 and on top of it another d.vect of the same data but without color table, with color set to white, and with fill_color=none.

# bottom
d.vect map=country_boundaries@with_color_table type=point,line,boundary,area,face color=0:29:57:255 width=11
# top
d.vect map=country_boundaries@no_color_table color=255:255:255:255 fill_color=none width=1

[HuidaeCho]

I found this sentence from the d.vect manual:

By default d.vect areas are filled with fill_color and outlined with color. Area outlines can be suppressed with

d.vect map=vector_map color=none

and areas can be made transparent with

d.vect map=vector_map fill_color=none

I think you're right that the original intention was to draw boundaries with areas by default. That's interesting. I always assumed based on the behavior of d.vect that, at least for display purposes, areas are only interiors not including their boundaries. Anyway, if that is the case, this PR actually fixes the same-color bug. Now I want that bug back as a new feature then. Adding a new flag -b (Use area color for boundaries) would be great. Maybe, in a separate PR once you merge it.

[HuidaeCho]

the following images where lines drawn by boundary and by area follow different rules.

Just curious.. Is it intended?

[ninsbl]

Bumping up milestone as 8.0.1 is due in two days, while this has not been part of RC1 and there has not been activity for some time.

[echoix]

Solved conflicts