From 631b16e7ad192df936195658fe79f2ada85f755c Mon Sep 17 00:00:00 2001
From: Kyle Bayes <kylebayes@google.com>
Date: Mon, 7 Aug 2023 08:21:47 -0700
Subject: [PATCH] Add better user error messages to Python renderer
 update_scene along with missing unit tests.

PiperOrigin-RevId: 554482720
Change-Id: Ief3ba496fb5720668f98fb9d44a5c82e6a14b9cc
---
 doc/changelog.rst              | 14 +++++++---
 python/mujoco/renderer.py      | 16 +++++------
 python/mujoco/renderer_test.py | 51 ++++++++++++++++++++++++++++++++--
 3 files changed, 66 insertions(+), 15 deletions(-)

diff --git a/doc/changelog.rst b/doc/changelog.rst
index 840ec836..d24d1c4e 100644
--- a/doc/changelog.rst
+++ b/doc/changelog.rst
@@ -8,10 +8,16 @@ Upcoming version (not yet released)
 General
 ^^^^^^^
 
-- Added a new :ref:`dyntype<actuator-general-dyntype>`, ``filterexact``, which updates first-order filter states with
-  the exact formula rather than with Euler integration.
-- Added an actuator attribute, :ref:`actearly<actuator-general-actearly>`, which uses semi-implicit integration for
-  actuator forces: using the next step's actuator state to compute the current actuator forces at the current timestep.
+1. Added a new :ref:`dyntype<actuator-general-dyntype>`, ``filterexact``, which updates first-order filter states with
+   the exact formula rather than with Euler integration.
+#. Added an actuator attribute, :ref:`actearly<actuator-general-actearly>`, which uses semi-implicit integration for
+   actuator forces: using the next step's actuator state to compute the current actuator forces at the current timestep.
+
+Python bindings
+^^^^^^^^^^^^^^^
+
+3. Fixed issue where calling ``update_scene`` with invalid camera name uses the default camera.
+   (`#870 <https://github.com/deepmind/mujoco/issues/870>`_)
 
 Version 2.3.7 (July 20, 2023)
 -----------------------------
diff --git a/python/mujoco/renderer.py b/python/mujoco/renderer.py
index c82deff5..4026a531 100644
--- a/python/mujoco/renderer.py
+++ b/python/mujoco/renderer.py
@@ -223,6 +223,10 @@ the clause:
       camera: An instance of `MjvCamera`, a string or an integer
       scene_option: A custom `MjvOption` instance to use to render
         the scene instead of the default.
+
+    Raises:
+      ValueError: If `camera_id` is outside the valid range, or if camera does
+        not exist.
     """
     if not isinstance(camera, _structs.MjvCamera):
       camera_id = camera
@@ -230,14 +234,10 @@ the clause:
         camera_id = _functions.mj_name2id(self._model,
                                           _enums.mjtObj.mjOBJ_CAMERA, camera_id)
         if camera_id == -1:
-          raise ValueError(f'camera_id={camera_id} is not defined in the XML.')
-      if camera_id < -1:
-        raise ValueError('camera_id cannot be smaller than -1.')
-      if camera_id >= self._model.ncam:
-        raise ValueError(
-            f'model has {self._model.ncam} fixed cameras. '
-            f'camera_id={camera_id} is invalid.'
-        )
+          raise ValueError(f'The camera "{camera}" does not exist.')
+      if camera_id < -1 or camera_id >= self._model.ncam:
+        raise ValueError(f'The camera id {camera_id} is out of'
+                         f' range [-1, {self._model.ncam}).')
 
       # Render camera.
       camera = _structs.MjvCamera()
diff --git a/python/mujoco/renderer_test.py b/python/mujoco/renderer_test.py
index f8ec0907..8aa44a97 100644
--- a/python/mujoco/renderer_test.py
+++ b/python/mujoco/renderer_test.py
@@ -23,9 +23,54 @@ import numpy as np
 @absltest.skipUnless(hasattr(mujoco, 'GLContext'),
                      'MuJoCo rendering is disabled')
 class MuJoCoRendererTest(parameterized.TestCase):
+  def test_renderer_unknown_camera_name(self):
+    xml = """
+<mujoco>
+  <worldbody>
+    <camera name="a"/>
+  </worldbody>
+</mujoco>
+"""
+    model = mujoco.MjModel.from_xml_string(xml)
+    data = mujoco.MjData(model)
+    renderer = mujoco.Renderer(model, 50, 50)
+    mujoco.mj_forward(model, data)
+    with self.assertRaisesRegex(ValueError, r'camera "b" does not exist'):
+      renderer.update_scene(data, 'b')
+
+  def test_renderer_camera_under_range(self):
+    xml = """
+<mujoco>
+  <worldbody>
+    <camera name="a"/>
+  </worldbody>
+</mujoco>
+"""
+    model = mujoco.MjModel.from_xml_string(xml)
+    data = mujoco.MjData(model)
+    renderer = mujoco.Renderer(model, 50, 50)
+    mujoco.mj_forward(model, data)
+    with self.assertRaisesRegex(ValueError, '-2 is out of range'):
+      renderer.update_scene(data, -2)
+
+  def test_renderer_camera_over_range(self):
+    xml = """
+<mujoco>
+  <worldbody>
+    <camera name="a"/>
+  </worldbody>
+</mujoco>
+"""
+    model = mujoco.MjModel.from_xml_string(xml)
+    data = mujoco.MjData(model)
+    renderer = mujoco.Renderer(model, 50, 50)
+    mujoco.mj_forward(model, data)
+    with self.assertRaisesRegex(ValueError, '1 is out of range'):
+      renderer.update_scene(data, 1)
+
   def test_renderer_renders_scene(self):
     xml = """
- <mujoco>
+<mujoco>
   <worldbody>
     <camera name="closeup" pos="0 -6 0" xyaxes="1 0 0 0 1 100"/>
     <geom name="white_box" type="box" size="1 1 1" rgba="1 1 1 1"/>
@@ -50,7 +95,7 @@ class MuJoCoRendererTest(parameterized.TestCase):
 
   def test_renderer_output_without_out(self):
     xml = """
- <mujoco>
+<mujoco>
   <worldbody>
     <camera name="closeup" pos="0 -6 0" xyaxes="1 0 0 0 1 100"/>
     <geom name="white_box" type="box" size="1 1 1" rgba="1 1 1 1"/>
@@ -82,7 +127,7 @@ class MuJoCoRendererTest(parameterized.TestCase):
 
   def test_renderer_output_with_out(self):
     xml = """
- <mujoco>
+<mujoco>
   <worldbody>
     <camera name="closeup" pos="0 -6 0" xyaxes="1 0 0 0 1 100"/>
     <geom name="white_box" type="box" size="1 1 1" rgba="1 1 1 1"/>