flutter_map icon indicating copy to clipboard operation
flutter_map copied to clipboard

Improve tile handling

Open JosefWN opened this issue 1 year ago • 12 comments

A suggestion to simplify the tile handling a bit:

  • Move AnimatedTile into former TileWidget (no need for state, the tiles are managed by the TileManager), the AnimationBuilder does the heavy lifting for us
  • Do not recreate AnimationController for every animation

Haven't gotten rid of the opacity widget yet, I think we'd need to crossfade between old and new tile on zoom

JosefWN avatar Sep 05 '22 21:09 JosefWN

@JosefWN Hi there, just getting round to looking at all of these. Rather than dealing with our own animation controller and ticker etc., can we just use a TweenAnimationBuilder?

JaffaKetchup avatar Sep 17 '22 14:09 JaffaKetchup

One reason I kept the animation controller is that we allow for this:

animationController?.forward(from: from);

So you can choose to play the animation from, say, 50% to 100%. I don't know why this is necessary, but we have exposed the option to the users as well (TileLayer.tileFadeInStart). I could have another look at if we can further simplify if we were to remove the option?

JosefWN avatar Sep 19 '22 17:09 JosefWN

I see. I guess it depends on how much things can or cannot be simplified, and how often it's used. I'd say if it's not used in the example app, then we can probably get rid of it: although this will be a breaking change. @ibrierley @MooNag @TesteurManiak What do you guys think?

JaffaKetchup avatar Sep 19 '22 18:09 JaffaKetchup

IMO we can replace with a TweenAnimationBuilder the use of the AnimationController seems too specific.

TesteurManiak avatar Sep 19 '22 18:09 TesteurManiak

I'm a bit hesitant to do breaking changes atm (we've had quite a lot of late), but no problems with these type of changes implemented in a future version where there's several breaking changes if that makes sense.

ibrierley avatar Sep 19 '22 19:09 ibrierley

Yeah, I would agree with that. I think leaving an at least 2-3 month gap between breaking releases is the minimum.

In this case, @JosefWN can you explore how much you can simplify things with this option - and any similar options you think fall under the same bracket - and maybe we merge this into a separate branch 'delayed-breaking-changes', which we can merge into main and release at a later date. Of course, if you don't have time, let us know and we'll just merge this for now.

Thanks :)

JaffaKetchup avatar Sep 19 '22 19:09 JaffaKetchup

I can have a look in the coming days!

JosefWN avatar Sep 19 '22 19:09 JosefWN

I know the tile layer stuff is all kinda spaghetti together and hard to keep separate, but we could look at making this the migration to a new tile layer system. Give it a new name, RasterTileLayer, WMSTileLayer, VectorTileLayer, etc.... And then deprecate the old stuff. This would allow for non-breaking changes and allow us to basically rewrite the entire tiling system. I don't think this change has to be breaking for end users.

One thing I would like to see be worked on, I don't have the time right now, but support wrapping on the map left and right; and an extension of that would be to extend the system to support rendering on a sphere.

mootw avatar Sep 19 '22 19:09 mootw

I'm not sure, if you deprecate something in 1.2, and replace it in 1.3, is that OK with SemVer? Or would you need to move it 2.0? Are we even sticking with SemVer (we are atm, and it seems kind of sensible to remain doing so)?

But yes, I would like to see that rewrite and separation as well. This might even pave the way for #1337. But alas, I don't have the time (or much knowledge) to do so either at the moment. I wonder how @greensopinion would feel about helping out, do you have time? You seem to be pretty knowledgeable about this stuff - and the naming/separation scheme could benefit your plugin, if you see what I mean.

Should we be making this aim more formal? Ie. use a Discussion, Discord channel, and/or Git branch.


support wrapping on the map left and right

See #1338, #1201

extend the system to support rendering on a sphere

I think someone had a crack at 3D rendering, but I think they ran out of time.

JaffaKetchup avatar Sep 19 '22 20:09 JaffaKetchup

I wonder how @greensopinion would feel about helping out, do you have time?

Hey, thanks for the ping. I'm pretty busy with other things at the moment, unfortunately I don't have any spare cycles to chip in.

I'm a bit hesitant to do breaking changes atm

Improvements are great, but any breaking change is a pain for users and for plug-in maintainers. Unless there's substantial benefit, we should try to avoid it as much as possible. I know that might sound like a drag, but it's a big deal. For example, going to 3.0.0 was a lot of work. For the VectorTileLayer I had to understand the nature of the change and then get it working. It cost me about a full day, and although the code is a little cleaner on the consumer side, it's an incremental benefit. I'm not sure that it's worth it to break the API for such a minor improvement.

I'm not sure what's planned for the new tile layer system. There is quite a lot of logic in vector_map_tiles related to loading tiles of different sizes, depending on the data that's available and to optimize the frame rate. It would be great if we could avoid breaking that.

greensopinion avatar Sep 20 '22 02:09 greensopinion

Thanks anyway @greensopinion :)

Yeah, understood with the breaking changes. For me, I don't mind them so much, but I can see & understand they are a pain for a lot of people, especially when the changes are as big as last time. As I mentioned above, this PR will probably go into a delayed breaking changes branch, until we have accumulated them over maybe 2-3 months. This change should break much anyway - I can't see why anyone would be using it.

Those greater tile system changes probably won't be coming any time soon, so no need to worry about them for now.

JaffaKetchup avatar Sep 20 '22 06:09 JaffaKetchup

@JosefWN I've converted to a draft just for now, so we can differentiate between needs-review/in-progress/ready. If you are ready to merge, let me know.

JaffaKetchup avatar Sep 21 '22 19:09 JaffaKetchup

Hi @JosefWN, is this ready for review? If not, no pressure!

JaffaKetchup avatar Oct 10 '22 20:10 JaffaKetchup

I propose we just work on new tile layer stuff from the ground up, give it a new widget (ideally plural with different behaviors) with new logic. That is realistically the best way to improve the tile handling at this point. The current system is not extensible, has too many edge cases, and way too many options.

mootw avatar Oct 10 '22 22:10 mootw

@MooNag I agree with this, however I'm not sure when this would get finished?

JaffaKetchup avatar Oct 11 '22 06:10 JaffaKetchup