-
Notifications
You must be signed in to change notification settings - Fork 822
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
Separate scaling from rotating on WebXR #2619
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
@@ -33,6 +33,7 @@ const INIT_FRAMES = 30; | |||
// estimation, which will be used once available in WebXR. | |||
const AR_SHADOW_INTENSITY = 0.3; | |||
const ROTATION_RATE = 1.5; | |||
const ROTATION_THRESHOLD = 0.05; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Can you describe why you think this UX is an improvement? I was following more the Google Maps style where both actions happen together. There is already a 30% snap on scaling to make it sticky near 100%. Also I'd always recommend setting ar-scale: "fixed"
to disable scaling entirely for products that are meant to be seen at 100%.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do use ar-scale: "fixed"
whenever we need the product scale precision!
I'm checking a lot on the WebXR code to make our migration and when I read the comment of actions being independent it felt awkward trying to rotate a model without scaling, mostly because it's hard to maintain a constant distance while rotating two fingers. I've also realized I personally use fingers from both hands in opposite directions to rotate, which always make the model shrink then expand. The same movement has always worked nicely on scene-viewer where movements are not interchangeable during a two-finger interaction.
On the other hand, a little bit of rotation while scaling feels acceptable so it's mainly for the reasons above.
Maybe I'm being picky on something that's actually a good feature for most users, since there's already a precise 1 finger rotation, so I don't mind leaving this PR for some extra research😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! I'll leave this for some further discussion for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this for some further discussion for now.
Awesome, thanks for opening it in the thread!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I do agree that slight rotation while scaling is not an issue, but accidentally scaling while trying to rotate is not desired. So if I understood correctly it should be definitely an improvement :) (when rotating beyond threshold scaling is disabled, right ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when rotating beyond threshold scaling is disabled, right ?
That's it! I can make an online demo if it may help on deciding between both scenarios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it is clear now and I support the change :) But maybe for the others :) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Online simplified demo for testing
@lucadalli I'd be interested to get your opinion on this. It only affects WebXR mode, so just Android for now, so at least you can test it. |
I don't use the WebXR viewer, nor have I have ever worked with WebXR input events, so I'm assuming the same capabilities as touch events. I can agree with the sentiment to make rotation and scaling distinct. In fact I would be inclined to have 1 finger for rotation and 2 fingers exclusively for scaling, similarly to This brings me to the changes proposed in this PR. They make rotation and scaling somewhat distinct, without compromising the ability to use 2 fingers within the placement box to rotate the model. I do consider it an improved UX. To allow for scale adjustments without impacting rotation we can make the interactions which start outside the placement box mutually exclusive. Similarly, 2 fingers inside the placement box both rotate and scale, while 2 fingers outside would only scale. On top of all this, I think we should also introduce the ability to alternate between the 1 finger interaction and the 2 finger interaction without having to reset by lifting all fingers, like we just did with This leaves us with 4 possible behaviors. Behaviors in the same column can be alternated between by adding/removing the second finger. (For simplicity's sake, inside or outside of the placement box would be determined by the first touch. It is not recalculated until all fingers are lifted and a new interaction is started.)
I am not strongly against or in favor of this PR, nor do I feel strongly about the alternative I suggested. I'm sharing my thoughts to further the discussion. |
I'm not sure if this is some intended behavior which is being erratic but it seems like a bug to me. |
const {separation} = this.fingerPolar(fingers); | ||
this.firstRatio = separation / scene.scale.x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is some intended behavior which is being erratic but it seems like a bug to me.
When testing the demo linked above by @FelipeR2U, sometimes I am able to double tap box corners which are diagonally opposite and make the model do a 180. I am able to reproduce onmaster
but it's far less common.
Well noticed, I had the same issue sometimes and realized I should probably rollback this change so that this.lastAngle
is updated.
@lucadalli - About your first Comment |
Sorry I forgot about this one! I'm in the midst of switching to pointer events, and still open to this once that lands. |
Sorry, I was organizing my GH account and closed some of my PRs, I'm gonna reopen it for further discussion |
Reference Issue
With two finger interaction, scaling and rotating are not really mutually exclusive.
Description
I've created a threshold to detect if it's a real rotation or a small movement when pulling fingers apart.
ROTATION_THRESHOLD
was chosen based on some experimentation locally, but I would like to open this value for discussion.If the threshold is crossed the rotation is active, if not then in the first frame we set the new
firstRatio
value for scaling.@elalish , I've also changed the function name I pinpointed yesterday 😅