diff --git a/python/rcs/sim/sim.py b/python/rcs/sim/sim.py index b6f40d33..309c4f3e 100644 --- a/python/rcs/sim/sim.py +++ b/python/rcs/sim/sim.py @@ -1,4 +1,5 @@ import atexit +import contextlib import multiprocessing as mp import uuid from logging import getLogger @@ -63,6 +64,7 @@ def __init__(self, mjmdl: str | PathLike | ModelComposer, cfg: SimConfig | None self._gui_client: Optional[_GuiClient] = None self._gui_process: Optional[mp.context.SpawnProcess] = None self._stop_event: Optional[EventClass] = None + self._gui_atexit_registered = False if cfg is not None: self.set_config(cfg) @@ -72,17 +74,26 @@ def close_gui(self): if self._gui_process is not None: self._gui_process.join() self._stop_gui_server() + self._gui_uuid = None + self._gui_client = None + self._gui_process = None + self._stop_event = None + if self._gui_atexit_registered: + with contextlib.suppress(ValueError): + atexit.unregister(self.close_gui) + self._gui_atexit_registered = False def open_gui(self): if self._gui_uuid is None: self._gui_uuid = "rcs_" + str(uuid.uuid4()) self._start_gui_server(self._gui_uuid) - if self._gui_client is None: - ctx = mp.get_context("spawn") - self._stop_event = ctx.Event() - self._gui_process = ctx.Process( + if self._gui_process is None or not self._gui_process.is_alive(): + self._stop_event = self._mp_context.Event() + self._gui_process = self._mp_context.Process( target=gui_loop, args=(self._gui_uuid, self._stop_event), ) self._gui_process.start() - atexit.register(self.close_gui) + if not self._gui_atexit_registered: + atexit.register(self.close_gui) + self._gui_atexit_registered = True diff --git a/python/tests/test_sim_gui.py b/python/tests/test_sim_gui.py new file mode 100644 index 00000000..7ea8522c --- /dev/null +++ b/python/tests/test_sim_gui.py @@ -0,0 +1,27 @@ +import multiprocessing +from pathlib import Path +from uuid import uuid4 + +REPO_ROOT = Path(__file__).resolve().parents[2] + + +def _gui_server_stop_then_step(): + import os + + os.chdir(REPO_ROOT) + from rcs.sim import Sim + + sim = Sim(Path("assets/scenes/empty_world/scene.xml")) + sim._gui_uuid = "rcs_test_" + str(uuid4()) + sim._start_gui_server(sim._gui_uuid) + sim.step(2) + sim.close_gui() + sim.step(1) + + +def test_gui_server_can_be_stopped_before_later_steps(): + ctx = multiprocessing.get_context("spawn") + proc = ctx.Process(target=_gui_server_stop_then_step) + proc.start() + proc.join(timeout=10) + assert proc.exitcode == 0, f"process exited with {proc.exitcode}" diff --git a/src/sim/gui.h b/src/sim/gui.h index 36360492..ab456e45 100644 --- a/src/sim/gui.h +++ b/src/sim/gui.h @@ -75,7 +75,7 @@ class GuiClient { private: mjModel* m; mjData* d; - const std::string& id; + const std::string id; struct shm shm; }; diff --git a/src/sim/gui_client.cpp b/src/sim/gui_client.cpp index 3eb37a21..22998796 100644 --- a/src/sim/gui_client.cpp +++ b/src/sim/gui_client.cpp @@ -9,12 +9,12 @@ namespace sim { using namespace boost::interprocess; GuiClient::GuiClient(const std::string& id) - : shm{.manager{open_only, id.c_str()}, - .state_lock{open_only, (id + STATE_LOCK_POSTFIX).c_str()}, - .info_lock{open_only, (id + INFO_LOCK_POSTFIX).c_str()}}, + : m{nullptr}, + d{nullptr}, id{id}, - m{nullptr}, - d{nullptr} { + shm{.manager{open_only, id.c_str()}, + .state_lock{open_only, (id + STATE_LOCK_POSTFIX).c_str()}, + .info_lock{open_only, (id + INFO_LOCK_POSTFIX).c_str()}} { // setup shared memory std::tie(this->shm.info_byte, std::ignore) = this->shm.manager.find(INFO_BYTE); diff --git a/src/sim/sim.cpp b/src/sim/sim.cpp index 164ad2c8..99eb9de2 100644 --- a/src/sim/sim.cpp +++ b/src/sim/sim.cpp @@ -176,11 +176,17 @@ void Sim::start_gui_server(const std::string& id) { throw std::runtime_error("Start gui server should be called only once."); } this->gui.emplace(this->m, this->d, id); - this->register_cb( - std::bind(&GuiServer::update_mjdata_callback, &this->gui.value()), 0); + if (!this->gui_callback_registered) { + this->register_cb( + [this]() { + if (this->gui.has_value()) { + this->gui->update_mjdata_callback(); + } + }, + 0); + this->gui_callback_registered = true; + } } -// TODO: when stop_gui_server is called, the callback still exists but now -// points to an non existing gui void Sim::stop_gui_server() { this->gui.reset(); } } // namespace sim } // namespace rcs diff --git a/src/sim/sim.h b/src/sim/sim.h index 9aa5f0af..6ebe5df7 100644 --- a/src/sim/sim.h +++ b/src/sim/sim.h @@ -68,6 +68,7 @@ class Sim { size_t convergence_steps = 0; bool converged = true; std::optional gui; + bool gui_callback_registered = false; public: // TODO: hide m & d, pass as parameter to callback (easier refactoring)