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

[BUG] TileLayer lag/jitter #1245

Closed
3 of 11 tasks
Tracked by #1165
rorystephenson opened this issue May 23, 2022 · 19 comments · Fixed by #1247
Closed
3 of 11 tasks
Tracked by #1165

[BUG] TileLayer lag/jitter #1245

rorystephenson opened this issue May 23, 2022 · 19 comments · Fixed by #1247
Labels
bug This issue reports broken functionality or another error

Comments

@rorystephenson
Copy link
Contributor

rorystephenson commented May 23, 2022

Describe The Bug
When doing animated panning in flutter web using the example app (Animated Mapcontroller page) the markers (and any other non tile layer) seem to jitter against the map tiles.

Expected Behavior
The markers and other non tile layers should move together with the tile layer.

Screenshots & Recordings

flutter-map-panning-jitter.mp4

Additional Information
In the example I slowed the animation to 5 seconds to make the issue more obvious. I have not observed this issue when running the example on the Android emulator, only with flutter web on chrome. This is encountered both using the latest published version of flutter_map as well as master. Flutter version 2.10.5.

When I run in release mode the effect is more subtle but there is still jittering. In any case the movement of the markers and the map tiles should be synchronised so that any jitter/jank should affect them together.


Doctors Report

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel stable, 2.10.5, on macOS 11.5.2 20G95 darwin-x64, locale en-GB)
[✓] Android toolchain - develop for Android devices (Android SDK version 32.1.0-rc1)
[✓] Xcode - develop for iOS and macOS (Xcode 13.2.1)
[✓] Chrome - develop for the web
[✓] Android Studio (version 2021.1)
[✓] VS Code (version 1.65.0)
[✓] Connected device (2 available)
[✓] HTTP Host Availability

To Reproduce
Steps to reproduce the behavior:

  1. Edit the example app to slow the animation of the Animated Mapcontroller page movement to 5 seconds.
  2. Run the example app with flutter web and go to the animated mapcontroller page.
  3. Click the different location buttons and observe the jitter during panning.

Severity
This will help us to label the issue quicker and decide what needs attention first. Only choose fatal if the entire app crashes, otherwise choose non-fatal.

  • Non-Fatal
  • Fatal / App Crashes

Frequency/Rarity
This will help us to label the issue quicker and decide what needs attention first.

  • Once
  • Uncommon
  • Common
  • Always

Applicable Platforms
Only select those that you've tested on - one or more. If possible, test on a variety of platforms.

  • Android
  • iOS
  • Web
  • Windows
  • Others (beta platforms)
@rorystephenson rorystephenson added bug This issue reports broken functionality or another error needs triage This new bug report needs reproducing and prioritizing labels May 23, 2022
@JaffaKetchup

This comment was marked as off-topic.

@JaffaKetchup JaffaKetchup added non-fatal and removed needs triage This new bug report needs reproducing and prioritizing labels May 23, 2022
@ibrierley
Copy link
Contributor

I don't get a problem on this for the Animated Controller example (in release or debug mode), is this on a very old device ? Are you using the latest from Git ?

Try a, flutter pub deps | grep flutter_map and check it says 0.15.0.

I'd be surprised if its related to slow calculations, there's only 3 markers there.

@ibrierley
Copy link
Contributor

Out of interest, I've created a webspace where I can publish the web stuff to, I'm going to try and have one for each version (happy to be added to the docs if wanted). Example..

https://fluttermap.dabbles.info/v0.15/#map_controller_animated (base at https://fluttermap.dabbles.info/v0.15/ )

