-
Notifications
You must be signed in to change notification settings - Fork 353
Parallel asset download #2992
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
Parallel asset download #2992
Conversation
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
arjo129
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this! This is such an amazing (and much needed) improvement in User Experience! I only have a handful of minor code-related knits.
It'd also be great if we could follow this up with some form of signalling, so that the GUI client can at least be aware of the state of the download.
| // so we need to keep this object around. | ||
| // However, everything seems to work fine without storing this. | ||
| // \todo(nkoenig): Look into removing the sdfRoot member variable. | ||
| this->dataPtr->sdfRoot = sdfRoot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Knit: I think its perfectly safe to remove this. It seems we no longer use this inside ServerPrivate and this is the only mention in Server.cc. While the copy constructor for SdfRoot has been optimized recently, this is still an unessecary operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We mainly want to keep the sdf::Element shared pointers valid since they will be used by systems to configure themselves. Some of these plugins might traverse to their parent elements via a weak pointer. If the sdf::Root object is destroyed, it could cause these shared pointers to become invalid.
I do think we should use a pointer here to avoid the extra copy, but that could be a followup PR.
src/SimulationRunner.hh
Outdated
| /// at the appropriate time. | ||
| private: std::unique_ptr<msgs::WorldControlState> newWorldControlState; | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Knit: Remove space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 17834d5
Signed-off-by: Addisu Z. Taddese <[email protected]>
|
I think the homebrew test failures are due to a type mismatch on component IDs between gz-sim and gz-msg. See gazebosim/gz-msgs#529 |
azeey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't done a full review, but can confirm it works as described.
Fixes: #1260
🎉 New feature
Summary
This will download simulation assets in background threads by default. You can force simulation to wait for assets using the
--wait-for-assetsCLI option. I'm defaulting to async download because:Test it
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.