use anchor point during direct manipulation
ClosedPublic

Authored by rcameron on Oct 3 2016, 7:00 PM.

Diff Detail

Repository
rMDMDIRECTMANIPULATIONSWIFT material-motion/direct-manipulation-swift
Branch
anchor (branched from develop)
Lint
Lint OK
Unit
Unit Tests OK
rcameron retitled this revision from to use anchor point during direct manipulation.Oct 3 2016, 7:00 PM
rcameron updated this object.
rcameron edited the test plan for this revision. (Show Details)
Restricted Application added a project: Material Motion. · View Herald Transcript

In the absence of tests or examples can you provide the testing you did for this diff in the description?

Podfile.lock
12

Looks like you might be running an older version of psych. You can upgrade to 2.1.0 by running the following:

# View the version
gem list | grep psych

# Update psych
xcode-select --install # May need to run this first
sudo gem update psych
src/DirectManipulationMotionFamily.swift
340

I can imagine two ways to implement this method:

  1. Always adjust the anchor point each time a gesture recognizer begins.
  2. Only adjust the anchor point the first time a gesture recognizer begins.

What were your considerations for going with 1.?

340

The plan name should be called ChangeAnchorPoint following the verb- prefix convention.

https://material-motion.gitbooks.io/material-motion-starmap/content/specifications/motion_family/direct_manipulation.html

featherless requested changes to this revision.Oct 4 2016, 10:35 AM
featherless added a reviewer: featherless.
This revision now requires changes to proceed.Oct 4 2016, 10:35 AM
rcameron updated this revision to Diff 6961.Oct 4 2016, 2:34 PM
  • rename plan to ChangeAnchorPoint
rcameron marked an inline comment as done.Oct 4 2016, 2:37 PM
featherless added a comment.EditedOct 4 2016, 3:42 PM

Looks good. I see that the anchor point is only adjusted when using the DirectlyManipulable plan.

Should Draggable/Rotatable/Pinchable emit anchor point manipulations as well?

Thinking critically about that, a view that is rotatable but not scalable won't necessarily benefit from having its anchor point modified. I'd previously assumed that you'd always want to rotate a view around the centroid of the gesture, but perhaps we only want to manipulate the anchor point when a view is completely manipulable as you've done here?

What are your thoughts?

Having the shouldAdjustAnchorPointOnGestureStart properties on the individual plans makes me lean towards having the respective performers emit ChangeAnchorPoint plans themselves. It seems like we're not fulfilling the contract.

Possibly, DirectlyManipulable could no longer be responsible for modifying the anchor point, which seems like a win for composition.

Should we be resetting the anchorPoint to (0.5, 0.5) when manipulation ends to minimize unexpected behavior in clients? I'm imagining a situation where a client is animating a view into place using center at the end of manipulation.

Having the shouldAdjustAnchorPointOnGestureStart properties on the individual plans makes me lean towards having the respective performers emit ChangeAnchorPoint plans themselves. It seems like we're not fulfilling the contract.

Possibly, DirectlyManipulable could no longer be responsible for modifying the anchor point, which seems like a win for composition.

This seems like a solid approach. My vote is to emit the anchor point manipulation plan in each of the draggable/pinchable/rotatable performers.

Should we be resetting the anchorPoint to (0.5, 0.5) when manipulation ends to minimize unexpected behavior in clients? I'm imagining a situation where a client is animating a view into place using center at the end of manipulation.

Good question. Let's say "yes" for now and see if/when this ends up biting us in the future.

featherless requested changes to this revision.Oct 5 2016, 1:55 PM

Requested changes:

  • Make Draggable/Pinchable/Rotatable performers emit the anchor point manipulation plan.
  • Reset the anchor point to its initial value when gestures have completed.
This revision now requires changes to proceed.Oct 5 2016, 1:55 PM
rcameron updated this revision to Diff 7018.Oct 6 2016, 5:08 PM
  • Move AnchorPointPlan emitting to individual Performers
featherless accepted this revision.Oct 7 2016, 1:18 PM

Epic! How's it feel in testing? Can you upload a video?

This revision is now accepted and ready to land.Oct 7 2016, 1:18 PM
rcameron closed this revision.Oct 11 2016, 12:51 PM