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

USDZExporter: Make including anchoring properties optional. #28363

Merged
merged 1 commit into from
May 15, 2024
Merged

USDZExporter: Make including anchoring properties optional. #28363

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 additions & 4 deletions examples/jsm/exporters/USDZExporter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class USDZExporter {
anchoring: { type: 'plane' },
planeAnchoring: { alignment: 'horizontal' }
},
includeAnchoringProperties: true,
Copy link
Owner

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?

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

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good 👍

quickLookCompatible: false,
maxTextureSize: 1024,
}, options );
Expand Down Expand Up @@ -198,6 +199,10 @@ function buildHeader() {

function buildSceneStart( options ) {

const alignment = options.includeAnchoringProperties === true ? `
token preliminary:anchoring:type = "${options.ar.anchoring.type}"
token preliminary:planeAnchoring:alignment = "${options.ar.planeAnchoring.alignment}"
` : '';
Copy link
Collaborator

@Mugen87 Mugen87 May 14, 2024 •
edited
Loading

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?

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

includeAnchoringProperties sounds good.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

return `def Xform "Root"
{
def Scope "Scenes" (
Expand All @@ -211,10 +216,7 @@ function buildSceneStart( options ) {
}
sceneName = "Scene"
)
{
token preliminary:anchoring:type = "${options.ar.anchoring.type}"
token preliminary:planeAnchoring:alignment = "${options.ar.planeAnchoring.alignment}"

{${alignment}
`;

}
Expand Down