Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebGL not compatible with projected shapes in view mode / tm_basemap(NULL) didn't disable basemaps #972

Open
barryrowlingson opened this issue Dec 2, 2024 · 18 comments

Comments

@barryrowlingson
Copy link

Help page for tm_basemap says:

Use ‘NULL’ in ‘tm_basemap()’ to disable basemaps.

But in both plot and view mode I still get an ESRI base map. I would expect to not see a basemap. Obvious fix is
to not have the tm_basemap() call if you don't want a basemap, but that's harder to do if the basemap is driven
by a variable, eg tm_basemap(whichone) + tm_shape(...) + ...

Ideally I think tm_basemap should be able to specify with arguments no basemap, the default basemap, or a
specific basemap. To preserve backward compatibility I suggest:

  • tm_basemap() : adds default basemap
  • tm_basemap(NULL) : adds default basemap, change documentation to clarify
  • tm_basemap(NA) : no basemap, think of it as a "missing value".
  • tm_basemap(xyz) : add basemap for provider xyz, xyz may also be NULL or NA to get behaviour as above.

Having no tm_basemap in a map would also remain the current behaviour which I think is default basemap in view mode, no basemap in plot mode. But I don't know if this minor inconsistency is worth preserving going forward...

> packageVersion("tmap")
[1] ‘3.99.9003’
@mtennekes
Copy link
Member

Thx for bringing this up!

FYI: the default options for tmap are mode-specific. The mode-specific options are accessible via tmap_options("modes"). The function tmap_options_mode() returns all the options for a specific mode. It's not possible to easily change the default options for a mode. I'm still looking to make this more user-friendly...

