custom symbols #79
custom symbols #79
Conversation
…er. We need it to lead the images.
…ing the image takes scaling into account.
There was a problem hiding this comment.
This is great start @TimothySealy, happy to work on the Android equivalent for this!
|
I just implemented it on Android (mattreid1@0cbcc0d) but it's a little thrown together. (I'm also not too sure how to merge it into this pull request) I noticed that in @TimothySealy's iOS implementation, you used hardcoded in I had to add an additional boolean parameter, |
|
Hi @mattreid1, thanks for your contribution. I can pull your changes in to this branch adding your work to this pull request. Regarding the hardcoded path, it was a basic first implementation and can be removed in future versions. The idea was to add scale dependant images. Based on a device's screen capabilities the application would load the 2.0x or 3.0x image. These images have different resolutions and reside in different folders. Loading the right image means creating the correct path and loading the image from there. Have a look at this commit (TimothySealy@031993f) to see how this has been implemented in iOS. Using a hardcoded path simply makes this easier. Of course we could always provide the full path in the iconImage property (in dart) and do some string parsing in order to create the proper path. In order for this to work there would have to be some pattern in order to differentiate between custom and default icons. Currently iOS only supports PNG (or PDF but I have not gotten this to work) for assets. A pattern could be iconImage ending with .png or iconImage starting with 'assets'. In my opinion this would be a better approach instead of passing a boolean flag. What are your thoughts on this? |
|
I've formatted my code a little better in my most recent commit to my custom_symbol branch. The scale point makes sense. Maybe it could automatically look for folders named I certainly agree that removing the parameter would be best. I made it optional to maintain backwards compatibility, though. I just wanted to ensure there would be no conflicts between the default icon names and a file path. Maybe a good approach would be to check if the string provided points to an accessible file? I don't think that should cause any issues. I'll try implementing and testing these later tonight/tomorrow to see how it works out. |
|
I removed the parameter in this commit mattreid1@3e1cb0e and added scaling in this commit mattreid1@acd6641. Scaling just gets the screen density and counts down in a for loop to try to find the biggest sized image directory. If it gets down to It should be fairly easy to implement in Swift too if you wanted. |
|
@tobrun I've update the PR. I've merged the changes of @mattreid1 for the implementation on Android. Because we are using the legacy API on iOS you can only remove a Symbol or change its position. When you have the time could you review this PR? |
Planning to do this weekend. @TimothySealy I have given you collaborator access to this repo. You can now push branches directly without having to fork and you can perform reviews. Thank you for the time your are putting into this project! |
There was a problem hiding this comment.
Awesome work @TimothySealy, I'm 👍 on merging this and revisiting later to address #79 (comment)
| } | ||
|
|
||
| if (bitmap != null) { | ||
| mapboxMap.getStyle().addImage(symbol.getIconImage(), bitmap); |
There was a problem hiding this comment.
style can return null if the underlying style has been fully loaded. This can be worked around by using the async callback version of the method that takes in OnStyleLoaded callback
There was a problem hiding this comment.
Done. Also simplified the code a bit.
There was a problem hiding this comment.
Still think we can improve this by adding images when the OnStyleImageMissingListener fires. This way images can be reused (which is not the case at the moment).
Note: In order to do so we need to use the latest version of the SDKs.
There was a problem hiding this comment.
Implemented the addOnStyleImageMissingListener callback for loading the image.
@tobrun This PR is ready to be merge. Could you review the last changes and merge the PR if you accept the changes?
There was a problem hiding this comment.
Changes look good but can't merge due to: This branch has conflicts that must be resolved. Could you fix that, happy to merge after!
There was a problem hiding this comment.
@tobrun I've merged master to resolved the conflicts.
android/src/main/java/com/mapbox/mapboxgl/MapboxMapController.java
Outdated
Show resolved
Hide resolved
# Conflicts: # ios/Classes/MapboxMapController.swift
Implementation of custom symbols on both iOS and Android.