fix ios accelerometer input bugs #124
Conversation
|
@Maliac11 and I just spoke offline and we're going to see if we can get a 💪 🔥 ui test to cover this behavior so that we can verify going forward that it's 💯 working. We'll revisit this topic on Friday, and if things aren't progressing with a test that can cover that, we'll re-evaluate (both of us are new to testing in swift). |
bengreenier
changed the title
There was a problem hiding this comment.
looks really great! thanks @Maliac11 🔥 🔥
just a few nitpicky things that could be improved, but i'm satisfied with this code! As discussed, we'll hold off on merging this (hence my non-approval here) until we can get some tests or investigate and decide testing this behavior is overly complicated. 💯
| navPitch = prevNavPitch + dpitch | ||
|
|
||
| if UIDevice.current.orientation == .landscapeLeft { | ||
| print("left") |
There was a problem hiding this comment.
nit: can we remove these prints, seems like the rest of the nearby deviceMotion code doesn't log any info, and if we're confident this works i think we can avoid these statements 🥇
| } else if UIDevice.current.orientation == .landscapeRight { | ||
| navPitch = prevNavPitch + droll | ||
| navHeading = prevNavHeading - dheading | ||
| print("right") |
There was a problem hiding this comment.
nit: can we remove these prints, seems like the rest of the nearby deviceMotion code doesn't log any info, and if we're confident this works i think we can avoid these statements 🥇
| } else { | ||
| navPitch = prevNavPitch + dpitch | ||
| navHeading = prevNavHeading - dheading | ||
| print("else") |
There was a problem hiding this comment.
nit: can we remove these prints, seems like the rest of the nearby deviceMotion code doesn't log any info, and if we're confident this works i think we can avoid these statements 🥇
melmaliacone
force-pushed
the
fixLandscapeMode
branch
from
730baf1
to
952fcbc
Compare
|
@michaelperel @Maliac11 if i understand correctly this also now fixes #119 ? |
bengreenier
changed the title
There was a problem hiding this comment.
per @michaelperel there is a disconnect crash bug present here, and he has a pending fix. blocking merge until the fix goes in
|
per convo with @Maliac11 we're going to merge this without these ui tests at this point, as we believe the cost of authoring them is much greater than the value they'll provide. as such, merging. |
Previously, rotation with the accelerometer did not work in landscape mode.
The heading variable needed to be changed to use the device motion roll rather than the device motion pitch to have to correct behavior. This is because the axis along the y direction changes when the phone is put in landscape mode.
I tested this by running the app on my phone and rotating it around in portrait, left landscape, and right landscape mode. I made sure to switch from one mode to the other and confirm that I still saw the desired behavior.
Fixes #117