Skip to content

Add handles for overlaying user text and images to the Passive Viewer #2503

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

Merged
merged 6 commits into from
Apr 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions python/mujoco/simulate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ class SimulateWrapper {

void Destroy() {
if (simulate_) {
ClearImages();
delete simulate_;
simulate_ = nullptr;
destroyed_.store(1);
Expand Down Expand Up @@ -148,6 +149,59 @@ class SimulateWrapper {

void ClearFigures() { simulate_->user_figures_.clear(); }

void SetTexts(
const std::vector<std::tuple<int, int, std::string, std::string>>& texts) {
// Collection of [font, gridpos, text1, text2] tuples for overlay text
std::vector<std::tuple<int, int, std::string, std::string>> user_texts;
for (const auto& [font, gridpos, text1, text2] : texts) {
user_texts.push_back(std::make_tuple(font, gridpos, text1, text2));
}

// Set them all at once to prevent text flickering.
simulate_->user_texts_ = user_texts;
}

void ClearTexts() { simulate_->user_texts_.clear(); }

void SetImages(
const std::vector<std::tuple<mjrRect, pybind11::array&>> viewports_images
) {
// Clear previous images to prevent memory leaks
ClearImages();

for (const auto& [viewport, image] : viewports_images) {
auto buf = image.request();
if (buf.ndim != 3) {
throw std::invalid_argument("image must have 3 dimensions (H, W, C)");
}
if (static_cast<int>(buf.shape[2]) != 3) {
throw std::invalid_argument("image must have 3 channels");
}
if (buf.itemsize != sizeof(unsigned char)) {
throw std::invalid_argument("image must be uint8 format");
}

// Calculate size of the image data
size_t height = buf.shape[0];
size_t width = buf.shape[1];
size_t size = height * width * 3;

// Make a copy of the image data to prevent flickering
unsigned char* image_copy = new unsigned char[size];
Copy link
Contributor

Choose a reason for hiding this comment

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

See top level review comment about how we should handle locking. It should prevent the flickering and the need to make this copy.

std::memcpy(image_copy, buf.ptr, size);

simulate_->user_images_.push_back(std::make_tuple(viewport, image_copy));
Copy link
Contributor

Choose a reason for hiding this comment

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

user_images_ is being modified while it is potentially used by Simulate::Render. See top level review comment about how we can resolve this issue in general.

}
}

void ClearImages() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method deallocates memory and clears the vector that may be being used concurrently by Simulate::Render. See top level review comment about how we can resolve this issue in general.

// Free memory for each image before clearing the vector
for (const auto& [viewport, image_ptr] : simulate_->user_images_) {
delete[] image_ptr;
}
simulate_->user_images_.clear();
}

private:
mujoco::Simulate* simulate_;
std::atomic_int destroyed_ = 0;
Expand Down Expand Up @@ -249,6 +303,12 @@ PYBIND11_MODULE(_simulate, pymodule) {
.def("set_figures", &SimulateWrapper::SetFigures,
py::arg("viewports_figures"))
.def("clear_figures", &SimulateWrapper::ClearFigures)
.def("set_texts", &SimulateWrapper::SetTexts,
py::arg("overlay_texts"))
.def("clear_texts", &SimulateWrapper::ClearTexts)
.def("set_images", &SimulateWrapper::SetImages,
py::arg("viewports_images"))
.def("clear_images", &SimulateWrapper::ClearImages)
.def_property_readonly("m", &SimulateWrapper::GetModel)
.def_property_readonly("d", &SimulateWrapper::GetData)
.def_property_readonly("viewport", &SimulateWrapper::GetViewport)
Expand Down
84 changes: 82 additions & 2 deletions python/mujoco/viewer.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import sys
import threading
import time
from typing import Callable, Optional, Tuple, Union
from typing import Callable, List, Optional, Tuple, Union
import weakref

import glfw
Expand Down Expand Up @@ -115,16 +115,96 @@ def viewport(self):
return sim.viewport
return None

def set_figures(self, viewports_figures):
def set_figures(
self, viewports_figures: Union[Tuple[mujoco.MjrRect, mujoco.MjvFigure],
List[Tuple[mujoco.MjrRect, mujoco.MjvFigure]]]
):
"""Overlay figures on the viewer.

Args:
viewports_figures: Single tuple or list of tuples of (viewport, figure)
viewport: Rectangle defining position and size of the figure
figure: MjvFigure object containing the figure data to display
"""
sim = self._sim()
if sim is not None:
# Convert single tuple to list if needed
if isinstance(viewports_figures, tuple):
viewports_figures = [viewports_figures]
sim.set_figures(viewports_figures)

def clear_figures(self):
sim = self._sim()
if sim is not None:
sim.clear_figures()

def set_texts(self, texts: Union[Tuple[Optional[int], Optional[int], Optional[str], Optional[str]],
List[Tuple[Optional[int], Optional[int], Optional[str], Optional[str]]]]):
"""Overlay text on the viewer.

Args:
texts: Single tuple or list of tuples of (font, gridpos, text1, text2)
font: Font style from mujoco.mjtFontScale
Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible to make font_scale and gridpos use reasonable defaults if None is passed. Additionally, is it possible to pass text1 and text2 as None? (right now the user has to pass an empty string).

Additionally, it would be nice to make this API more pythonic in general, but I don't see a straightforward way to do so that is consistent with the other set_X functions.

gridpos: Position of text box from mujoco.mjtGridPos
text1: Left text column, defaults to empty string if None
text2: Right text column, defaults to empty string if None
"""
sim = self._sim()
if sim is not None:
# Convert single tuple to list if needed
if isinstance(texts, tuple):
texts = [texts]

# Convert None values to empty strings
default_font = mujoco.mjtFontScale.mjFONTSCALE_150
default_gridpos = mujoco.mjtGridPos.mjGRID_TOPLEFT
processed_texts = [(
default_font if font is None else font,
default_gridpos if gridpos is None else gridpos,
"" if text1 is None else text1,
"" if text2 is None else text2)
for font, gridpos, text1, text2 in texts]

sim.set_texts(processed_texts)

def clear_texts(self):
sim = self._sim()
if sim is not None:
sim.clear_texts()

def set_images(
Copy link
Contributor

Choose a reason for hiding this comment

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

This method should get a docstring as overlay_text does.

self, viewports_images: Union[Tuple[mujoco.MjrRect, np.ndarray],
List[Tuple[mujoco.MjrRect, np.ndarray]]]
):
"""Overlay images on the viewer.

Args:
viewports_images: Single tuple or list of tuples of (viewport, image)
viewport: Rectangle defining position and size of the image
image: RGB image with shape (height, width, 3)
"""
sim = self._sim()
if sim is not None:
# Convert single tuple to list if needed
if isinstance(viewports_images, tuple):
viewports_images = [viewports_images]

processed_images = []
for viewport, image in viewports_images:
targ_shape = (viewport.height, viewport.width)
# Check if image is already the correct shape
if image.shape[:2] != targ_shape:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are no longer resizing the image, passing a viewport with the image is no longer required, the size is implicit and this check is not needed. However, I'm ok with leaving the check here to avoid breaking the API in the future if resizing is brought back somehow.

raise ValueError(f"Image shape {image.shape[:2]} does not match target shape {targ_shape}")
flipped = np.flip(image, axis=0)
contiguous = np.ascontiguousarray(flipped)
processed_images.append((viewport, contiguous))
sim.set_images(processed_images)

def clear_images(self):
sim = self._sim()
if sim is not None:
sim.clear_images()

def close(self):
sim = self._sim()
if sim is not None:
Expand Down
18 changes: 18 additions & 0 deletions simulate/simulate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,14 @@ void ShowFigure(mj::Simulate* sim, mjrRect viewport, mjvFigure* fig){
mjr_figure(viewport, fig, &sim->platform_ui->mjr_context());
}

void ShowOverlayText(mj::Simulate* sim, mjrRect viewport, int font, int gridpos, std::string text1, std::string text2){
mjr_overlay(font, gridpos, viewport, text1.c_str(), text2.c_str(), &sim->platform_ui->mjr_context());
}

void ShowImage(mj::Simulate* sim, mjrRect viewport, const unsigned char* image) {
mjr_drawPixels(image, nullptr, viewport, &sim->platform_ui->mjr_context());
}

// load state from history buffer
static void LoadScrubState(mj::Simulate* sim) {
// get index into circular buffer
Expand Down Expand Up @@ -2597,6 +2605,16 @@ void Simulate::Render() {
ShowFigure(this, viewport, &figure);
Copy link
Contributor

Choose a reason for hiding this comment

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

These calls to ShowFigure, ShowOverlayText and ShowImage are being made from Simulate's Render function. Render is called from the RenderLoop after unlocking Simulate's lock (this helps with parallelism and makes the viewer run smoother).

However, ShowFigure, ShowOverlayText and ShowImage use vectors, user_figures_, user_text_, and user_images_ which can be modified concurrently by the user code. The result is that undefined behavior can occur.

See top level review comment about how we can resolve this issue in general.

}

// overlay text
for (auto& [font, gridpos, text1, text2] : this->user_texts_) {
ShowOverlayText(this, rect, font, gridpos, text1, text2);
}

// user images
for (auto& [viewport, image] : this->user_images_) {
ShowImage(this, viewport, image);
}

// finalize
this->platform_ui->SwapBuffers();
}
Expand Down
4 changes: 3 additions & 1 deletion simulate/simulate.h
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,12 @@ class Simulate {
mjvFigure figsize = {};
mjvFigure figsensor = {};

// additional user-defined visualization geoms (used in passive mode)
// additional user-defined visualization
mjvScene* user_scn = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The comment just above this line (Github won't let me comment on that line in this review) reads additional user-defined visualization geoms (used in passive mode). Can the comment be updated? Additionally, as far as I can tell, methods should work outside of passive mode.

mjtByte user_scn_flags_prev_[mjNRNDFLAG];
std::vector<std::pair<mjrRect, mjvFigure>> user_figures_;
std::vector<std::tuple<int, int, std::string, std::string>> user_texts_;
std::vector<std::tuple<mjrRect, unsigned char*>> user_images_;

// OpenGL rendering and UI
int refresh_rate = 60;
Expand Down
Loading