There is an option called basemap.show. When TRUE (view mode only) it adds tm_basemap(). However, it does not remove added basemaps. So tm_shape(World) + tm_polygons() + tm_basemap() + tm_options(basemap.show = FALSE) will still show a basemap (actually a choice of three since tmap_options("basemap.server") contains three values.

Apart from the current bug, should we use NULL or NA? One should be used for "not specified, just use the default" and the other for "non-existent". I would propose:

NULL: non-existent
NA: non-specified (so: use default values)

This is also the behaviour in tmap3.

This question is also relevant for visual variables:

tm_shape(World) +
	tm_polygons(fill = NA) 

could/should use the default fill color for polygons (default is grey, but theme dependent)

tm_shape(World) +
	tm_polygons(fill = NULL) 

should not fill the polygons

Does this make sense @barryrowlingson @Nowosad @Robinlovelace @gilbertocamara ?

@barryrowlingson
Copy link
Author

I think I only suggested the opposite sense for NA and NULL to keep compatibility with what happens already with basemaps! But if there's other places they are used then keeping consistent meanings is probably better.

@Robinlovelace
Copy link
Collaborator

```r
tm_shape(World) +
	tm_polygons(fill = NA) 

could/should use the default fill color for polygons (default is grey, but theme dependent)
tm_shape(World) +
tm_polygons(fill = NULL)

should not fill the polygons

Makes sense and sounds good to me 👍

@Nowosad
Copy link
Member

Nowosad commented Dec 2, 2024

For the context, I added examples of the current behavior. The question: wouldn't this change impact many existing code bases?

library(tmap)
data(World)

tm_shape(World) +
        tm_polygons()

tm_shape(World) +
        tm_polygons(fill = NA)

tm_shape(World) +
        tm_polygons(fill = NULL)
#> Error in val[[1]]: subscript out of bounds

@mtennekes
Copy link
Member

The question: wouldn't this change impact many existing code bases?

Do you mean with the existing v4 code bases?

I've checked with v3:

tm_shape(World) +
    tm_polygons()

image

tm_shape(World) +
	tm_polygons(col = NA)

image

tm_shape(World) +
	tm_polygons(col = NULL)
#> Warning message: In rep(x, length.out = nx) : 'x' is NULL so the result will be NULL

image

For the context, I added examples of the current behavior. The question: wouldn't this change impact many existing code bases?

library(tmap)
data(World)

tm_shape(World) +
        tm_polygons()

tm_shape(World) +
        tm_polygons(fill = NA)

tm_shape(World) +
        tm_polygons(fill = NULL)
#> Error in val[[1]]: subscript out of bounds

@Nowosad
Copy link
Member

Nowosad commented Dec 2, 2024

👍🏻 Ok, now I understand the need for the consistency here (I've been using tmap4 for so long that I do not remember the old ways of doing things)

@barryrowlingson
Copy link
Author

As a clarification, when you say "tmap4" does that mean version 3.99.xxx as is the current master branch? Or is that the last ever 3.x version? There is a v4 branch with somewhat older changes, but the instructions for "Installation of tmap (version 4) is straightforward:" seem to get master from github.

@Robinlovelace
Copy link
Collaborator

As a clarification, when you say "tmap4" does that mean version 3.99.xxx as is the current master branch?

Yes that's correct. v4 is on the main branch.

@barryrowlingson
Copy link
Author

I don't see a "main" branch, only a "master" branch and a "v4" branch... I'm assuming the "v4" branch is stale in some sense, that the "master" branch is the latest tmap version 4 code base, and that someone didn't change all their "master" branches to "main" :)

@Robinlovelace
Copy link
Collaborator

master is the main branch. The default one.

@Robinlovelace
Copy link
Collaborator

I'm assuming the "v4" branch is stale

Correct. I just deleted it to avoid others getting confused, thanks for the nudge.

mtennekes added a commit that referenced this issue Dec 3, 2024
mtennekes added a commit that referenced this issue Dec 3, 2024
@mtennekes
Copy link
Member

Done!

FYI (for developers), see

tmap_options("value.const", "value.blank")

The option value.const are the constant visual values used when set to NA, and the option value.blank contains the "invisible" visual values. Note that there is also an option "values.null", used for filtered out features, e.g. tm_shape(World, filter = World$continent == "Africa") + tm_polygons().

Visual variables

# default fill (grey)
tm_shape(World) +
	tm_polygons(fill = NA)

# default no fill
tm_shape(World) +
	tm_polygons(fill = NULL)

# default fill (grey), no borders
tm_shape(World) +
	tm_polygons(fill = NA, lwd = NULL)

Basemaps

# Default basemap(s)
tm_shape(World) +
	tm_polygons() +
	tm_basemap()

# Other basemap
(tm = tm_shape(World) +
	tm_polygons() +
	tm_basemap("OpenStreetMap"))

# Adding another aux layer (tm_grid) and removing basemaps
(tm2 = tm +
	tm_grid() +
	tm_basemap(NULL))

# Add basemap again  
tm2 +
	tm_basemap()

Please test carefully (as we are close to CRAN submission).

I haven't update the docs yet. Feel free to do so (PRs welcome)

@Nowosad
Copy link
Member

Nowosad commented Dec 16, 2024

I've tried it, and there seems to be an issue for a non-global case:

library(tmap)

# works fine
(tm = tm_shape(NLD_dist) +
    tm_polygons() +
    tm_basemap("OpenStreetMap"))

# works not fine
(tm2 = tm +
    tm_grid() +
    tm_basemap(NULL))

@mtennekes
Copy link
Member

I get this for the second plot:

image

Seems alright to me.

@Nowosad
Copy link
Member

Nowosad commented Jan 16, 2025

@mtennekes the issue is in the view mode:

library(tmap)
tmap_mode("view")
# works fine
(tm = tm_shape(NLD_dist) +
    tm_polygons() +
    tm_basemap("OpenStreetMap"))

# works not fine (cannot see the polygons)
(tm2 = tm +
    tm_grid() +
    tm_basemap(NULL))

@mtennekes
Copy link
Member

Funny. It seems to be an issue with NLD_dist.

NLD_muni and NLD_prov both work well. (Also for you?)

tm_grid is not supposed to be working with projected shapes in view mode, but tm_mouse_coordinates indicates that the shape is correctly projected:

tm_shape(NLD_muni) +
	tm_polygons() +
	tm_basemap(NULL) + 
	tm_mouse_coordinates()

Glad this is not a major bug (bad timing:-))

@mtennekes mtennekes changed the title tm_basemap(NULL) doesn't disable basemaps NLD_dist issue / tm_basemap(NULL) didn't disable basemaps Jan 16, 2025
@mtennekes mtennekes added the bug label Jan 16, 2025
@mtennekes mtennekes changed the title NLD_dist issue / tm_basemap(NULL) didn't disable basemaps WebGL not compatible with projected shapes in view mode / tm_basemap(NULL) didn't disable basemaps Jan 16, 2025
@mtennekes
Copy link
Member

mtennekes commented Jan 16, 2025

It seems to be an issue with NLD_dist.

Nope. NLD_dist is fine.

Apparently projected shapes in view mode don't work with WebGL enabled (which is the default for large shapes, and NLD_dist is considered large). This also works:

tm_shape(NLD_dist) +
	tm_polygons() +
	tm_basemap(NULL) + 
	tm_mouse_coordinates() +
	tm_view(use_WebGL = FALSE)

So still a bug, bug minor, and probably something upstream @tim-salabim does leafgl work with projected shapes and L.CRS.Simple?

@tim-salabim
Copy link

Yeah, seems like it's not compatible with L.CRS.Simple... I have no idea why, but a guess is that it doesn't handle the maps latLngToLayerPoint() function properly. Maybe @trafficonese has any ideas/suggestions?

Here's a minimal reprex

library(leaflet)
library(leafgl)
library (tmap)
library(sf)

data("NLD_dist")

leaflet(options = leafletOptions(
  crs = leafletCRS(crsClass = "L.CRS.Simple")
  , minZoom = -1000
)) |>
  addGlPolygons(data = st_cast(NLD_dist, "POLYGON"))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants