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

USDZLoader: Make parsing more spec-conformant #28639

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jun 12, 2024

Fixed #28631

Description

This PR ensures USDZLoader parses vertex data correctly when they are annotated with meta data. It also makes sure texture paths are trimmed and normals are correctly processed during the geometry build step.

@jeffzhkw
Copy link

jeffzhkw commented Jun 12, 2024

Hello Mugen87,

I tested with your code with the sneaker file, and it shows a somewhat similar render to the screenshot I included in the issue #28631.

However, I tested this with another file, a director chair model. Google Drive Link. In this case, it renders nothing with no error in console log.

I wonder if the conversion from .usdc to .usda is not reliable and it's causing additional trouble ?

Thank you the quick response!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 12, 2024

I've added another commit that fixes the texture loading. The path contained a white space at the end which broke the loading process. An additional call of trim() solved the issue.

However, something is strange in the geometry definition. To render the asset correctly, faceVertexIndices should only apply to the vertices, not to the normal attribute. It's not yet clear to me when face indices should only affect points definitions but not normals.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 12, 2024

From https://openusd.org/dev/api/class_usd_geom_mesh.html:

The normals attribute inherited from UsdGeomPointBased is not a generic primvar, but the number of elements in this attribute will be determined by its interpolation. See UsdGeomPointBased::GetNormalsInterpolation() . If normals and primvars:normals are both specified, the latter has precedence. If a polygonal mesh specifies neither normals nor primvars:normals, then it should be treated and rendered as faceted, with no attempt to compute smooth normals.

I'll try to honor this information with another commit.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 12, 2024

@jeffzhkw With the latest commit, the sneaker should be renderer as expected. Can you confirm on your side?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 12, 2024

BTW: The chair asset contains a USDA file but it refers to other USDC files which the loader can't parse, see #24574 (comment).

@jeffzhkw
Copy link

jeffzhkw commented Jun 12, 2024

@Mugen87 I tested and can confirm the sneaker loaded properly. Also, thanks for pointing out that there are still usdc files referenced in the director chair model!

I did some additional testing on other models. Swan chair and Retro TV. I double checked and make sure the contents of these file doesn't contain any usdc.

However, models are not loaded properly and there is no error in the console. Screenshots are attached below. The camera's position and rotation are adjusted to better capture the scene.

chair_swan_a: Notice how only the bottom of the 4 legs are rendered.
Screenshot 2024-06-12 at 15 44 29

tv_retro_a: The tv's general shape is there, but there are lots of floating fragments of geometry.
Screenshot 2024-06-12 at 15 45 22

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 12, 2024

I've checked both assets and they break because they have faceVertexCounts values of 4. The loader does not support quads yet, only triangles. So this is a complete separate issue.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 12, 2024

I'll have a look at this in the next days.

For now, let's just celebrate the fact that the sneaker asset which previously produced runtime errors is now rendered in the editor like so:

image

🎉 🎉 🎉

@Mugen87 Mugen87 changed the title USDZLoader: Fix parsing of vertex data when meta data are present. USDZLoader: Make parsing more spec conform. Jun 12, 2024
@Mugen87 Mugen87 added this to the r166 milestone Jun 12, 2024
@jeffzhkw
Copy link

Thank you!

@GitHubDragonFly
Copy link
Contributor

@jeffzhkw if you would need a workaround for getting a functional USDZ with USDA packed in (out of USDZ with USDC), there is currently a webpage that provides GLB exports which can then be exported to USDZ either by using the official editor or even my GLTF Viewer.

This process appears to be functional at least for what I tried.

Just keep in mind that USDZ Loader and Exporter on my end might be slightly different than the current official ones. I have used the fix provided by @Mugen87 but had to rearrange it slightly to maintain the vertex colors functionality (which is not being used in the official files).

Not necessarily that code on my end is accurate either but seems to be functional.

@Mugen87 Mugen87 merged commit ff6cdbb into mrdoob:dev Jun 13, 2024
11 checks passed
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 13, 2024

@jeffzhkw After todays PRs the AO map of the sneakers loads now correctly. The asset looks a bit better now:

image

Since quads are now supported, the TV's geometry is now rendered correctly as well.

image

However, parsing is still incorrect since the loader does not correctly supported multiple meshes per Xform as well as nested Xforms. That means objects are wrongly composed which means missing geometries or wrong materials.

@WestLangley WestLangley changed the title USDZLoader: Make parsing more spec conform. USDZLoader: Make parsing more spec-conformant Jun 13, 2024
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 15, 2024

However, parsing is still incorrect since the loader does not correctly supported multiple meshes per Xform as well as nested Xforms. That means objects are wrongly composed which means missing geometries or wrong materials.

I've tried to fix this issue yesterday but failed since the code became a mess after a while. To fix this properly, it would be best to have an USDA parser that produces real node objects which makes it easier to process them.

For that, a parser like https://github.com/Kroxilon/usda-parser would be an ideal solution. However, I'm not sure it's safe to rely on a third party project in this case. When we detect a parsing error, we should be able to fix this on our side.

We could use the same approach like with VRMLLoader as mentioned in #28631 (comment) but writing a lexer+parser with chevrotain is a quite large task. Just the definition of all the USD tokens will take some time. I don't have the bandwidth to immediately start working on this though. I'm of course open for any other solution to fix nested XForms and multiple mesh definitions and support/review respective PRs.

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.

USDZLoader: TypeError: target is undefined
3 participants