Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Create basic visualiser #17

Merged
merged 37 commits into from
Aug 14, 2023
Merged

Create basic visualiser #17

merged 37 commits into from
Aug 14, 2023

Conversation

nic-barbara
Copy link
Collaborator

@nic-barbara nic-barbara commented Aug 7, 2023

This is to track progress on development of a basic visualiser - don't merge to main just yet :)

The main idea is to create a visualiser built off the LyceumMuJoCoViz.jl package that is easy to use and integrate into custom simulation/control loops. As per #3, the syntax we want is something like this:

viewer = MuJoCoViewer(model, data)
while <window is open>
    # do controls, data.ctrl = ....
    step!(model, data)
    render!(viewer)
end
close!(viewer)

The render!(viewer) step should be set up with mouse/button callbacks to make the visualisation interactive.

@nic-barbara nic-barbara linked an issue Aug 7, 2023 that may be closed by this pull request
@nic-barbara nic-barbara marked this pull request as draft August 7, 2023 23:03
@nic-barbara nic-barbara requested a review from JamieMair August 7, 2023 23:03
@nic-barbara
Copy link
Collaborator Author

Current status: basic viewer that displays the model, but no interactivity yet. We should try to use the PhysicsState type fromLyceumMuJoCoViz.jl to integrate their default callbacks and handlers into our visualiser. This should be reasonably straightforward once we have nicer array handling on the structs.

@nic-barbara nic-barbara self-assigned this Aug 7, 2023
@JamieMair JamieMair marked this pull request as ready for review August 12, 2023 07:05
Copy link
Owner

@JamieMair JamieMair left a comment

Choose a reason for hiding this comment

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

For this PR, I suggest we can commit the files in vis (except for basic.cc) after some minor tweaks, but we should remove the lyceum-vis from this branch and put it in the next PR. You can ignore all the comments on the lyceum code for now, as it will only be useful as a guide for the next PR.

We should probably move to package extensions in the next PR. It doesn't seem too involved. We can follow this post for a guide, and also the docs. This should allow us to use Requires.jl as well to support people on older versions, before v1.9.

examples/lyceum-vis/LyceumMuJoCoViz.jl Outdated Show resolved Hide resolved
examples/lyceum-vis/LyceumMuJoCoViz.jl Outdated Show resolved Hide resolved
examples/lyceum-vis/LyceumMuJoCoViz.jl Outdated Show resolved Hide resolved
examples/lyceum-vis/LyceumMuJoCoViz.jl Outdated Show resolved Hide resolved
examples/lyceum-vis/glfw.jl Outdated Show resolved Hide resolved
Comment on lines 23 to 27
scn::Ptr{mjvScene} = alloc(mjvScene)
cam::Ptr{mjvCamera} = alloc(mjvCamera)
vopt::Ptr{mjvOption} = alloc(mjvOption)
con::Ptr{mjrContext} = alloc(mjrContext)
figsensor::Ptr{mjvFigure} = alloc(mjvFigure)
Copy link
Owner

Choose a reason for hiding this comment

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

alloc is fine here, but we should add a finaliser to GC the objects since this is a mutable struct.

examples/vis/visualiser.jl Outdated Show resolved Hide resolved
m.internal_pointer,
d.internal_pointer,
viewer.ui.vopt,
C_NULL,
Copy link
Owner

Choose a reason for hiding this comment

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

Remind me what this C_NULL is again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just NULL but in C.

Copy link
Owner

Choose a reason for hiding this comment

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

I mean the argument to the function, why are we setting it to null? The function calls it pert but I'm not sure what that is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's either perturbations to the scene (eg: dragging around the view with the mouse) or perturbations to the environment (eg: due to dragging around objects in the environment). I've set it as null for now because we don't need either of those for the most basic visualiser.

Comment on lines 150 to 162
function close_viewer!(viewer::MuJoCoViewer)

LibMuJoCo.mjv_freeScene(viewer.ui.scn)
LibMuJoCo.mjr_freeContext(viewer.ui.con)

Libc.free(viewer.ui.cam)
Libc.free(viewer.ui.vopt)
Libc.free(viewer.ui.scn)
Libc.free(viewer.ui.con)

GLFW.DestroyWindow(viewer.manager.state.window)

return nothing
Copy link
Owner

Choose a reason for hiding this comment

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

This is all great - we should move the frees into the GC.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. How do we go about doing that?

Copy link
Owner

Choose a reason for hiding this comment

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

We should probably wrap these objects in a type just like Model and Data to make things easier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. That's happening in #35 right?

src/MuJoCo.jl Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Does this need the using UnsafeArrays?

Also, good idea to validate the path. Perhaps there is a mj function which also validates an xml file that we can use? But that's for a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly not, I'll check.

@JamieMair JamieMair linked an issue Aug 13, 2023 that may be closed by this pull request
3 tasks
@nic-barbara nic-barbara merged commit 5b0bc4d into main Aug 14, 2023
@nic-barbara nic-barbara deleted the visualiser branch August 28, 2023 07:04
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
2 participants