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

Use simplified /get_scene service #349

Merged
merged 2 commits into from
Feb 9, 2021
Merged

Conversation

scpeters
Copy link
Contributor

This uses a simplified form of /get_scene proposed in maliput/delphyne#746 that avoids the need for a second service call. It also happens to match the signature expected by the ign-gui Scene3D plugin.

Use a simplified form of /get_scene that avoids the need
for a second service call.
@scpeters scpeters requested a review from agalbachicar January 28, 2021 20:35
agalbachicar
agalbachicar previously approved these changes Jan 29, 2021
Copy link
Collaborator

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -132,8 +132,9 @@ class RenderWidget : public ignition::gui::Plugin {

private:
/// \brief Set the initial scene
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we edit the docstring to state that it will only happen when result is true ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

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

LGTM

@scpeters scpeters merged commit de1195e into master Feb 9, 2021
@scpeters scpeters deleted the scpeters/simplify_get_scene branch February 9, 2021 01:49
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.

2 participants