search

GeoTIFF / georaster-layer-for-leaflet

Display GeoTIFFs and soon other types of raster on your Leaflet Map
https://geotiff.github.io/georaster-layer-for-leaflet-example/
Apache License 2.0
297 stars 58 forks source link

use new georaster getValues method for better COG support #11

Closed mhiley closed 5 years ago

mhiley commented 5 years ago

Hi @DanielJDufour, this is a first stab at an implementation for #10 . There is probably some cleanup worth doing but I wanted to get your thoughts on whether I'm headed in the right direction.

The basic strategy is: If the georaster was constructed form a URL, then for each tile determine the extent and dimensions needed to render that tile, and make a single georaster.getValues to get the data needed for the tile being rendered. The render loop is now split into two loops: a first pass to determine the extent for the getValues call, and a second pass to actually do the rendering as before.

Note, if the georaster had been constructed from a data array, things work essentially the same as before, though the implementation has been altered slightly to better match how I'm doing things for COG (for each tile, build up a 1D array with needed values, then loop over the values again to render them).

One other change is to do the createTile asynchronously which seems to improve browser performance (tiles appear in browser as they finish instead of all at once).

Note this depends on the following georaster PR: https://github.com/GeoTIFF/georaster/pull/31

Let me know if you think this is headed in the right direction and if you have any suggestions! Thanks!

DanielJDufour commented 5 years ago

@mhiley , thank you for the PR! I went over the proposed changes and I have a few questions.

I'm not sure I'm reading it right, but would we be calling L.point and map.unproject 4x for each rendered point? I'm afraid that calling the function two more additional times for each pixel would degrade performance when a user renders a GeoTIFF that they loaded into memory in the browser. If possible, I'd like to avoid that. Part of the reasoning is that some users of geotiff.io have expressed requirements that a high-resolution raster be rendered (which is already tough on performance) and also that we want users to be able to upload their own file to geotiff.io. Basically, any way we can avoid additional function calls or loops is important.

Would it be possible to move the COG related stuff into a separate function like getRasters. Then if rasters doesn't exist execute rasters = await this.getRasters(bounds). Naturally, I think we would want that after we get number_of_rectangles_across and number_of_rectangles_down and before the for loops.

Let me know what you think or if I am making sense! I'm definitely open to new ways to doing things, just as long as performance isn't impacted too much :-)

mhiley commented 5 years ago

@DanielJDufour Thanks for the comments, all good points/ideas. This first pass was definitely focused on "getting it to work" without much focus on performance.

I'm going to make another pass on this to address the performance concerns and will update this PR when I'm finished - hopefully by the end of the week. I'll get back to you soon on your comments on the georaster PR also. Talk to you more soon!

DanielJDufour commented 5 years ago

Great. Thanks!

mhiley commented 5 years ago

@DanielJDufour I updated the PR and am ready for you to take another look. I removed the unnecessary/redundant for-loop and moved the new COG-related logic into its own function getRasters. Let me know what you think - thanks!

DanielJDufour commented 5 years ago

I reviewed you recent changes and great job! I think we're almost ready to merge. The outstanding tasks as far as I see it are: 1) Figure out how to render a non-RGB COG or just require a pixelValueToColorFn value in the options 2) Answer the babel-polyfill question here: https://github.com/GeoTIFF/georaster/pull/31/files 3) Add a simple test to georaster

And then we should be ready to merge and publish.

I don't want to stall your use of the library though, so it's not a requirement for this PR to be merged, but it would be awesome to add a COG example to https://github.com/GeoTIFF/georaster-layer-for-leaflet-example after the new code is published.

Thank you so much!! This is awesome work.

mhiley commented 5 years ago

@DanielJDufour Thanks for the comments, that all sounds good! I should be able to address those remaining tasks early next week (including adding a COG example to georaster-layer-for-leaflet-example).

DanielJDufour commented 5 years ago

fyi, I'm working on updating this code to handle the new version of georaster and to work with UTM rasters.