-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
USDZExporter: Make including anchoring properties optional. #28363
Conversation
const alignment = options.ar !== undefined ? ` | ||
token preliminary:anchoring:type = "${options.ar.anchoring.type}" | ||
token preliminary:planeAnchoring:alignment = "${options.ar.planeAnchoring.alignment}" | ||
` : ''; |
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.
There is already the quickLookCompatible
property. Could it also be an option to do this:
const alignment = options.quickLookCompatible === false ? `
token preliminary:anchoring:type = "${options.ar.anchoring.type}"
token preliminary:planeAnchoring:alignment = "${options.ar.planeAnchoring.alignment}"
` : '';
If there are use cases where you want to use the anchoring
and planeAnchoring
properties, maybe a new boolean is easier to understand than ar: undefined
. Something like quickLookAutoAnchoring
with false
as the default value?
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 don't think we can use the existing quickLookCompatible here. You may want quickLookCompatible=true for the material fix but still want the anchoring to be floor (alignment horizontal) only.
Another flag quickLookAutoAnchoring may be better indeed, but that one is not a good name imo, it's not really specific to QuickLook here, as far as I know those properties are read for Reality Composer, and probably other softwares that can load usdz. "Auto" in the name is confusing, not sure what that does. I propose includeAnchoringProperties
as a better name. What do you think?
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.
includeAnchoringProperties
sounds good.
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.
Ok, I'll make the changes tomorrow. With includeAnchoringProperties: true
as the default in options.
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.
Ok that's done. I also retested my model-viewer build with it.
…tions to anchor on wall or floor
c1e0897
to
c001513
Compare
@Methuselah96 you seems to be the guy doing the latest updates on |
@vincentfretin You can create a PR at https://github.com/three-types/three-ts-types. That's where the maintenance of |
@@ -25,6 +25,7 @@ class USDZExporter { | |||
anchoring: { type: 'plane' }, | |||
planeAnchoring: { alignment: 'horizontal' } | |||
}, | |||
includeAnchoringProperties: true, |
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.
Should we set this to false
by default?
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 think for most models, you want it to be anchored to the floor. Having a model snap to the wall may be confusing if it was not intended, so having it true by default make sense to me. Having the model snap to the wall should be explicitly stated in my opinion. For model-viewer tag, default is ar-placement="floor", and you can change it to ar-placement="wall" (working currently for Android WebXR). For iOS using a-placement="wall" will set includeAnchoringProperties=false in a future PR I'll do once r165 is released.
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.
Sounds good 👍
Description
Quicklook actually supports anchoring a model on wall or floor (placing a fridge for example) when you don't include the anchoring and planeAnchoring properties in the generated usdz.
There was no way to do that with the options, you could only chose between horizontal and vertical. I propose here to be able to give
{ar: undefined}
in the options so we don't include those two properties when generating the usdzAlso when those properties are not present, the model is actually not auto rotated by 90 degrees compared to using
alignment: 'vertical'
, see discussion in google/model-viewer#3989 (comment)I have a branch of model-viewer 3.5.0 based on three r163 where I tested this change https://github.com/vincentfretin/model-viewer/tree/ios-placement-wall