For me, when I drag, I don't have a problem, however I do note if I click on Paris, then Dublin etc, it's almost as if the Markers move ahead and back as part of the animation (but again, doesn't if you drag them).

@rorystephenson
Copy link
Contributor Author

rorystephenson commented May 24, 2022

I just updated the description to clarify that I slowed down the animation to 5 seconds as it makes the jitter more obvious.

However even when it's not slowed down there is the weird affect where markers don't seem to move in sync with the map as you mentioned @ibrierley. I'm guessing the same root cause is responsible for the jitteriness as well as the markers seeming to move ahead and back again.

To confirm I have no issues when dragging.

Also I can confirm that I'm trying this on v0.15.0.

EDIT: I've just noticed that the glitch where markers move out of sync with the map occurs when flinging the map but not when dragging it.

@JaffaKetchup
Copy link
Member

In that case, I'm likely wrong. Oops!

@ibrierley
Copy link
Contributor

I suspect this one will be quite tough to fix, so any inspiration welcome.

If you add another layer in, like a polyline layer, I note that both that and the markers move in sync, but the tiles don't. So it "feels" like the tile layer is lagging behind, however the tile layer is a bit of an unweildly beast these days.

@rorystephenson
Copy link
Contributor Author

I suspect this one will be quite tough to fix, so any inspiration welcome.

Agreed. I'm going to keep digging.

If you add another layer in, like a polyline layer, I note that both that and the markers move in sync, but the tiles don't. So it "feels" like the tile layer is lagging behind, however the tile layer is a bit of an unweildly beast these days.

That's a great point. I think this combined with the fact that the lagging/jittering doesn't seem to happen when dragging are the big clues.

@rorystephenson
Copy link
Contributor Author

I've just been playing around with the tile layer code and I had a suspicion that its unusual way of listening to the movement stream and then calling setState when necessary was causing the lag. I have hacked up a local version which uses StreamBuilder like all of the other layers seem to do and I moved the _handleMove code in to the StreamBuilder and this change has removed the lag 🎉 .

In the following video I am using the usual tile layer and you can see the lag when flinging the map. Then I change to my modified version of AnimatedMapController which is using the hacked up tile layer with a StreamBuilder and there is no longer lag.

fixed-tile-layer.mp4

So the method of listening to the movement stream and then calling setState seems to be the cause of the problem. I can imagine that flutter performs the build caused by the setState call in the next frame (or something similar, I'm not extremely well versed in flutter's building/rendering semantics) which makes the tile layer lag. Even if I set the updateInterval to 0 the issue persists.

I'm not very familiar with the TileLayer code but it seems to listen to the movement stream and only call setState when it deems necessary to avoid loading too many tiles (e.g. if you fling the map fast we wouldn't want to download/load all of the tiles you fling past). Perhaps we need to reconsider this pattern. The first thing that comes to mind is having a delay on the actual downloading/loading of the tile whilst not having that delay on the rebuilding of the displayed tiles. Does anyone else have other ideas on alternatives. Obviously we don't want to impact the performance of the TileLayer with this changes.

@ibrierley
Copy link
Contributor

Great digging! I suspected it was something like that, just hadn't had chance (or didn't want to :D), dig into the tile layer code.

So I guess one question is, are there are any obvious negatives of using the typical Streambuilder method...why is the tile layer using a listen....

I'm a little confused about the fast map fling thingy though (I understand the concept of not wanting to download tiles passed through fast)...if the map is flung fast, and there's a stream listening...wouldn't it still have to reposition tiles, so I'm unsure how the method prevents tile loads....that's me probably not having dug through the code though ? Or is it more that the stream may mean it doesn't hitch so much or something ?

@ibrierley
Copy link
Contributor

Btw one thing we should probably do when testing any workarounds for this one, is test on some old slower devices.

@rorystephenson
Copy link
Contributor Author

It would be amazing to have @maRci002 's input on this since they overhauled the tile layer in #572 and probably understand it quite well.

@rorystephenson
Copy link
Contributor Author

I've spent a lot of time in the last couple of days understanding/refactoring/fixing the TileLayer code and I seem to have solved the lag, my changes are in PR #1247. Let me know what you think @ibrierley.

@ibrierley
Copy link
Contributor

Hey, firstly great stuff! Thought things were quiet on this one, now I know why ;). I'll try and have a better dig at some point over the weekend as there's quite a bit changed, but just wanted to add a couple of thoughts on first test/glance.

I think maybe there's a change in behaviour when there are none available tiles at the correct zoom level (but should be available tiles at a lower zoom that could be scaled). I'm guessing this is maybe around the toRemove() code, but I may be wrong. (although I can see the odd time it is scaled in certain cases)

I.e are tiles now being removed from zoom 1 or 2 say, when we are on zoom 3 (but not loaded yet), as we're only testing for what we should draw now, not also what we could use as a "backup" tile. Best way to show this is on the animated MapController page.

Click on London, in the original it will zoom in and use the old tiles scaled. In the new code, it's grey, and I think the previous behaviour is preferable (or at least as an option).

The Levels stuff is interesting about precalculating, I had wondered if previous stuff was over optimization. I'd be interested if anyone else who knows that code better, knows if there is any specific reason for that, as I can't myself.

What would be an extra bonus (as no one else bothers ;D, but it may be good to have when its fresh in peoples minds...), would be snippets of code documentation in a couple of areas, especially bits you found non-intuitive. I.e for me "what is a level, what is the purpose of updateLevels() type thing", , the word isn't clear and it's always bugged me :). Also what is "wrapping", there's crs wrapping and coord wrapping etc, and it's not always clear on glancing through code.

