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.

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

Conversation

vincentfretin
Copy link
Contributor

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 usdz
Also 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

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

@Mugen87 Mugen87 May 14, 2024

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.

@vincentfretin vincentfretin force-pushed the usdzexporter-wall-and-floor branch from c1e0897 to c001513 Compare May 15, 2024 09:10
@Mugen87 Mugen87 added this to the r165 milestone May 15, 2024
@Mugen87 Mugen87 merged commit ce4536a into mrdoob:dev May 15, 2024
11 checks passed
@Mugen87 Mugen87 changed the title USDZExporter: Allow to place on wall or floor USDZExporter: Make including anchoring properties optional. May 15, 2024
@vincentfretin vincentfretin deleted the usdzexporter-wall-and-floor branch May 15, 2024 09:24
@vincentfretin
Copy link
Contributor Author

@Methuselah96 you seems to be the guy doing the latest updates on @types/three.
I did the changes to USDZExporter.d.ts for this PR and #28049 in vincentfretin/DefinitelyTyped@7204ae9 and tested it inside the model-viewer repo, my editor doesn't give typescript error so I think the changes are ok. I'm not sure how to proceed just to be sure this changes are included in @types/three 0.165.0. There may be other changes needed to the types elsewhere for 0.165.0 I am not aware of.

@Methuselah96
Copy link
Contributor

Methuselah96 commented May 15, 2024

@vincentfretin You can create a PR at https://github.com/three-types/three-ts-types. That's where the maintenance of @types/three happens and for each monthly release we just flush the changes to DefinitelyTyped, so no need to worry about other changes for 0.165.0 since those can be handled separately.

@@ -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 👍

@Methuselah96 Methuselah96 mentioned this pull request May 18, 2024
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants