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

feature request: custom CRS support #233

Closed
peter00k opened this issue Feb 1, 2016 · 32 comments
Closed

feature request: custom CRS support #233

peter00k opened this issue Feb 1, 2016 · 32 comments
Assignees
Labels
type: enhancement Adds a new, backwards-compatible feature
Milestone

Comments

@peter00k
Copy link

peter00k commented Feb 1, 2016

Hi,

Leaflet supports only few projection out of the box. These projections (like WGS84) are mainly suitable to show a map of the entire world. A lot of use cases only require to show a map of a certain area (e.g. country, state, etc.). Depending on the latitude the standard projections can create maps that are very malformed (instead of what maps.google.com suggests Alaska does not represent ~30-35% of United States landmass).

There is a JS library called Proj4leaflet that extends on leaflet and adds supports for all major projections. It would be nice to have these available in leaflet for R. Proj4leaflet provides a branch for leaflet 0.7.x and for the 1.0 branch of leaflet (now in beta 2). Maybe a future switch to the 1.0 branch of leaflet (issue 175) could provide an opportunity to also support all major projections through proj4leaflet.

Unfortunately my JS skills are close to non-existent (which is the reason why i'm so happy with rstudio's shiny!) so i don't really know how to contribute myself. What are the plans on switching to leaflet 1.0? Are you waiting for a stable release of leaflet 1.0?

Peter

@jcheng5
Copy link
Member

jcheng5 commented Feb 1, 2016

That would be really nice.

I haven't moved to 1.0 because of lack of time. First step would be to make sure all the plugins we support have moved to 1.0 too.

@bhaskarvk
Copy link
Collaborator

Proj4js is on my radar of plugins to incorporate. Hoping to find some free time.

@maarten-vermeyen
Copy link

I'm also interested in this feature. I find reprojecting large datasets to be rather expensive and prefer to work with geographical data in it's native CRS. Reprojecting can also result in topological errors if the data wasn't created with topology in mind.

Are there developer guidelines for working on leaflet?

@jhollist
Copy link

jhollist commented Apr 7, 2016

👍

@maarten-vermeyen
Copy link

This issue seems like a duplicate of / related to #24.

@gisma
Copy link

gisma commented Apr 12, 2016

I absolutely appreciate the integration of proj4leaflet in leaflet for R. Up to this point the functions projView and makeTile from the develop branch of the mapview package might be useful.

You will find some explanations at the projView vignette and with respect to projected local map tiles you may have a look at makeTile help.
cheers chris

@jcheng5 jcheng5 added this to the v1.1 milestone Sep 1, 2016
@jcheng5
Copy link
Member

jcheng5 commented Sep 1, 2016

@bhaskarvk Would be great to investigate this in the v1.1 timeframe (still use proj4leaflet? or move to Leaflet 1.0? etc.) though not necessarily fix for this release.

@bhaskarvk
Copy link
Collaborator

Some discussion here for ref. r-spatial/mapview#13.
The biggest issue is the CRS needs to be specified at Map creation which happens inside the Widget factory/initialize function, where there is no way to pass params.

This the current code. Passing a CRS to it is not going to be easy. Probably impossible.

