-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Implement TileJSON bounds property #3703
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! 🙇 I'm excited to work with you to ship this feature soon.
this.cache[i] = [ | ||
[Math.floor(this.lngX(this.bounds.getWest(), i)), Math.floor(this.latY(this.bounds.getNorth(), i))], | ||
[Math.ceil(this.lngX(this.bounds.getEast(), i)), Math.ceil(this.latY(this.bounds.getSouth(), i))] | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the performance impact of not caching these values and doing the calculations from scratch every time hasTile
is called? If we could reduce the amount of state we hold & simplify this logic without a measurable perf impact, that'd be worth looking into.
serialize() { | ||
return { | ||
type: 'raster', | ||
url: this.url, | ||
tileSize: this.tileSize, | ||
tiles: this.tiles | ||
tiles: this.tiles, | ||
bounds: this.bounds, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serialize
is expected to return a plain object (aka JSON serializable object, POJO). this.bounds
is an instance of TileBounds
. We'll need to think of a way to serialize this.bounds
into a plain object, probably by writing a TileBounds#serialize
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why I kept both this.bounds
as unmodified from what the client provides as well as storing the initialized object as this.tileBounds
. The naming at the moment leaves something to be desired for sure. It seems preferable to me that TileBounds not need to worry about how to round-trip whatever source format the tile source uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I was tripped up by the naming of bounds
vs tileBounds
. Curious if there's a quick fix that would make this clearer, perhaps serializedBounds
?
} | ||
} | ||
|
||
module.exports = TileBounds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some linter errors in this file. You can see these errors on the CI build. You can run all the GL JS tests, including the linter, locally per these instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah editor didn't pickup 4 spaces on the new file. Wanted to see that it was on the right track before spiffing up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely on the right track. Spiff away! 👍 You may want to check if there's a plugin for your editor that supports the .editorconfig
spec.
@lucaswoj how do you feel about the approach of delegating hasTile from SourceCache to the Source object? If good, I'll extend that to the vector tile source as well. |
That sounds like the right thing to do. |
Any chance of movement on this? I'd be happy to help tidy up the lint issues, but the other changes will take a bit more for me to grok. |
Any chance this will be merged soon? Really looking forward to get this fixed! :) |
I'm also in need of this feature. |
This is rough code to implement the bounds property in TileJSON to prevent loading/rendering tile data outside an extent. This functionality is useful for eliminating useless network roundtrips (that either result in 404 or an empty response) and eliminating lots and lots of error messages in the console in such a situation. Addresses #1775 and probably others.
If someone can confirm this is something you'd accept and 👍 the approach, I'll clean it up for inclusion.
Launch Checklist