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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Separate scaling from rotating on WebXR #2619

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2cb9c9
Two Fingers - Separate Rotation/Translation
Jul 27, 2021
d82cb06
Revert auto-fix, rename moveToFloor
Jul 27, 2021
7a63512
Fix never setting scaling to false
Jul 27, 2021
9496a55
Remove unecessary firstRatio, set scaling to true on second finger
Jul 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions packages/model-viewer/src/three-components/ARRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

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%.

Copy link
Author

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😁

Copy link
Collaborator

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.

Copy link
Author

@felipe-chamas felipe-chamas Jul 27, 2021 •
edited
Loading

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!

Copy link

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 ?)

Copy link
Author

@felipe-chamas felipe-chamas Jul 28, 2021 •
edited
Loading

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

Copy link

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 :) ?

Copy link
Author

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

// Angle down (towards bottom of screen) from camera center ray to use for hit
// testing against the floor. This makes placement faster and more intuitive
// assuming the phone is in portrait mode. This seems to be a reasonable
Expand Down Expand Up @@ -107,6 +108,7 @@ export class ARRenderer extends EventDispatcher {
private placementComplete = false;
private isTranslating = false;
private isRotating = false;
private isScaling = false;
private isTwoFingering = false;
private lastDragPosition = new Vector3();
private firstRatio = 0;
Expand Down Expand Up @@ -509,7 +511,7 @@ export class ARRenderer extends EventDispatcher {
null;
}

public moveToFloor(frame: XRFrame) {
public moveToPlane(frame: XRFrame) {
const hitSource = this.initialHitSource;
if (hitSource == null) {
return;
Expand Down Expand Up @@ -569,19 +571,18 @@ export class ARRenderer extends EventDispatcher {
} else if (fingers.length === 2) {
box.show = true;
this.isTwoFingering = true;
const {separation} = this.fingerPolar(fingers);
this.firstRatio = separation / scene.scale.x;
Comment on lines -572 to -573
Copy link
Author

@felipe-chamas felipe-chamas Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@lucadalli

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 on master 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.

}
};

private onSelectEnd = () => {
this.isTranslating = false;
this.isRotating = false;
this.isScaling = false;
this.isTwoFingering = false;
this.inputSource = null;
this.goalPosition.y +=
this.placementBox!.offsetHeight * this.presentedScene!.scale.x;
this.placementBox!.show = false
this.placementBox!.show = false;
};

private fingerPolar(fingers: XRTransientInputHitTestResult[]):
Expand Down Expand Up @@ -625,10 +626,17 @@ export class ARRenderer extends EventDispatcher {
this.isTwoFingering = false;
} else {
const {separation, deltaYaw} = this.fingerPolar(fingers);
if (this.placeOnWall === false) {
if (Math.abs(deltaYaw) > ROTATION_THRESHOLD) {
this.isScaling = false;
this.isRotating = true;
} else if (!this.isScaling) {
this.isScaling = true;
this.firstRatio = separation / scene.scale.x;
}
if (this.placeOnWall === false && this.isRotating) {
this.goalYaw += deltaYaw;
}
if (scene.canScale) {
if (scene.canScale && this.isScaling) {
const scale = separation / this.firstRatio;
this.goalScale =
(scale < SCALE_SNAP_HIGH && scale > SCALE_SNAP_LOW) ? 1 : scale;
Expand All @@ -640,6 +648,7 @@ export class ARRenderer extends EventDispatcher {
// to scaling instead.
this.isTranslating = false;
this.isRotating = false;
this.isScaling = true;
this.isTwoFingering = true;
const {separation} = this.fingerPolar(fingers);
this.firstRatio = separation / scale;
Expand Down Expand Up @@ -756,7 +765,7 @@ export class ARRenderer extends EventDispatcher {
this.updateView(view);

if (isFirstView) {
this.moveToFloor(frame);
this.moveToPlane(frame);

this.processInput(frame);

Expand Down