p-lr / MapView

A Fast, memory efficient Android library to display tiled maps, with support for markers, paths, and rotation.
Apache License 2.0
184 stars 38 forks source link

Markers rotate with map #6

Closed unbearables closed 4 years ago

unbearables commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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