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

Design document for syncing GUI ECM with server ECM #8

Merged
merged 5 commits into from
Dec 13, 2021

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Oct 6, 2021

Signed-off-by: Ashton Larkin 42042756+adlarkin@users.noreply.github.com

I've added a design document for how we can go about updating the server's ECM if the GUI's ECM is modified directly while simulation is paused.

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
@adlarkin adlarkin requested review from nkoenig and chapulina October 6, 2021 19:53
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Show resolved Hide resolved
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Show resolved Hide resolved
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
@adlarkin
Copy link
Contributor Author

adlarkin commented Oct 7, 2021

Thanks for the feedback, @chapulina! I have spent some time revising the document (and a bit of the design itself) and have produced a second draft in 5650632, which is ready for another review.

@adlarkin adlarkin requested a review from chapulina October 7, 2021 23:06
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Show resolved Hide resolved
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
@chapulina
Copy link
Contributor

Now that the implementation is underway, it would be nice to eventually update the design to reflect the implementation. Has it diverged, @adlarkin ?

@adlarkin
Copy link
Contributor Author

adlarkin commented Nov 9, 2021

Yeah, the design will need to be updated. I will update the design here to reflect the new changes discussed in gazebosim/gz-sim#1109 (comment) and let you know when this PR is ready for another review (I should have it done this week).

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
@adlarkin
Copy link
Contributor Author

The design document has been updated to reflect the implementation in gazebosim/gz-sim#1109 (comment).

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Show resolved Hide resolved

### Server/Client Same Process
When running the server and client in the same process, the server and GUI share the same ECM.
In this scenario, the GUI (`gazebo::GuiRunner`) does not need to share its ECM with the server (`gazebo::SimulationRunner`), but the server still needs to make sure that server plugins are updated according to any changes made to the ECM while paused.
Copy link
Contributor

Choose a reason for hiding this comment

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

@mjcarroll , do you think this is reasonable for same-process? Or do we need a mechanism to bundle the GUI-only changes and sync them with the server on play? My concern is that every little change to the GUI ECM will be immediately processed by the server separately unless we bundle them. I think we also need to take the Recreate component into account somewhere in here.

gui_server_ecm_sync.md Outdated Show resolved Hide resolved
gui_server_ecm_sync.md Outdated Show resolved Hide resolved
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for iterating!

@adlarkin
Copy link
Contributor Author

Thanks for iterating!

Does #8 (comment) need to be addressed before merging, or is that comment just something to think about for the same process work?

@chapulina
Copy link
Contributor

Does #8 (comment) need to be addressed before merging, or is that comment just something to think about for the same process work?

I wouldn't block on that

@adlarkin adlarkin merged commit 046101c into main Dec 13, 2021
@adlarkin adlarkin deleted the adlarkin/gui_server_ecm branch December 13, 2021 22:30
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.

3 participants