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

Groundwork for Feature MO-7031 #3

Merged
merged 16 commits into from
Jul 28, 2016
Merged

Conversation

jaredsinclair
Copy link
Contributor

@jaredsinclair jaredsinclair commented Jul 27, 2016

MO-7031 Calls for selectively disabling vertical panning for 360 player views embedded inline in a scroll view. This PR lays down the groundwork for supporting this feature:

  • Breaks out some complex math that was implemented in NYT360CameraController.m into separate NYT360EulerAngleCalculations helper functions, both to isolate this logic and also to expose it more easily to unit tests.
  • Cleans up some inline functions and constants around the calculation code that was previously in the camera controller.
  • Adds an allowedPanningAxes property to NYT360CameraController which allows host applications to suppress x or y (or both) axis input from both device motion and pan gesture changes.
  • Implements the suppression of x/y axis movement in the newly-created NYT360EulerAngleCalculations helper functions.
  • Adds unit tests for verifying that the correct axes are suppressed when calculating new camera position and euler angles.

With these changes in place, it should be possible in a followup PR to adjust the axis option on both NYT360ViewController and on any future inline 360 video player view.

// TODO: [jaredsinclair] Consider adding constants for the multipliers
// TODO: [jaredsinclair] Find out why the y multiplier is 0.4 and not 0.5
position = CGPointMake(position.x + 2 * M_PI * rotateDelta.x / viewSize.width * 0.5,
position.y + 2 * M_PI * rotateDelta.y / viewSize.height * 0.4);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why the y component uses a 0.4 multiplier instead of 0.5.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we were testing the movements this speed was discomforting on the y axis

@cdzombak
Copy link
Contributor

👍 after my comments are addressed, and this is a simple enough PR (in code I'm familiar with) I'm not going to add a second reviewer.

- Makes all schemes shared.
- Adds the unit test target to the module's test phase.
- Includes source files in the unit tests build phase so that it does not depend on the public 360 module.
@jaredsinclair
Copy link
Contributor Author

@cdzombak I've addressed all your comments, though in order to fix NYT360EulerAngleCalculations.h I had to reorganize the test target so it works like, e.g., the LogicTests target in the newsreader project: source files are added directly to the unit test target, rather than the test target importing the module. I'm assuming this change is acceptable.

@cdzombak
Copy link
Contributor

I had to reorganize the test target so it works like, e.g., the LogicTests target in the newsreader project: source files are added directly to the unit test target, rather than the test target importing the module. I'm assuming this change is acceptable.

👍 this works for me. Thanks, @jaredsinclair.

@cdzombak cdzombak merged commit dcdc925 into master Jul 28, 2016
@cdzombak cdzombak deleted the feature/MO-7031-disable-vert branch July 28, 2016 02:30
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

3 participants