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

Add updateImageSource. #271

Merged
merged 10 commits into from
Sep 15, 2023
Merged

Add updateImageSource. #271

merged 10 commits into from
Sep 15, 2023

Conversation

CaviarChen
Copy link
Contributor

@CaviarChen CaviarChen commented Jul 16, 2023

This PR basically backports an existing function from https://github.com/flutter-mapbox-gl/maps.
Unlike that one, this does not suffer from mapbox/mapbox-maps-android#302 and it works nicely on both iOS and Android. The web version is still unimplemented, similar to existing image source related functions.
(Sorry a thing I'm building only requires updateImageSource on both iOS and Android.)

@m0nac0
Copy link
Collaborator

m0nac0 commented Jul 22, 2023

@CaviarChen Thanks for the contribution! I haven't had time to test this, yet, but the code looks good 👍

@CaviarChen
Copy link
Contributor Author

merged from the upstream and tested, everything works fine.

@CaviarChen
Copy link
Contributor Author

A gentle ping on this.

@m0nac0
Copy link
Collaborator

m0nac0 commented Aug 14, 2023

Sorry, I haven't had time to actually test this. I left a comment about the documentation.
And could you also add an example for the new method? That makes testing easier. Could e.g. be just one more button in the existing Place Source page in the example app.

@CaviarChen
Copy link
Contributor Author

Changed as requested. However, I do spot an issue and I don't think I know this lib enough to know where went wrong. It seems that the Change image button works fine on iOS but on Android, I need to touch (move) the map to trigger a refresh to make the image change.

@CaviarChen
Copy link
Contributor Author

Played with the example a bit and I think the bug is not introduced by my PR.
Reproduce:
open the example app and go to the place source page,
click add source
then click show layer
you can see the image show up as expected
then click hide layer
the image does not go away until you do something like touch the map (which trigger a redraw?)

@CaviarChen
Copy link
Contributor Author

ping

@m0nac0
Copy link
Collaborator

m0nac0 commented Sep 14, 2023

Thank you for your patience!
LGTM! I just noticed that the images appear flipped in the iOS simulator, but that doesn't seem to be related to your work.

Just one more question: is the new asset file your own picture or licensed in a way that's compatible with the repo license?

@CaviarChen
Copy link
Contributor Author

Ah that's a good question, I might just found it from google. I have no experence in this, do you have any recommanded way of finding pic for this?

@m0nac0
Copy link
Collaborator

m0nac0 commented Sep 14, 2023

I guess you could search on wikimedia commons and use e.g. a CC0 licensed image

e.g. https://commons.wikimedia.org/w/index.php?search=sydney&title=Special:MediaSearch&go=Go&type=image&haslicense=unrestricted

@CaviarChen
Copy link
Contributor Author

Replaced the image, the new one is found here: https://commons.wikimedia.org/wiki/File:Sydney_Opera_House_(2017).jpg

@m0nac0 m0nac0 enabled auto-merge (squash) September 15, 2023 17:03
@m0nac0
Copy link
Collaborator

m0nac0 commented Sep 15, 2023

Thank you again for your contribution and patience!

@m0nac0 m0nac0 merged commit 5d49239 into maplibre:main Sep 15, 2023
6 checks passed
CaviarChen added a commit to MemoLanes/MemoLanes that referenced this pull request Sep 30, 2023
This PR includes:
- Use the proper upstream for fultter-maplibre-gl, our image source PR
is merged: maplibre/flutter-maplibre-gl#271
- Only trigger a rerender when needed.

However, there are bugs when antimeridian is in the view, and it require
some none trivial changes in the upstream lib:
maplibre/maplibre-native#1681
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants