rohanpsingh / mujoco-python-viewer

Simple renderer for use with MuJoCo (>=2.1.2) Python Bindings.
BSD 2-Clause "Simplified" License
163 stars 24 forks source link

Quitting does not release ctx #12

Closed rpapallas closed 1 year ago

rpapallas commented 1 year ago

When ESC is pressed to terminate the viewer, the code will just:

print("Prssed ESC")
print("Quitting.")
glfw.terminate()
sys.exit(0)

Is there a reason why this code is not just calling the self.close() which does partly the same and in addition releases ctx?

rohanpsingh commented 1 year ago

I assumed that ctx is freed when sys exit is called. We can call self.close() and then do a sys exit too. Are there any disadvantages you see in the current implementation?

rpapallas commented 1 year ago

I think the sys.exit is the problem, not that we don't release ctx. I think when sys.exit from the viewer, the client code will terminate ungracefully. For example, on macOS, when the viewer is running and some other code is running in a separated thread, ESC (i.e., sys.exit) will not wait for the thread of the client code to finish and this gives an error. It might be better not to sys.exit in the viewer, or pass to the viewer a clean-up client code that will run when terminating before sys.exit (although this doesn't seem clean)?

rpapallas commented 1 year ago

This is the solution I propose: https://github.com/rpapallas/mujoco-python-viewer/commit/2110f91ca66a079ccd4263500fb05c77c722fa00

Let me know what you think. The client needs to do the following:

with MujocoViewer(model, data) as viewer:
    while viewer.is_running:
        viewer.render()

If not, the program will still terminate due to NotImplemenetedError error.

rohanpsingh commented 1 year ago

Hi @rpapallas Originally, I inserted the sys.exit on purpose because I did want to force kill the thread if I pressed ESC. But I agree this may not be the behavior most people want (and I never tried this on macOS).

Thanks for the PR but it causes a segfault for me on Ubuntu when I press ESC, which I think it should.
So I created a similar mechanism to exit on this branch, which seems to work fine. Could you please try this out and let me know?

rpapallas commented 1 year ago

Hi @rohanpsingh I can confirm your fix works fine on macOS, thank you!