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

Add support {-y} for TileLayer #8575

Closed
wants to merge 3 commits into from
Closed

Add support {-y} for TileLayer #8575

wants to merge 3 commits into from

Conversation

ShivangMishra
Copy link
Contributor

@ShivangMishra ShivangMishra commented Oct 16, 2022

modified TileLayer::getTileURL to support {-y} syntax from within infinite coordinate systems.
The change is safe - the only new behavior is supporting {-y} in some cases which previously were unsupported.

Fixes: #8395

@Falke-Design Falke-Design changed the title fixed issue #8395 Add support {-y} for TileLayer Oct 16, 2022
@Malvoz Malvoz added the feature label Oct 17, 2022
Copy link
Member

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't checked the code but a test is needed anyway

@ShivangMishra
Copy link
Contributor Author

@Falke-Design I need to add a test ?

@Falke-Design
Copy link
Member

Falke-Design commented Oct 17, 2022

For some reason there was a expliciet test for {-y} to be not set on infinite crs (this is also then one who is failing):

it('Does not replace {-y} on map with infinite CRS', function () {
var simplediv = document.createElement('div');
simplediv.style.width = '400px';
simplediv.style.height = '400px';
simplediv.style.visibility = 'hidden';
document.body.appendChild(simplediv);
var simpleMap = L.map(simplediv, {
crs: L.CRS.Simple
}).setView([0, 0], 5);
var layer = L.tileLayer('http://example.com/{z}/{-y}/{x}.png');
expect(function () {
layer.addTo(simpleMap);
}).to.throwError('No value provided for variable {-y}');
simpleMap.remove();
document.body.removeChild(simplediv);
});

Added with: #4338

@ShivangMishra
Copy link
Contributor Author

ShivangMishra commented Oct 17, 2022

@Falke-Design This is what I understood -
In the issue #4338 , the problem was that TMS "needs an upper bound for the CRS's Y coordinate, to be able to flip the tile row". In the case of infinite crs, we don't have an upper bound, that's probably why they added the check on {-y}.
In our case, we don't use the upper bound, we simply set {-y} = (-1 * coords.y) - 1.
So, I think the test should be fixed. Do you have any suggestions for me regarding #8575 ?

@Falke-Design
Copy link
Member

@mourner @IvanSanchez maybe you can take a look into this. The tile and rendering logic is still pretty hard for me

@ShivangMishra ShivangMishra closed this by deleting the head repository Oct 17, 2022
@Falke-Design Falke-Design reopened this Oct 17, 2022
@ShivangMishra
Copy link
Contributor Author

@Falke-Design I reforked this repo. Should I open a new PR for this issue #8395 ?

@Falke-Design
Copy link
Member

@ShivangMishra go for it

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

Successfully merging this pull request may close these issues.

Extend support for {-y} tiled maps into infinite
3 participants