@rorystephenson
Copy link
Contributor Author

Thanks for already having a look @ibrierley and nice spotting on the fact that the "backup" tiles are disappearing too soon. It's not immediately clear to my why this is the case. Interestingly I noticed that if I run in chrome with a throttled network connection (slow 3g, caching disabled) the behaviour is the same as before, the tile sticks around until the new one is loaded. If, however, I don't throttle the connection I notice what you observed where there is a gray empty tile until the new one loads. 🤔

I agree that further refactoring/commenting would be nice (ideally the code should speak for itself) although I must say I'd be pretty happy to get the code to a full working lag-less state before proceeding with that.

@ibrierley
Copy link
Contributor

Yes, I don't want to hold changed up at all, I only mean short comments if there's any bits that help in the interim, not an overhaul of docs or anything! That can sometimes help others spot problems reviewing, and thinking of any flaws in the future etc, and as mentioned, just a bonus, not core to this change.

The grey tiles issue I saw on Chrome release mode, I haven't had chance to test on a mobile or anything yet. I could see there are occasions in other example pages where none-current zoom scaled tiles are used, but not on the animated controller...(speed of zoom or something if it's a couple of zoom levels out ?)

@ibrierley
Copy link
Contributor

I think I can see that in the new code, abortLoading() gets called, and the old tiles (eg from zoom level 5) seem to have a tile.loaded of false (so they all get removed), whereas in the original code the _tiles all have a tile.loaded of true (after the map is loaded, but you click on London).

I'm a bit confused though, as if I look at the home page, I can see in tile_layer.dart that tile.loaded = DateTime.now(); gets set for tiles. However, when I then navigate to AnimatedMapController, that piece of code never gets called, as the tile is null (called from tile = _tileManager.tileAt(tile.coords);); (guessing as _tiles in the original is populated, but I keep going around in circles at that point :D).

@ibrierley
Copy link
Contributor

I think I have an idea what the problem may be....

Each tile we want, we do a

_tileManager.add(
        coords,
        Tile(
          coords: coords,
          tilePos: _getTilePos(coords),
          current: true,
          imageProvider: options.tileProvider
              .getImage(coords.wrap(_wrapX, _wrapY), options),
          tileReady: _tileReady,
        )..loadTileImage(),
);

Now, _tileReady is used as a callback, and when that runs, it will check ask the TileManage for the tile..

tile = _tileManager.tileAt(tile.coords);

(which uses _tiles behind the scenes).

So first time around, there's a delay, the callback gets called lets set 50ms later, all is fine.

However, what happens when the tile is already kinda previous loaded, so the app has it in cache somewhere or something like that. So I think tileReady may be being called, before the tile has finished being added via the _tileManager.add(), so a sort of race condition, where it then checks, and _tileManage.tileAt doesn't think the tile is in its list, so tile.loaded never gets set, and hence never added as a backup...

Erm I think....I'm not sure of an elegant solution to test that, and running out of time though...

@ibrierley
Copy link
Contributor

ibrierley commented May 27, 2022

Ok, what happens for you if we swap...

_tileManager.add(
        coords,
        Tile(
          coords: coords,
          tilePos: _getTilePos(coords),
          current: true,
          imageProvider: options.tileProvider
              .getImage(coords.wrap(_wrapX, _wrapY), options),
          tileReady: _tileReady,
        )..loadTileImage(),
      );

With

final tile = Tile(
        coords: coords,
        tilePos: _getTilePos(coords),
        current: true,
        imageProvider: options.tileProvider
            .getImage(coords.wrap(_wrapX, _wrapY), options),
        tileReady: _tileReady,
      );

      /// make sure Tile is created before adding, to prevent race condition
      _tileManager.add(coords, tile);

      tile.loadTileImage();

Does that get around the issues you were seeing as well ?

Edit: I'm not entirely sure it is a race condition so maybe we can remove the comment if incorrect, but my brains given up for the day :D.

@rorystephenson rorystephenson changed the title [BUG] Animated panning jitter on flutter web [BUG] TileLayer lag/jitter May 28, 2022
@rorystephenson
Copy link
Contributor Author

@ibrierley Nice digging! That was the cause of the problem. On a throttled connection there was enough time for the tile to be added before the loading finished but on a fast connection or with the tile images cached the loaded value never got set because the Tile was not in the TileManager yet when the callback fired.

That change has been made now 👍 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue reports broken functionality or another error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants