p-lr / MapCompose

A fast, memory efficient Jetpack Compose library to display tiled maps, with support for markers, paths, and rotation.
Apache License 2.0
220 stars 19 forks source link

Question: Tiles with padding #93

Closed cwsiteplan closed 11 months ago

cwsiteplan commented 1 year ago

Hi,

first of all - thanks for all the work! Really appreciate that.

I'm having map tiles that do have a certain padding (vertical and horizontal) applied to them (the border tiles - this is necessary when map tiles are created from non-uniform sizes so we can get 256x256 tiles). Unfortunately i cannot change the tiling source - so i need to respect (remove) that padding when rendering the tiles/map. Basically shrinking the viewport by the amount of the padding.

image

(note: all tiles still use a 256x256 ratio- this sketch is a bit misleading)

Would there be an easy way of adjusting that without forking the lib? if not - could you point me to the corresponding code so i can implement that (probably creating a PR if this could be helpful to others as well).

cwsiteplan commented 1 year ago

mhm - just saw https://github.com/p-lr/MapCompose/pull/59 maybe that helps already - will investigate

cwsiteplan commented 1 year ago

seems like setVisibleAreaPadding() would help me to zoom/pan properly but still the tiling canvas would include/render the padding and so placing markers would not work properly.

any ideas how i could shrink the tilingCanvas so the padding does not get rendered at all?

cwsiteplan commented 1 year ago

i tried now to add the scaled padding as absoluteOffset to the marker. which seems to work fine on a single render, but upon zooming these offsets seem to cause glitches - see video below

https://github.com/p-lr/MapCompose/assets/141158723/d5c25683-1721-4e70-a097-8b0d87da0d3d

p-lr commented 1 year ago

Hi, I'll be able to look into this in a few days.

p-lr commented 1 year ago

If I understand correctly, you want to constrain the scroll so that only the blue part of the canvas is visible. Do you confirm? Is is possible for you to fork the lib and edit one of the demo with your tile source? It would be useful for me to test necessary changes.

cwsiteplan commented 1 year ago

Yes, but it's not only the "scroll" - the whole canvas of the map should be constrained to that. as placing markers would also be a bit messy if i need to calculate padding offsets.

Will do a fork and update you on that 👍

cwsiteplan commented 1 year ago

i've now added a map source that contains the padded tiles in the Simple demo screen. here's the fork.

note: the padding of the tiles differs on the different zoom levels as we need to fit the Images to the nearest multiple of 256.

thanks for having a look!

p-lr commented 1 year ago

I managed to remove the padding at rendering level, and pushed the changes in my repo (branch feature/tile-padding). Given the tile at highest level, I could compute the padding at scale 1 (which is 685px left and 1760px top). I introduced a setTilePadding api. However, there are major downsides : it requires non trivial changes, and not just at rendering level. For example, the scroll constraints need to take into account the tile padding. It feels too much for this edge case.

If the changes I made are enough for you, that's great. But required changes turns out to deviate too much from the core assumptions of the library.

cwsiteplan commented 1 year ago

Thanks for investigating and providing the change.

The tile padding is known so i can set that using your API 👍

Unfortunately, as you mentioned, the rendering alone is not sufficient. Restricting the panning (scrolling) would be a nice to have but not mandatory. More important is the placing of the markers fullWidth/fullHeight still contain the padding - so placing markers is off (i placed a blue circle at 0,0)

✅ I took that into account in the MarkerLayout - see change here

I know handling tile padding might be an edge case, but as soon as there is a map tile source with a size that is not a multiple of the tile size - adding a padding is necessary. It would be greatly appreciated if this change could be incorporated. 🙏

p-lr commented 1 year ago

Good job finding a solution. I'm aware that this feature can be useful, but I don't want to incorporate this - at least for now. The reason is: it as to be fully supported. For the rendering part, it already complicated a lot the TileCanvas implementation. Scroll constraints are already quite tricky even without this feature. Keeping the code base in a sane state is important. Most often, it's way simpler to adapt the tile source (when possible).

Another approach would be to transform the tile source on the fly (making bitmap stitching), so that all of this become transparent for the library.