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

Allow executing a paused step in SimulationRunner #61

Closed
wants to merge 1 commit into from
Closed

Allow executing a paused step in SimulationRunner #61

wants to merge 1 commit into from

Conversation

diegoferigo
Copy link
Contributor

This PR enables the execution of paused steps as described in #51.

While this is a very small change, I suspect that it's quite delicate. I'd like to start the discussion of what's needed to make it land upstream. Any recommendation about possible tests? I can initially start with something confined as in the linked issue.

Let me know if I have to change the target branch, I initially pointed ign-gazebo3.

cc @chapulina @azeey

OT

I take advantage of this PR to congratulate with all the team for the massive migration! Having the project in GitHub is amazing and believe it will pay off in the long run, I'm sure the community involvement will increase.

@chapulina
Copy link
Contributor

@osrf-jenkins run tests

@diegoferigo
Copy link
Contributor Author

diegoferigo commented Apr 18, 2020

@diegoferigo
Copy link
Contributor Author

I'd like to start the discussion of what's needed to make it land upstream.

Friendly ping @azeey @chapulina

@chapulina
Copy link
Contributor

Hi @diegoferigo , this looks like a change of behaviour to me. There could be users starting simulation paused on purpose and choosing when the iterations start being run. Would you be open to targeting this at master?

@azeey
Copy link
Contributor

azeey commented May 11, 2020

The performance/each.cc test is flaky so I don't believe it's related to this PR. The Server_TEST.cc, however, is something we need to resolve. It looks to me that, currently, calling Server::Run(false, 100, true) will block until Server::IterationCount == 100, but since it is paused, that will never happen unless another thread calls Server::SetPaused(false). This implies that there is an expectation that when the Server::Run call returns, Server::IterationCount will be 100. But, with the change in this PR, Server::Run(false, 100, true) will return after a little while and there is no guarantee that Server::IterationCount will be 100.

Similarly, as demonstrated in the failing test RunNonBlockingPaused of Server_Test.cc, Server::Run(true, 100, true) will run in the background and will keep running until Server::SetPaused(false) is called and iterations can count up again. The test fails because it expects the server to be running immediately after calling Server::SetPaused(false), but with this PR, the server is likely done running before the Server::SetPaused(false) is even called.

I agree that it would be nice to allow executing a paused step, but I think we need a new API for it. I'm not sure what that would like like yet. I need to think about it some more.

@chapulina chapulina added the 🏰 citadel Ignition Citadel label May 12, 2020
@diegoferigo
Copy link
Contributor Author

diegoferigo commented May 12, 2020

Similarly, as demonstrated in the failing test RunNonBlockingPaused of Server_Test.cc, Server::Run(true, 100, true) will run in the background and will keep running until Server::SetPaused(false) is called and iterations can count up again. The test fails because it expects the server to be running immediately after calling Server::SetPaused(false), but with this PR, the server is likely done running before the Server::SetPaused(false) is even called.

My understanding of the failure is the change of behaviour about how simulator iterations are counted internally:

  • Currently: the server can perform a number of paused steps and calling server.IterationCount() would provide 0. Though, the numbers of steps to excute are cached internally and they are processed as soon as the server gets unpaused.
  • With this PR: the server can perform a number of paused steps and calling server.IterationCount() would provide 0. Though, the numbers of steps to execute are not cached internally and they are eaten by paused steps. This means that when the server gets unpaused, there are less (or no) iterations left to be executed.

I agree that it would be nice to allow executing a paused step, but I think we need a new API for it. I'm not sure what that would like like yet. I need to think about it some more.

Awesome, thanks, I'll be waiting further information then! A use case that I found very interesting with single paused steps is to create a server object that starts paused and provides the possibility to get stepped deterministically:

ignition::gazebo::ServerConfig config;
config.SetSdfFile("empty.sdf");
config.SetUpdateRate(1000);

ignition::gazebo::Server server(config);

server->Run(/*blocking=*/true, /*iterations=*/1, /*paused=*/true)

After this operation, the caller has complete control of the simulation, and it is able to perform either other paused steps (e.g. to insert models and trigger the ECM to add their entities and components) or start running the simulation issuing unpaused commands. It fits gracefully with the idea of having a simulator-as-a-library, and it would simplify dramatically the creation of simulator bindings (gazebosim/gz-physics#3) similar to pybullet and mujoco-py, to name a few. I've been using this branch for a while now, and I'm very happy of the performance.

Hi @diegoferigo , this looks like a change of behaviour to me. There could be users starting simulation paused on purpose and choosing when the iterations start being run. Would you be open to targeting this at master?

I can surely change the target branch. Considering also @azeey comments, it seems that new APIs are needed in any case.

@chapulina
Copy link
Contributor

a server object that starts paused and provides the possibility to get stepped deterministically

+1, this is an important use case that's currently underserved.

I can surely change the target branch. Considering also @azeey comments, it seems that new APIs are needed in any case.

Depending on the APIs, they could be backwards compatible. My main concern is not breaking the current behaviour. If we can support your use case while still keeping the current behaviour by default, it would be great to do it in Citadel.

@diegoferigo diegoferigo changed the base branch from ign-gazebo3 to master May 18, 2020 07:59
@diegoferigo
Copy link
Contributor Author

Depending on the APIs, they could be backwards compatible. My main concern is not breaking the current behaviour. If we can support your use case while still keeping the current behaviour by default, it would be great to do it in Citadel.

In the meantime I updated the PR to target master. If we find a backward-compatible solution I'd be more than happy to rebase it again to target Citadel.

@diegoferigo
Copy link
Contributor Author

Is there any chance to get this merged within the Dome window? For other few weeks I won't have much time for major extensions, but I'd like at least to start the discussion of the actions to take waiting August.

@chapulina
Copy link
Contributor

Hi Diego, sorry for the wait. I'm leaning towards adding new API for ECS iterations (as opposed to simulation iterations) in Dome. This would allow us to maintain the current behaviour, as well as support your use case. Is this something you'd be interested in helping with?

Sorry for the back-and-forth, thanks for redirecting this PR to master, but I think that's the right thing to do in order to avoid confusion in the future. The current semantics of iteration is "simulation iterations", so if you're running paused and blocked, it should loop until unpaused.

My proposal:

@diegoferigo
Copy link
Contributor Author

Hi @chapulina, no problem, that sounds a good plan to me. I can definitely help with this task 😉 I tried to recap the current status in #51 (comment), please let me know if you already have something more concrete in mind. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants