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

Provide parameter for retrieving additional data around tile edges to minimise edge effects #104

Merged
merged 8 commits into from May 8, 2019

Conversation

rowanwins
Copy link
Contributor

@rowanwins rowanwins commented May 4, 2019

As proposed in #56 this PR aims to reduce the edge effects associated with tiles by introducing a tile_edges_padding parameter. Additional data is retrieved around the tiles edges during the tile generation process which helps to reduce sharp edges that occur due to resampling.

@vincentsarago did suggest

and then overwrite it to 0 if we don't need resampling (if the internal tiles match the requested bounds)

I'm not really sure how we'd know about the internal tiles so happy to take any advice on this one :)

Let me know if you need anything else on this.

@vincentsarago
Copy link
Member

thanks @rowanwins this is great. I need to think about how to check if the tile is aligned then also check the performances (which should not vary).

rio_tiler/utils.py Outdated Show resolved Hide resolved
@vincentsarago
Copy link
Member

@rowanwins maybe we can do something like

def is_aligned(src_dst, bounds, tilesize):
    """Check if tile is aligned with internal tiles."""
    if src_dst.crs != CRS.from_epsg(3857):
        return False

    col_off, row_off, w, h = windows.from_bounds(
        *bounds, height=tilesize, transform=src_dst.transform, width=tilesize
    ).flatten()

    if round(w) % 64 and round(h) % 64:
        return False

    if (src_dst.width - round(col_off)) % 64:
        return False

    if (src_dst.height - round(row_off)) % 64:
        return False

    return True

The idea is to check:

  • if the CRS is EPSG:3857
  • if the size of the window is a multiple of 64 px (minimum size of internal tile)
  • if the offset is a multiple of 64px

why 64px ?

ideally we want to check if the window is a multiple of the internal tile size (raw data and overview) but we don't have a way to do it with rasterio and I'm not sure this is needed. By checking if we have a multiple of 64 we make sure that even on border tiles we don't need to do resampling.

aligned_tile

Note: this might be totally wrong, but did a quick check with a web-optimized file and it work

rio_tiler/utils.py Outdated Show resolved Hide resolved
…ad. Clarify param in docstring. Provide better default window.
@rowanwins
Copy link
Contributor Author

Hi @vincentsarago

I've made those changes.
I think it would be good to add a test around the _requested_tile_aligned_with_internal_tile however I wasn't sure if any of the test fixtures had those internal web-optimised tiles, if you can point me in the right direction there that would be great.

@vincentsarago
Copy link
Member

@vincentsarago
Copy link
Member

@rowanwins The PR looks excellent (I like the tests you added).
Couples things before we merge this:

Thanks

@rowanwins
Copy link
Contributor Author

rowanwins commented May 8, 2019

Hi @vincentsarago

I attempted to change the base but I'm not sure if that worked TBH (I ran git rebase --onto maint-1.0 followed by git push -force origin pad-tile), my git skills are average :)

I think everything else is ticked off other than codecov complaining about a decrease in coverage. It would be good to test that _requested_tile_aligned_with_internal_tile function a little more, I'm just not entirely sure how...

@vincentsarago
Copy link
Member

this is great @rowanwins I'll take it from here. thanks for your help

@vincentsarago vincentsarago changed the base branch from master to maint-1.0 May 8, 2019 03:41
@rowanwins
Copy link
Contributor Author

Out of interest how did you change the base branch @vincentsarago ?

@vincentsarago
Copy link
Member

@rowanwins In fact I had the option in the Github UI 😬 (I didn't know I could do that before)

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

Successfully merging this pull request may close these issues.

None yet

2 participants