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

[workspace] Fix OpenUSD plugin loading #21408

Merged
merged 1 commit into from
May 9, 2024

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented May 7, 2024

Towards #20898.

Something is still weird with operator== stuff on the stage, but I'm hoping we can co-evolve that with real parser regression tests. This at least moves us closer.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added the release notes: none This pull request should not be mentioned in the release notes label May 7, 2024
@jwnimmer-tri jwnimmer-tri mentioned this pull request May 7, 2024
22 tasks
@jwnimmer-tri
Copy link
Collaborator Author

OMG I really hate custom smart pointers, but I think I finally got to the root cause and this should be ready to go now.

+@rpoyner-tri for feature review, please.

\CC @hong-nvidia FYI

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review May 7, 2024 22:25
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers


tools/workspace/openusd_internal/patches/dlopen_forbidden.patch line 1 at r2 (raw file):

WIP

Working

On the next push, I'll write some explanatory text here.

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

With fresh eyes this morning I noticed a few tweaks, and pushed some changes just now.

Reviewed 4 of 8 files at r1, 2 of 3 files at r2, 12 of 12 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 8 files at r1, 2 of 3 files at r2, 12 of 12 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers


tools/workspace/openusd_internal/patches/weakptrfacade_cxx20.patch line 11 at r3 (raw file):

`this` and `p` were different, so the swapped ordering was formally
ambiguous. We can fix this by using matching the type of `this` and
`p`. There's no need to downcast to the derived type for any reason.

nit the grammar of this sentence lost me. Not sure what to suggest exactly.

Code quote:

ambiguous. We can fix this by using matching the type of `this` and
`p`. There's no need to downcast to the derived type for any reason.

Also remove openusd_internal use of boost_internal. As it turns out,
the recent v24.05 had enough of the boost-ectomy landed that we can
shave off the last the loose ends ourselves with some small patches.
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

+@sherm1 for platform review per schedule, please.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform)


tools/workspace/openusd_internal/patches/weakptrfacade_cxx20.patch line 11 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit the grammar of this sentence lost me. Not sure what to suggest exactly.

I gave it another shot.

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5.
Reviewable status: LGTM missing from assignee sherm1(platform)

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Platform :lgtm:

Reviewed 3 of 8 files at r1, 1 of 3 files at r2, 12 of 12 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion


multibody/parsing/test/usd_parser_test/box_plane.usda line 18 at r5 (raw file):

    }
    def Cube "boxActor" (
        prepend apiSchemas = ["PhysicsCollisionAPI", "PhysicsRigidBodyAPI", "PhysicsMassAPI"]

BTW consider wrapping these to 80 chars for easier reading

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sherm1(platform)


multibody/parsing/test/usd_parser_test/box_plane.usda line 18 at r5 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW consider wrapping these to 80 chars for easier reading

This is an good thought, but I'm going to put it on the back burner for now. I think many times these files are saved by a GUI, not written by hand. As we work with more and more USD files, we'll need to come up with a policy one way or another.

@jwnimmer-tri jwnimmer-tri merged commit de0ee27 into RobotLocomotion:master May 9, 2024
10 checks passed
@jwnimmer-tri jwnimmer-tri deleted the usd-stage-open branch May 9, 2024 20:16
RussTedrake pushed a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024
Also remove openusd_internal use of boost_internal. As it turns out,
the recent v24.05 had enough of the boost-ectomy landed that we can
shave off the last the loose ends ourselves with some small patches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants