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

Viewport.Rotation could get to 360 ° or more #2615

Open
charlenni opened this issue May 2, 2024 · 9 comments
Open

Viewport.Rotation could get to 360 ° or more #2615

charlenni opened this issue May 2, 2024 · 9 comments

Comments

@charlenni
Copy link
Member

Mapsui Version
Mapsui 5.x

Mapsui Platform
All platforms

Device
Windows, but the problem is in common code

Describe the bug
If you change the Rotation via Navigator.RotateTo(), then the degrees for rotation could get 360 ° and larger. There is no check for this case.

To Reproduce
Use the "Animated Viewport - Rotate" sample and press the button in the left upper corner for 8 times or more and check the Rotation of Viewport.

Expected behavior
Rotation is in the range of 0 <= Rotation < 360.

Screenshots
grafik

@pauldendulk
Copy link
Member

Thanks for the report.

This was done on purpose. The reason is that this makes the logic simpler for animations and other gradual changes over time. You can just work with difference calculations and rotation distance. If you decide to normalize the rotation between 0 and 360 all those calculations need to take a possible sudden change into account. We had such logic before, and checks in several places and there were bugs in that code.

Of course there should be no error logged. So, at least that is a bug. Are you aware of any other situations where this could cause problems?

@charlenni
Copy link
Member Author

Ok

Logging an error was only the fastest method to display the Rotation value.

@charlenni
Copy link
Member Author

@charlenni charlenni reopened this May 2, 2024
@pauldendulk
Copy link
Member

Thanks for the movie. It looks indeed weird and it made me re-evaluate the current logic, however, I think the current solution is the best one. Considerations:

  • When pressing 'Click to rotate to zero' the rotation is at that point 3x360. So the developer is at that point requesting an animation from 3x360 to 0. So, this is what you get. It can be explained.
  • We could assume that we know what the user (talking about the developer-user, not the app-user) wants and fix it for them. If we keep everything between 0 and 360 this won't happen. However, if we do this it will not be possible at all to do any rotation bigger than 180 and sometimes this might be what you want. And with the current logic it is still possible to rotate directly back to 0 by doing a rotation %= 360 in that button. For the app-user it will behave as expected. So, now everything is possible, and if we try to fix it for the user we block other options.
  • I can imagine a user wanting to do a full rotation or more in a geo game like app, rotating and zooming in to a specific location.
  • Fixing this for the user might be handy in this situation but could be hard to understand in other situations. For instance what should happen if we rotate from 355 to 5? I guess we will have to take the shortest route, but it not super obvious. And with the current solution is it very straight forward. It is just like an other double value. Going from 355 to 5 is a simple linear change of a double from 355 to 5, just like any other of the double we animate.

@charlenni
Copy link
Member Author

I can't follow your argumentation. It isn't intuitive to say 'Go in direction of 875°'. That's nothing someone expects. I can't find this value on my compass. So why is it important for 99.9 % of the users to have rotations bigger than 360 degrees? Where is the benefit for all them to check all over their app, if the app user rotated the map two or three times (what could happen on a mobile device) and then press the 'Go to north' button? Why isn't it possible for the rest 0.01 % to divide their rotation in 9 rotations by 90 degrees and one by 65 degrees (example from above with 875 degrees)? They get the same result as expected.

What most of the users expect when calling an animation from 355 to 5 is, that it takes the shortest way. If you want to have a different route, there are three possible ways:

  1. Add a direction to the RotateTo() function with possible values of shortest, CW and CCW
  2. Add a function for rotating relative, with positive and negative values for the angle
  3. Rotate in steps from 355 to 265, to 175, to 85, to 5.

And if this is the best solution, why don't use any of the other maps libraries this? They call it often 'heading' and a heading shouldn't be over 360 degrees. So it will be possible to divide both in Mapsui. Your heading is always between 0 and 360 and gives the actual orientation of the map, but a rotation could be greater than 360 or small than 0. With this, it is clear what the user wants.

@peterblazejewicz
Copy link

I assume folks have been using maps api, say Google Maps, heading (rotation) seems to be capped to 0-360 range. CW/CCW are derived outcome of using heading, can be extension methods for better devex.
I won't be able to get spinning animation when using Google Maps Api

@pauldendulk
Copy link
Member

@peterblazejewicz The google api may still use values outside of the 0-360 range at a lower level. Or perhaps they which they did, because when I was working on the rotation animation bugs things got a lot simpler when I did not try normalize it. Also it allows all options and you can always normalize at a higher level. It is easy to add GetHeading and SetHeading extension methods on the Viewport that work with normalized values. It could use the RotationSnapper.NormalizeRotation method.

@charlenni
Copy link
Member Author

That is much overhead for 99.9 % of the devs to make it easier for the rest 0.1 %.

@pauldendulk Did you ever had a use case (in real life or while using a digital map) to have a rotation bigger than 360°? I couldn't find any. Perhaps that the reason, that no other map library does it in this way.

@pauldendulk
Copy link
Member

This is not an effort to make it easy for the 1% percent of developers. This is an effort to keep the Mapsui code simple and straightforward. There were al least two bugs in the pervious code base because it did not deal correctly with jumps over the 0/360 divide. Some were hard to track because they only showed up in some animation. This is core functionality. If we really need this limited to values between 0 and 360 this could be added in a layer on top of that. It can not be done the other way around.

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

No branches or pull requests

3 participants