HTMLWidgets.widget({
  name: "leaflet",
  type: "output",
  initialize: function(el, width, height) {
    // hard-coding center/zoom here for a non-empty initial view, since there
    // is no way for htmlwidgets to pass initial params to initialize()
    let map = L.map(el, {
      center: [51.505, -0.09],
      zoom: 13
    });

@bhaskarvk bhaskarvk self-assigned this Sep 15, 2016
@timelyportfolio
Copy link
Contributor

Probably a dumb question, but does the Map creation need to occur in initialize. Leaflet is far more complicated than most of my simple htmlwidgets, but in general I prefer to defer creation until render when we have x.

@bhaskarvk
Copy link
Collaborator

We are thinking along the same lines. I'll have to dig thru. HTMLWidget code to see how to do that.

@timelyportfolio
Copy link
Contributor

I would start by just copying/pasting all the initialize code to before doRenderValue in render. Happy to submit pull if easier.

@bhaskarvk
Copy link
Collaborator

Don't bother, I'm almost half way there. Trying to refactor it to new binding specs of htmlwidgets 0.6 too.

@timelyportfolio
Copy link
Contributor

great, 2-in-1 :)

@bhaskarvk
Copy link
Collaborator

Ideally htmlwidget's factory's return object should support a 'initialize' callback, which gets called right after the JS deps are loaded, & before renderValue.

@bhaskarvk
Copy link
Collaborator

Might not be as straight forward, there's the shiny part too.

@bhaskarvk
Copy link
Collaborator

I think I've made progress on this front. Expect to see results in a day or two.

@bhaskarvk
Copy link
Collaborator

Got it! http://rpubs.com/bhaskarvk/proj4leaflet
I still need to iron out the API, but we got by jove.

@bhaskarvk bhaskarvk added type: enhancement Adds a new, backwards-compatible feature in progress labels Sep 15, 2016
@gisma
Copy link

gisma commented Sep 15, 2016

Thanks great job!
Way more comfortable and generic than the "clunky" wokraround in projView. Now real mapping and adequate spatial visualisation can start.

@bhaskarvk
Copy link
Collaborator

bhaskarvk commented Sep 16, 2016

OK there's a minor potential incompatibility here.
In earlier HTMLWidgets (pre 0.6) you returned the widget object which in our case happens to be the Map. so you could do

leaflet() %>% addTiles() %>%
htmlwidgets::onRender("
 new L.divIcon(....).addTo(this);
")

where the this was an instance of L.map.
Now in order to support passing options to the L.map constructor, I moved the map creation inside of the render code, but I also had to adapt to the newer binding specs of htmlwidgets 0.6
and here you return an object with contains your map instance along with factory, renderValue and resize functions.
http://www.htmlwidgets.org/develop_intro.html
So now in onRender you can't use this to refer to the L.map instance, rather you'll have to get your map using this.getMap(). Where getMap() is a newly added function that returns the map object.

This will mean every onRender implementation needs to change. This is not a API change perse as the R API is same.

Thoughts/comments @jcheng5 / @timelyportfolio , others ?

@bhaskarvk
Copy link
Collaborator

FWIW my inclination is to go ahead and introduce this change because the pros (control over map instance creation) out weigh the cons (potential rewrite of code inside onRender).
onRender is a special use case anyways and in no way will affect majority of the user base. But I'm interested in opinion from long time users and core devs.

@bhaskarvk
Copy link
Collaborator

bhaskarvk commented Sep 16, 2016

This is an working example with my change

leaflet(mapOptions = list(crs=list('_class'='L.CRS.Simple'))) %>%
  fitBounds(0,0,1000,1000) %>%
  # for now via onRender coz we don't have R API for L.ImageOverlay
  htmlwidgets::onRender("
  function(el, t) {
    var myMap = this.getMap();
    var bounds = myMap.getBounds();
    var image = new L.ImageOverlay(
                      'http://leafletjs.com/examples/crs-simple/uqm_map_full.png',
                      bounds);
    image.addTo(myMap);
  }
  ")

@timelyportfolio
Copy link
Contributor

I agree with you @bhaskarvk that benefits greatly outweigh the inconvenience.

@bhaskarvk
Copy link
Collaborator

Here's the updated version now with demos for L.CRS.Simple and some other Projections.
http://rpubs.com/bhaskarvk/proj4leaflet

@jcheng5
Copy link
Member

jcheng5 commented Sep 17, 2016

Let's discuss our options. I really hate to break compatibility if there's a way to avoid it. It's drastic but we could modify htmlwidgets to let us decorate the call to the onRender callback, for example. The big downside is then having to release htmlwidgets to CRAN first.

@tim-salabim
Copy link
Contributor

I also agree with @bhaskarvk that the benefits are huge in comparison to the potential problem of breaking onRender calls. As from a mapview point of view we do have a couple of functions using onRender which we could quickly fix to accommodate changes. On the other hand, spatial analysis without visual support for special CRSs is pretty useless, so this is a must have for a package like mapview. Therefore, I'd happily fix some broken bit, if this means we can finally get proper redering of data with abitrary CRS. I think the vast majority of spatial analysts will agree with this.

@jhollist
Copy link

Agree completely with @tim-salabim! CRS support is huge!

@bhaskarvk
Copy link
Collaborator

I've been thinking, the minor incompatibility is not due to CRS support per se. It's due to how htmlwidget's contract changed in 0.6.
So at any time we migrated Leaflet's code to htmlwidget 0.6 we will have to implement this change.
So better we do it now.
Also onRender is a very very specialized use case, so I think the people affected by this is very minimal, and besides the the actual R or javascript API is not changing, we're just changing (rather htmlwidgets 0.6 is changing) what the 'this' object refers to in the onRender function.

@jcheng5
Copy link
Member

jcheng5 commented Sep 18, 2016

Don't worry, I'm not suggesting not to implement the CRS support, just to look for a way to do so without breaking onRender code that's already out there. I just want the chance to try myself before giving up on the idea of doing it in a backwards compatible way.

@jcheng5
Copy link
Member

jcheng5 commented Sep 18, 2016

@bhaskarvk is there a branch I can look at yet? Even if it's half baked?

@bhaskarvk
Copy link
Collaborator

Coming up. I had one up but it was not even quarter baked. I'll push out one for you to look at in an hour or so.

@bhaskarvk
Copy link
Collaborator

Created PR #294

@bhaskarvk
Copy link
Collaborator

Proj4Leaflet support merged in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Adds a new, backwards-compatible feature
Projects
None yet
Development

No branches or pull requests

8 participants