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

Custom CRS Problem #5246

Open
marioatmicello opened this issue Jan 12, 2017 · 6 comments
Open

Custom CRS Problem #5246

marioatmicello opened this issue Jan 12, 2017 · 6 comments

Comments

@marioatmicello
Copy link

We have created a custom CRS where the pixel “x” direction is not aligned with the Latitude direction.

As a result, we have found two places (so far) where this causes a problem including the Map.fitBounds function and when instantiating the L.Circle class.

We believe these break down because these functions assume Latitude and pixel “x” are in the same direction.

Is this an implicit assumption in the CRS support?

@IvanSanchez
Copy link
Member

The CRS/projections code is a bit big and I cannot remember all of it right now, but it's certainly possible that such assumptions occur throughout the code.

If you could share your custom CRS and some of the cases that make the problems show up, that'd be much more helpful, as we could trace the code and have a proper look at the problem.

@perliedman
Copy link
Member

perliedman commented Jan 12, 2017

I know that Leaflet is used with projections where the X axis isn't aligned with latitudes, so it should work in general, although there might of course be places where this is a problem. An example of the issues you're seeing is best, like @IvanSanchez says.

In general, anything involving LatLngBounds (like fitBounds) can be problematic with such projections, since the bounds of a dataset in lat/lng space no longer necessarily correspond to the same bounds in projected space; you might have to tweak around this manually. This is the downside of the otherwise handy/elegant convention of using lat/lng throughout the API.

@marioatmicello
Copy link
Author

marioatmicello commented Jan 16, 2017

Expanding on the two examples I referenced in my initial post:

Line 82 and 83 of Circle.js:

this._radius = isNaN(lngR) ? 0 : Math.max(Math.round(p.x - map.project([lat2, lng - lngR]).x), 1);
this._radiusY = Math.max(Math.round(p.y - top.y), 1);

In line 82, it takes a vector in the Longitude direction and assumes the x component of it gives the entire length. When this is not true, the circle radius comes back as too small.

Line 787 of Map.js

boundsSize = L.bounds(this.project(se, zoom), this.project(nw, zoom)).getSize(),

The rotated bounds would be determined by the minimum and maximum x,y coordinates. This line assumes the SouthEast and NorthWest corners are the extreme points which might not be true after rotation.

@perliedman
Copy link
Member

@marioatmicello ok, yes I see the problem with the circle implementation, and you're correct in your initial observation that there are places where Leaflet is still a bit too closely tied to its default projection.

As I see it, there's no way to calculate a circle's radius for a generic CRS without more information about it, and in fact the current implementation is only an approximation even for the default CRS - a circle on the round surface of the earth will not project to an actual circle in mercator, although the result is close for small radii.

Long term, we could improve this by delegating the radius calculation to the CRS instance, to make it possible to override its implementation, but this will not help you right now. A workaround would be to create your own circle class with a better radius calculation for your CRS.

The other example is what I talked about earlier: extreme points in lat/lng space will not map to extreme points in projected space for arbitrary CRS, so bounds calculations with Leaflet's lat/lng API will not work for these. I don't see how we could fix this without at the same time imposing more complexity/worse performance for the common use case (transforming bounds from one CRS to another is complex).

What could be improved is the documentation of this fact and how you can work around it. It might even be possible to write a plugin to make working with this easier.

@marioatmicello
Copy link
Author

@perliedman Thanks for your thoughtful response. You were most helpful. We see the same issues as you have outlined here. We are going to investigate using other CRS (like, potentially, Game) to figure out how to offer rotation. We might revisit our custom CRS in the future as we have learned quite a bit.

@yingjiangqingzhou
Copy link

When CRS did not use CRS.EPSG3857 ,the _containsPoint() count is wrong. It not consider radiusY.

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