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

Support animated extents when using draw modes #1365

Merged

Conversation

marktucker
Copy link
Contributor

As discussed in https://groups.google.com/u/1/g/usd-interest/c/oqt13R4wDdo/m/zkScXGwkCQAJ,
handle animated extent and extentsHint attributes in the draw mode adapter by
recomputing the bounds at each time sample, rather than always computing bounds
at the UsdTimeCode::EarliestTime.

Description of Change(s)

Added tests for varying extent and extentsHint attributes on prims that use the draw mode adapter.

Fixes Issue(s)

…4wDdo/m/zkScXGwkCQAJ,

handle animated extent and extentsHint attributes in the draw mode adapter by
recomputing the bounds at each time sample, rather than always computing bounds
at the UsdTimeCode::EarliestTime.
@jilliene
Copy link

Filed as internal issue #USD-6430

@@ -322,11 +338,13 @@ UsdImagingGLDrawModeAdapter::_ComputeGeometryData(
VtValue* assign) const
{
if (drawMode == UsdGeomTokens->origin) {
*extent = _ComputeExtent(prim);
*extent = _ComputeExtent(prim,
_HasVaryingExtent(prim) ? time : UsdTimeCode::EarliestTime());
Copy link
Contributor

Choose a reason for hiding this comment

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

A small thing, but it seems less error-prone to have the _HasVaryingExtent inside the _ComputeExtent call (and maybe put a warning comment in the header?).

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 did consider this, but I didn't like the semantics of calling "_ComputeExtent(time)" and having it return an extent that was computed at some other time than what the caller asked for. Still happy to make the change because they are obviously functionally equivelent, but wanted to explain my reasoning before proceeding.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair, I'm ok with skipping that change.

@c64kernal
Copy link
Contributor

Thanks for this PR @marktucker -- we're getting close to merging this in. One thought was at some point if you could add a test for this case somehow (perhaps even sending us some example files?) it'd help make sure we don't break it because this isn't a case we exercise in our pipeline currently. Thanks again Mark!

@marktucker
Copy link
Contributor Author

I can easily send example files (actually there are some attached to #1364). I could try to create an actual test if you can point me to an example of a similar test. I have to imagine this test would involve capturing the viewport of usdview, but I don't know how to create a test that does that. Or maybe there is some other way of testing this you are thinking of?

@c64kernal
Copy link
Contributor

Thanks @marktucker -- I was imagining it could be one of these kinds of tests:

https:/PixarAnimationStudios/USD/tree/dev/pxr/usdImaging/bin/testusdview

Would that work you think? (If you do end up writing the test for this, please submit as a different PR at this point because this one is ready to go). Thanks again Mark!

@marktucker
Copy link
Contributor Author

Thanks, that's was what I was looking for. I have created a test, modeled after the testUsdviewSkinning test which generated a sequence of snapshot .png files, which I put into a "baseline" directory, just like in testUsdviewSkinning. The test runs, but, as far as I can tell, the images generated by the test are not actually compared against the baseline images. I tried changing one of the images in both my test and the Skinning test, and both tests still passed, even though the generated images I'm quite certain would have been different from the intentionally ruined baseline images. Should an image comparison actually be happening? Perhaps I have configured my build incorrectly?

@spiffmon
Copy link
Member

spiffmon commented Nov 2, 2020 via email

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.

7 participants