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

Markers rotate with map #6

Closed
wants to merge 2 commits into from
Closed

Markers rotate with map #6

wants to merge 2 commits into from

Conversation

unbearables
Copy link

Hey, I needed to rotate markers with the angle of the map, so I added functionality for the marker to stick to specified angle. This works nicely with displaying current position in custom map, in this case the angle of the map is determined by bearing. See updated demo.

Rotation of the marker is done optionally in addMarker, moveMarker (to optimize updating current position) and manually in rotateMarker function.

@p-lr
Copy link
Owner

p-lr commented Feb 26, 2020

Doing this is possible without adding modifications to MapView, but it involves adding ReferentialOwners on client code which are notified by the MapView when the angle changes. It's not that difficult, but I have to admit that for something as basic as fixing an angle for makers, the library could provide an API.
Regarding the modification of the addMarker api, it looks ok. However, the rotation pivot point is implicitly set to the center of the marker. While it fits your use case, it won't fit every use case.
On the other hand, I'm not convinced that adding the same option for moveMarker brings value. A marker can be added with an angle, then moved. It it requires further rotation, it can be done either with a new dedicated API (like the one you added), or by the client code. I'm not saying it's useless - I just care about not cluttering the existing API.
Also, the new angle argument you added to MarkerLayout.moveMarker is a breaking change, since MarkerLayout is a public open class since the 2.0.0 version. Users might have subclassed it.

Overall, you did a great job and it fits your use case. But I need more time to think about it. Everything that touches API requires careful design choices.

@p-lr
Copy link
Owner

p-lr commented Feb 26, 2020

For the moment, I'm thinking about adding an example in the demo, which would do exactly what you've done but using the ReferentialOwner I mentioned.

@unbearables
Copy link
Author

I didn't know it is possible with ReferentialOwner, I thought current state of the library does not allow rotation at all.

Regarding the addMarker pivoting - it is up to developer to change the pivot according to their needs, meaning I didn't change any pivoting configuration (just the demo position marker is set to center).

I was gentle about the API and MarkerLayout changes, and defaulted everything to not rotating. Therefore if someone migrates from previous versions to the version i proposed, absolutely nothing should change, as the rotation is completely optional.

I have added the angle to moveMarker because of better performance than calling moveMarker and right after that rotateMarker, which renders the markers twice instead of once - resulting in more computation. What do you think about splitting it to 2 methods - original moveMarker (without any change) and new moveAndRotateMarker (with angle)?

@p-lr
Copy link
Owner

p-lr commented Feb 26, 2020

I've just pushed a change in the RotatingMapFragment demo, which does exactly what you've done without touching the library. Also, performance wise, it's even better because it doesn't request a redrawn of the whole marker layout.
Can you try it out and tell me your thoughts?
If we step back a little, markers are just views. The user owns the references on those views. So anything like marker rotation is doable using nothing but view.rotation = ... on client code. You just didn't know that you could be notified by angle change. I should have documented that.
I think that this is better not to add those rotation APIs to MapView, because any custom behavior can be implemented using ReferentialOwner interface. Also, from a performance standpoint, it's better.

@unbearables
Copy link
Author

Great, the example code works like a charm. Thanks for the help.

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.

None yet

2 participants