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

fix: improve elephant bounds #76

Closed
wants to merge 2 commits into from

Conversation

beersandrew
Copy link
Contributor

Note: the bounds on frame 0 still don't capture the entire elephant as the bounds are not captured for the entire animation however the asset is at least visible in thumbnail form and when opened in usdview

Note 2: this includes geometry as separate usd files, maybe this is useful to include as an example, maybe it's not preferred, let me know

Improves, possibly fixes: #29 , #32

- used https://viewer.needle.tools to regenerate this usdc file
- re captured a valid thumbnail

Note: the bounds on frame 0 still don't capture the entire elephant as the bounds are not captured for the entire animation however the asset is at least visible in thumbnail form and when opened in usdview
@beersandrew
Copy link
Contributor Author

Interesting note here regarding the usdchecker check. The geometry files here are separated out from the 'core' usdc file. Since the geometry files have mesh data that uses skel related primvars, the mesh is applying the SkelBindingAPI schema. usdchecker doesn't like this because the type is not a skeleton or a skelroot. If I remove this however to fix usdchecker for the geometry files, it breaks usdchecker for the main asset.

@pablode @jcowles any thoughts here? my ideas are:

  1. after the index PR lands, possibly we can only run usdchecker on the index file?
  2. don't include the geometry files separately (this feels like avoiding the problem though)
  3. add this condition to usdchecker to allow the mesh to apply the SkelBindingAPI
  4. modify the asset structure here to a more usdchecker friendly structure (is this current structure 'wrong' or 'bad'? if so, what would be the proper way to format this asset so usdchecker is happy all around?)

@meshula
Copy link
Contributor

meshula commented Feb 5, 2024

Thanks for the PR! @hybridherbst Could we ask for your input on this change?

@jcowles
Copy link
Collaborator

jcowles commented Feb 20, 2024

I think only running usdchecker on the index usd file is tempting, but I'm also not sure if that means we'll end up missing subtle errors like we saw previously in the Vehicle example...

I'd like to understand the asset structure here a bit better - might be a good one to discuss in the WG sync this week.

@pablode
Copy link
Contributor

pablode commented Feb 20, 2024

It seems to me that since any USD file can be opened individually, all of them should be considered valid. Hence, I don't think option 1 is the proper approach. Option 4 is the way to go IMO.

Looking at the files, I think there's a likelihood that geometries/Geometry_24.usd@</Geometry/Geometry> is incorrectly a Mesh. Changing it to SkelRoot makes usdchecker happy for both the geometry file and the root file.

@jcowles
Copy link
Collaborator

jcowles commented Feb 22, 2024

Ah yeah, I think that makes sense - maybe there's less to discuss here than I though, but we'll still cover it tomorrow

@jcowles
Copy link
Collaborator

jcowles commented Feb 22, 2024

Yeah, looking at the structure here, it seems broken - I don't think we should land this PR and whatever tool created it did produced a super strange structure.

I think the more targeted fix is to just remove the 220x scale from the root transform, but keep the inverted X/Z axes:

[-1, 0, 0, 0
  0, 1, 0, 0
  0, 0, -1, 0]

Which has the same effect described by Andy above and doesn't break the animation.

Update: actually, what I said above is kind of wrong, because the authored extents are 10x too small, so when you adjust the scale at the root, the bbox extent is scaled as well, which results in the exact same problem, ha

The real fix is to the extent, not the scale - though the 220x scale also seems unnecessary

@jcowles
Copy link
Collaborator

jcowles commented Feb 22, 2024

I'll send a fixed version in a new PR

@jcowles
Copy link
Collaborator

jcowles commented Feb 22, 2024

#79

@meshula
Copy link
Contributor

meshula commented Mar 3, 2024

Hey folks, do we close this one as fixed by #79?

@beersandrew beersandrew closed this Mar 3, 2024
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.

Found the main problem with ElephantWithMonochord
4 participants