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.

Sign up for GitHub

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
merged 2 commits into from
May 1, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c3beb64
Fix object framing for sub-trees.
Apr 27, 2020
fbdadae
Add test
May 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions lib/mayaUsd/ufe/UsdObject3d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,9 @@ Ufe::BBox3d UsdObject3d::boundingBox() const
// Would be nice to know if the object extents are animated or not, so
// we can bypass time computation and simply use UsdTimeCode::Default()
// as the time.
TfTokenVector purposes{UsdGeomTokens->default_};
UsdGeomBBoxCache bboxCache(getTime(sceneItem()->path()), purposes);
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:

maya-usd/CMakeLists.txt

Line 140 in 4fa17a2

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...

auto range = bbox.ComputeAlignedRange();
auto min = range.GetMin();
auto max = range.GetMax();
return Ufe::BBox3d(toVector3d(min), toVector3d(max));
Expand Down
21 changes: 20 additions & 1 deletion test/lib/ufe/testObject3d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

from ufeTestUtils import mayaUtils
from ufeTestUtils import usdUtils
from ufeTestUtils.testUtils import assertVectorAlmostEqual
from ufeTestUtils.testUtils import assertVectorAlmostEqual, assertVectorEqual

import ufe

Expand Down Expand Up @@ -100,6 +100,7 @@ def testBoundingBox(self):
proxyShapePathSegment = mayaUtils.createUfePathSegment(
proxyShapeMayaPath)

#######
# Create a UFE scene item from the sphere prim.
spherePathSegment = usdUtils.createUfePathSegment('/parent/sphere')
spherePath = ufe.Path([proxyShapePathSegment, spherePathSegment])
Expand All @@ -115,6 +116,24 @@ def testBoundingBox(self):
assertVectorAlmostEqual(self, ufeBBox.min.vector, [-1]*3)
assertVectorAlmostEqual(self, ufeBBox.max.vector, [1]*3)

#######
# Create a UFE scene item from the parent Xform of the sphere prim.
parentPathSegment = usdUtils.createUfePathSegment('/parent')
parentPath = ufe.Path([proxyShapePathSegment, parentPathSegment])
parentItem = ufe.Hierarchy.createItem(parentPath)

# Get its Object3d interface.
parentObject3d = ufe.Object3d.object3d(parentItem)

# Get its bounding box.
parentUFEBBox = parentObject3d.boundingBox()

# Compare it to sphere's extents.
assertVectorEqual(self, ufeBBox.min.vector, parentUFEBBox.min.vector)
assertVectorEqual(self, ufeBBox.max.vector, parentUFEBBox.max.vector)


#######
# Remove the test file.
os.remove(usdFilePath)

Expand Down
4 changes: 4 additions & 0 deletions test/lib/ufe/ufeTestUtils/testUtils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,7 @@
def assertVectorAlmostEqual(testCase, a, b, places=7):
for va, vb in zip(a, b):
testCase.assertAlmostEqual(va, vb, places)

def assertVectorEqual(testCase, a, b):
for va, vb in zip(a, b):
testCase.assertEqual(va, vb)