Add panBuffer feature to include extended tilerange #1405
Conversation
| tileRange = tileRange.extend(CustomPoint( | ||
| math.max(_globalTileRange.min.x, tileRange.min.x - panBuffer), | ||
| math.max(_globalTileRange.min.y, tileRange.min.y - panBuffer))); | ||
| tileRange = tileRange.extend(CustomPoint( |
There was a problem hiding this comment.
We could use a condition if (panBuffer > 0) to avoid unecessary computation on tileRange 🤔
Otherwise the code looks good to me
There was a problem hiding this comment.
Yes, I think I like that, every little helps!
There was a problem hiding this comment.
Ok, have added an if( panBuffer != 0), maybe it should be > 0, but maybe there is some weird reason for someone to reduce it as well that I can't think of (although maybe there could be bugs if they did that, but there is a bit of caveat emptor or whatever it is)!
There was a problem hiding this comment.
Was thinking about it too but I supposed that it wouldn't make sense to buffer -1 tiles 🤔
|
Also I guess an example could be included (it will facilitate tests too) |
|
Well, it's literally only one line to test it, adding panBuffer = 1 to any example, so feels a little unnecessary and just adding to extra maintenance if we make breaking changes to options and have to refactor examples ? |
|
Although, maybe it could be added to a generic tile test widget (maybe adding to the existing example there?) as a button toggle ? |
Yeah it seems good to me it's mostly to showcase the difference between buffering tiles vs. no buffer |
|
Ok, I've added it as a toggle button on the tile_builder_example.dart (so will be 0/1). Worth testing on both mobile and web (as likely more tiles loaded on a large screen like web on desktop). |
|
@ibrierley I'll see if I can get round to testing this this evening or tomorrow. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
Wow, I'm really good at testing stuff. Sorry about that, I'll try to get it done soon :D |
There was a problem hiding this comment.
LGTM. I've done some basic testing, and I can't see any side effects. Also submitting on behalf of @TesteurManiak.
PS: Is it just me, or has the example page that you've changed had a hit to performance (with and without this feature)?
|
When you say with/without the feature...have you tried comparing to a previous commit, or are you just testing with it on/off and feel its slower ? |
|
@ibrierley I was just toggling the feature on and off. I didn't test any other screens, and I was only in debug mode in Chrome. Panning just felt more janky than usual. |
|
Hah sorry, (still a bit unclear). So feature on=performance hit ? (and no performance hit if feature is off?) I would expect this (and mentioned that in first comment when considering this one), and I wouldn't recommend having this on really (hence default off). Problem is, it has to load an extra number of tiles at edges, and if that's on each side, that's not an insignificant amount. Then take that into account when zooming and it will probably feel a little hitchy. Especially when it crosses a zoom threshold. If you mean its still janky even with feature off, then let me know and I'll do some more digging. |
|
Yep, my fault. The panning seems janky even when this is off. But that might just be me misremembering or something, so you might want to take a look. |
|
I don't experience it when its off, or on a previous commit before, so I can't see anything obvious, but am happy to wait until you've had a chance to test to see if you can replicate. |
This is a possible example for #1337 (nice PR number btw :D) if I'm understanding it correct. (preloading can be slightly ambiguous).
Note, this extends the tilerange in each direction by an integer, and will affect performance, maybe good in one sense, for preloading for panning, but the extra tileloads will have some extra impact in terms of waiting for tile loads, decoding, potential costs etc. So this needs some extra thought and testing if it's really what we want.
If this is purely for panning only, it could be more efficient to only load tiles ahead of the direction that's being panned (lets say we pan +X,-Y we don't need to extend -X,+Y), but that would probably add some extra complexity.
Needs proper testing :).