moagrius / TileView

TileView is a subclass of android.view.ViewGroup that asynchronously displays, pans and zooms tile-based images. Plugins are available for features like markers, hotspots, and path drawing.
MIT License
1.46k stars 335 forks source link

Issue 510 #521

Closed moagrius closed 5 years ago

moagrius commented 5 years ago

@peterLaurence not a huge rush on this one, I still need to get #511 done, and you're welcome to wait on this one (510) until 511 is up as well (or you can do this one now that's more convenient for you - your call).

TBH I'm not 100% sure what solved this, I just kept trying things, and TBF the remote version on an emulator is very very slow, but on a real device it's pretty snappy and I can't get it to ever miss a tile. I'm inclined to close, despite funky emulator behavior sometimes (it's always slow, but only very rarely misses tiles).

p-lr commented 5 years ago

Ok, will try that tomorrow.

moagrius commented 5 years ago

Thanks! Working on save/restore as we speak...

On Mon, May 20, 2019 at 4:22 PM peterLaurence notifications@github.com wrote:

Ok, will try that tomorrow.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/moagrius/TileView/pull/521?email_source=notifications&email_token=AAFLHIHINBTHL76P2PZNUATPWMJALA5CNFSM4HOE6KKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV2DPOQ#issuecomment-494155706, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFLHIAMXTWCV5DE7JMF72LPWMJALANCNFSM4HOE6KKA .

p-lr commented 5 years ago

So my testing didn't reveal any gaps. But it doesn't really mean it won't happen in some scenario. We know it happened (rarely) in v2. Looking at your changes, I can see that you introduced a retry (a single one). The probably of having a tile decoding failing twice in a row is very low, but I'm wondering if we should set a limit to retries. Also, before attempting a another tile decoding, should we check if the tile is still visible in the viewport?

moagrius commented 5 years ago

So my testing didn't reveal any gaps. But it doesn't really mean it won't happen in some scenario. We know it happened (rarely) in v2.

That's a very good point and I don't want to pretend that it's perfect, but I never get gaps in a real device. I think I've seen a couple missing pieces on an emulator. It might be worth trying that whole "unfilled area" thing again... I'm not sure if the Tile thinks it's decoded but isn't rendering, or the bitmap is null, or that it knows it's not decoded. Once I get the hot issues out of the way, I'll spend some time doing deep investigation on this (like hotspots over every tile so I can tap and check the state - but that's a pretty heavy lift).

I'm wondering if we should set a limit to retries.

so pass an int indicating the number of times it should retry?

Also, before attempting a another tile decoding, should we check if the tile is still visible in the viewport?

That's a very good idea, I'll include that.

p-lr commented 5 years ago

so pass an int indicating the number of times it should retry?

I was thinking of removing the limit of retries, provided the tile is still in the viewport. But it's just a suggestion.

moagrius commented 5 years ago

Gotcha. I think we need some kind of limit in case of missing files or 404s, which would create an infinite loop

p-lr commented 5 years ago

You're right. I had more in mind tiles from assets or sd card.

moagrius commented 5 years ago

So yeah maybe we let them pass in a limit?

p-lr commented 5 years ago

With a default of 1 or 2, yes passing a limit could be interesting.

moagrius commented 5 years ago

will do

moagrius commented 5 years ago

so i'm going to make the updates and merge and don't think i need you to re-review. i have a nutty weekend so it'll probably be early next week. if you could check out #511 and see if you agree that works as a temporary workaround, i'd love to merge that into this, before merging this into master. if not, it's not the end of the world.

fwiw, just in case, i had the tiles stored in google cloud but under my work email, which was linked to my employers account. i deleted that project and am re-uploading to a persona email, but the urls for the remote demo may fail in the branch. you probably wouldn't even have noticed, but i figured worth a mention.

thanks as always

p-lr commented 5 years ago

Ok omw to test it.