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

On compass press, tilt map if not tilted, when not tracking location #1725

Merged
merged 2 commits into from
Feb 12, 2020

Conversation

smichel17
Copy link
Member

This implementation does not currently work because even when the tilt has been set to 0f, it usually reports being very slightly tilted (eg, tilt=5.317779E-7) and thus, isFlat is almost always false.

If you spam click the compass button, it will occasionally report tilt=0f and work properly. I'm not sure exactly why this is.

@smichel17 smichel17 changed the title WIP: Tilt map if not tilted, when not tracking location WIP: On compass press, tilt map if not tilted, when not tracking location Feb 7, 2020
@smichel17
Copy link
Member Author

title is terrible but the context is in #1724 so you know what I'm going for.

@westnordost
Copy link
Member

You are quite hands-on. Okay, let's try out how users will accept this feature. I'll merge it once the problem with the "almost 0f" is resolved. :-)

@smichel17
Copy link
Member Author

smichel17 commented Feb 7, 2020

You are quite hands-on.

:D

I've done enough Android development to be comfortable source diving a bit. If I needed to do larger changes I'd leave it to you, but for a change like this, that doesn't require me to look at many files, it's less than an hour of work… that's reasonable.

almost 0f

Pushed a commit to work around it, most of the time, just by ignoring very small values. Although it does not fix the bug (my best guess is that it's a tangram-es bug, with a missing callback on their animator), I do not consider this a hack, because at rotation/tilt values that small it would look to the user like the map is flat.

There is still one issue: sometimes the rotation gets "stuck" and incorrectly reports a high value (eg, 6.0), even though the map is north-aligned. It does not appear to happen to the tilt value. I am not sure exactly what is causing it, but I can reliably reproduce it by scrolling the map while location is locked on (thereby breaking the lock). Repeatedly tapping the compass will get it back to normal after 4-5 taps.

@westnordost
Copy link
Member

The rotation and tilt is in radians, if I remember correctly. So 6.28... would again be north I guess.

@westnordost
Copy link
Member

I am not sure exactly what is causing it, but I can reliably reproduce it by scrolling the map while location is locked on (thereby breaking the lock)

I can't reproduce it or I do not understand the repro.

@smichel17
Copy link
Member Author

Your hint about radians was good. I think it getting stuck at ~6.28 was also due to the "almost 0f" bug — it did not quite make it all the way around to 2pi. I added a check for being close to 2pi, also, and now it seems to work pretty well. Do you want me to squash the commits?

@smichel17
Copy link
Member Author

I can't reproduce it or I do not understand the repro.

It's kind of irrelevant now that it's fixed, but the steps were:

  • Lock on
  • Switch to compass mode
  • Scroll the map away
  • Tap the compass button to bring the view back to north
  • Try tapping the compass button again ⇒ view does not tilt

@smichel17
Copy link
Member Author

smichel17 commented Feb 8, 2020

Ah! I dove a bit deeper and the animator is in SC, not tangram. I think I'm going to let you fix this one. The fix is super easy but understanding all the related code and figuring out the right spot for the fix will take longer than I want to spend on this.

Here's the deal: Over in CameraManager.animateCameraUpdate, you've got an ObjectAnimator. You add an update listener to push the new coordinates to tangram. The problem is, ObjectAnimator does not guarantee that the final onAnimationUpdate call will have the exact final value you passed to it. That is, it often over- or under-shoots by just a little bit. So, you need to push the final values to tangram inside the onEnd callback, that currently only unassigns the animator.

(This isn't documented anywhere that I know of, I had to discover through way-too-much-experience struggling with ValueAnimators)

@smichel17
Copy link
Member Author

Never mind, this was not actually hard to fix.

@smichel17 smichel17 force-pushed the compass-tilt branch 2 times, most recently from be1d9de to 6437bea Compare February 8, 2020 15:09
@smichel17
Copy link
Member Author

Okay, rebased so history is nice. I also used kotlin's run function to make it less verbose; I can revert that or split it into a separate commit if you don't like that style.

@smichel17 smichel17 force-pushed the compass-tilt branch 2 times, most recently from f7d52cb to b7302ac Compare February 8, 2020 16:42
@smichel17
Copy link
Member Author

Additional newly-discovered benefit of this: Makes it easier to use 3d with touchscreen gloves, which work fine for taps but aren't great for gestures.

@smichel17
Copy link
Member Author

smichel17 commented Feb 12, 2020

Rebased; I think this is ready to merge, assuming you also want the second commit.

@smichel17 smichel17 changed the title WIP: On compass press, tilt map if not tilted, when not tracking location On compass press, tilt map if not tilted, when not tracking location Feb 12, 2020
@westnordost
Copy link
Member

Looks great! Sorry for the long forth and back for such a small change

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