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

Selecting an object/sub-tree in the outliner and framing it isn't working well. #472

Merged

Conversation

marsupial
Copy link
Contributor

No description provided.

@kxl-adsk kxl-adsk requested a review from ppt-adsk April 28, 2020 10:35
@kxl-adsk kxl-adsk added ufe-usd Related to UFE-USD plugin in Maya-Usd workflows Related to in-context workflows labels Apr 28, 2020
auto bbox = bboxCache.ComputeLocalBound(fPrim);
auto range = bbox.GetRange();

auto bbox = UsdGeomImageable(fPrim).ComputeUntransformedBound(getTime(sceneItem()->path()), UsdGeomTokens->default_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @marsupial , can you give us some info on why your new code is an improvement? What existing scenes were not being covered by the previous code, and are being covered by your new code? And in the spirit of properly testing your change, is there any way you can augment the existing bounding box test

https://github.com/Autodesk/maya-usd/blob/dev/test/lib/ufe/testObject3d.py#L65

so that you can validate what you're doing? E.g. in Test Driven Development style, you could write the test using the test case that you know will fail with the existing code, make your change, and demonstrate that the test now passes with your new code. How does that sound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, I'm not sure I'll be able to get the whole testing-infrastructure up and running until next week, but it's easy to reproduce the issue:

When the following usda is loaded into a proxy, selecting any of the children in the outliner and then framing them works; however, when xform1 is selected and framed, the current result is incorrect. After these changes framing xform1 works as expected.

So the easiest test would be that UsdObject3d::boundingBox() returns a bounds that encompasses all of its children.

#usda 1.0
(
    doc = """Generated from Composed Stage of root layer 
"""
)

def Xform "xform1"
{
    matrix4d xformOp:transform = ( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (0, -100, 0, 1) )
    uniform token[] xformOpOrder = ["xformOp:transform"]

    def Sphere "sphere1"
    {
        double radius = 1
        matrix4d xformOp:transform = ( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (100, 0, 0, 1) )
        uniform token[] xformOpOrder = ["xformOp:transform"]
    }

    def Cube "cube1"
    {
        double size = 2
        matrix4d xformOp:transform = ( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (0, 0, 100, 1) )
        uniform token[] xformOpOrder = ["xformOp:transform"]
    }
}

def Sphere "sphere2"
{
    double radius = 1
    matrix4d xformOp:transform = ( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (100, 100, 0, 1) )
    uniform token[] xformOpOrder = ["xformOp:transform"]
}

def Cube "cube2"
{
    double size = 2
    matrix4d xformOp:transform = ( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (0, 100, 100, 1) )
    uniform token[] xformOpOrder = ["xformOp:transform"]
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @marsupial , that's a great test case and an excellent start. Running the tests is actually pretty easy:

  • first, run the build and install stages of the maya-usd build
  • then, go to the build directory and run an individual test, e.g. in the build/RelWithDebInfo
    directory:
    $ ctest -R testObject3d -VV --output-on-failure
    and that's it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I need to do a deployment of google-test though, that's the kicker...unless it's no longer needed ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@marsupial can you elaborate on what you mean by deployment of google-test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Largely an issue here, but your test suite uses googletest, and it's more efficient if that is deployed and available for others outside the mayaUsd project (which it is currently not)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello @marsupial , not sure about what efficiency you are referring to, but perhaps none of this is relevant: the Object3d test is written in Python:
https://github.com/Autodesk/maya-usd/blob/dev/test/lib/ufe/testObject3d.py
Cheers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're BUILD_TESTS=ON still requires google-test to be installed or downloadable (neither is true here currently).

Either way, I'll be able to get the test-suite up and running, and author the test, but feel free to add one as that may not be until next week.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're BUILD_TESTS=ON still requires google-test to be installed or downloadable (neither is true here currently).

@marsupial I don't believe this is a correct assumption. To be clear, when BUILD_TESTS variable is ON, we automatically build, and install GTest library as a shared library:

if (BUILD_TESTS)

All of which happens via fetch_googletest() macro:
https://github.com/Autodesk/maya-usd/blob/dev/cmake/googletest.cmake

All of google-test headers and shared libraries can be found inside build\RelWithDebInfo\googletest-install

We do have many unit-test across the project that are written in GTest and rely on google-test library:

e.g:
https://github.com/Autodesk/maya-usd/blob/dev/test/lib/usd/utils/CMakeLists.txt

Hope this clarify things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't want google-test as a dependency I'm all for that, but currently it is.

If it's not installed or downloadable (like behind a network with no external access) you currently cannot build with -DBUILD_TESTS=ON

Please give it a try...

@marsupial marsupial force-pushed the PR/usdMayaProxyShape-subtree-framing branch from e3a5f44 to fbdadae Compare May 1, 2020 00:19
@marsupial
Copy link
Contributor Author

Added a test.
From my read of line 80 of that test, "UFE bounds should not be affected by transformation hierarchies," this all seems correct (and definitely fixes the issue), but whether the issue is really in some lower UFE->Maya layer is unknown to me.

@ppt-adsk
Copy link
Collaborator

ppt-adsk commented May 1, 2020

Thanks for the test, that's perfect. Not sure what you mean about "the issue [being] really in some lower UFE->Maya layer"; when you make the call through the UFE Object3d interface, Maya is not involved, so your test demonstrates the fix, unless I'm missing something.

With respect to Google Test, clearly we did not have a situation as constrained as yours in mind, and yes of course we depend on being able to download Google Test.

Thanks for your PR, very appreciated!

@kxl-adsk kxl-adsk merged commit 3931b1c into Autodesk:dev May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ufe-usd Related to UFE-USD plugin in Maya-Usd workflows Related to in-context workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants