Add light keyboard control by yuecideng · Pull Request #140 · DexForce/EmbodiChain

yuecideng · 2026-02-14T07:25:57Z

Description

Similar with camera keyboard control. This PR adds control for light, including position, intensity and color.

Type of change

New feature (non-breaking change which adds functionality)

Checklist

I have run the black . command to format the code base.
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
Dependencies have been updated, if applicable.

Copilot

Pull request overview

This PR adds keyboard control functionality for lights in the simulation environment, similar to the existing camera keyboard control feature. The changes enable interactive adjustment of light position, intensity, falloff, and color through keyboard commands.

Changes:

Added run_keyboard_control_for_light function with keyboard controls for light properties (position, intensity, falloff/radius, and RGB color channels)
Updated camera and robot control functions to accept string UIDs in addition to object instances
Added get_light_uid_list method to SimulationManager for retrieving light UIDs
Enhanced preview_sensor_data method with save parameter and changed default visualization method from matplotlib to cv2
Made get_data_class function more extensible by adding optional extra_modules parameter

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
embodichain/lab/sim/utility/keyboard_utils.py	Added light keyboard control function with translation, intensity, falloff, and color controls; updated camera control to accept string UID
embodichain/lab/sim/utility/gizmo_utils.py	Updated robot gizmo control to accept string UID in addition to Robot object
embodichain/lab/sim/sim_manager.py	Added `get_light_uid_list` method to retrieve all light UIDs
embodichain/lab/gym/envs/embodied_env.py	Enhanced sensor preview with save parameter and changed default method from "plt" to "cv2"
embodichain/data/dataset.py	Added extensibility to `get_data_class` with optional `extra_modules` parameter

Comments suppressed due to low confidence (1)

embodichain/lab/sim/utility/gizmo_utils.py:97

When robot is a string and sim.get_robot returns None (robot not found), the code will attempt to access robot methods at line 97, resulting in an AttributeError. Add a null check after line 91 to handle the case where the robot is not found.

    if isinstance(robot, str):
        robot = sim.get_robot(uid=robot)

    # Enter auto-update mode.
    sim.set_manual_update(False)

    # Replace robot's default solver with PinkSolver for gizmo control.
    robot_solver = robot.get_solver(name=control_part)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-14T07:31:13Z

embodichain/lab/gym/envs/embodied_env.py

+        method: str = "cv2",
+        save: bool = False,
    ) -> None:
        """Preview the sensor data by matplotlib


The documentation comment says "Preview the sensor data by matplotlib" but this is no longer accurate since the default method is now "cv2" instead of "plt". Please update the docstring to reflect that the function can preview sensor data using either matplotlib or cv2.

Copilot · 2026-02-14T07:31:13Z

embodichain/lab/sim/utility/keyboard_utils.py

+        intensity_step (float, optional): Intensity adjustment step. Defaults to 0.1.
+        falloff_step (float, optional): Falloff/radius adjustment step. Defaults to 0.1.


The default values in the docstring don't match the function signature. The docstring states "Defaults to 0.1" for both intensity_step and falloff_step, but the actual defaults are 1.0 for both parameters. Please update the docstring to match the actual default values of 1.0.

Suggested change

intensity_step (float, optional): Intensity adjustment step. Defaults to 0.1.

falloff_step (float, optional): Falloff/radius adjustment step. Defaults to 0.1.

intensity_step (float, optional): Intensity adjustment step. Defaults to 1.0.

falloff_step (float, optional): Falloff/radius adjustment step. Defaults to 1.0.

embodichain/lab/gym/envs/embodied_env.py

Copilot · 2026-02-14T07:31:14Z

embodichain/lab/sim/utility/keyboard_utils.py

+        except:
+            pass


The bare except clause at line 505 silently catches all exceptions without logging or handling them. This can hide errors during terminal restoration. Consider catching specific exceptions (e.g., termios.error, OSError) and logging them if they occur, similar to how it's handled in the camera control function.

Suggested change

except:

pass

except (termios.error, OSError) as e:

log_warning(f"Failed to restore terminal settings: {e}")

Copilot · 2026-02-14T07:31:15Z

embodichain/lab/sim/utility/keyboard_utils.py

+        return
+
+    if isinstance(light, str):
+        light: Light = sim.get_light(uid=light)


When light is a string and sim.get_light returns None (light not found), the code will attempt to access light.num_instances at line 280, resulting in an AttributeError. Add a null check after line 278 to handle the case where the light is not found, similar to how it might be handled elsewhere in the codebase.

Suggested change

light: Light = sim.get_light(uid=light)

light_name = light

light: Light | None = sim.get_light(uid=light_name)

if light is None:

log_error(f"Light '{light_name}' not found. Aborting keyboard control.")

return

Copilot · 2026-02-14T07:31:15Z

embodichain/lab/sim/utility/keyboard_utils.py

+
+
+def run_keyboard_control_for_light(
+    light: object | str,


The type hint for the 'light' parameter uses 'object | str' in the function signature, but the docstring specifies 'Light | str'. For consistency with the camera control function which uses 'Camera | str' in both places, consider using the specific type 'Light | str' in the function signature as well. The Light class is already imported at line 266, so it could be imported at the module level or used with TYPE_CHECKING and forward references.

Copilot · 2026-02-14T07:31:15Z

embodichain/lab/sim/utility/keyboard_utils.py

+
+    marker = None
+    if vis_pose:
+        from embodichain.lab.sim import SimulationManager


The SimulationManager is imported twice in this function - once at line 267 and again at line 317 inside the vis_pose block. The second import at line 317 is redundant since the same import already exists earlier. Consider removing the duplicate import at line 317.

Suggested change

from embodichain.lab.sim import SimulationManager

Copilot · 2026-02-14T07:31:15Z

embodichain/lab/sim/utility/keyboard_utils.py

+
+        init_marker_pose = light.get_local_pose(to_matrix=True).squeeze().numpy()
+
+        sim = SimulationManager.get_instance()


The variable 'sim' is assigned at line 269, then reassigned at line 322 inside the vis_pose block. The second assignment at line 322 is redundant because it retrieves the same singleton instance. Consider removing the duplicate assignment.

Suggested change

sim = SimulationManager.get_instance()

Copilot · 2026-02-14T07:31:15Z

embodichain/lab/gym/envs/embodied_env.py

+        name: str,
+        data_type: str = "color",
+        env_ids: int = 0,
+        method: str = "cv2",


The default method parameter has been changed from "plt" to "cv2". This is a breaking change to the API that could affect existing code. When method="cv2" and save=False, the behavior is different from when method="plt" and save=False - the cv2 method blocks with waitKey(0), while plt shows and blocks as well. Consider whether this default change is intentional and if it should be documented as a breaking change.

Suggested change

method: str = "cv2",

method: str = "plt",

Copilot

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-14T07:53:06Z

embodichain/lab/sim/utility/keyboard_utils.py

+        intensity_step (float, optional): Intensity adjustment step. Defaults to 0.1.
+        falloff_step (float, optional): Falloff/radius adjustment step. Defaults to 0.1.


The documented default values in the docstring (0.1) do not match the actual parameter defaults (1.0). The docstring states "Defaults to 0.1" for both intensity_step and falloff_step parameters, but the function signature shows defaults of 1.0 for both. Please update the docstring to match the actual defaults of 1.0.

Suggested change

intensity_step (float, optional): Intensity adjustment step. Defaults to 0.1.

falloff_step (float, optional): Falloff/radius adjustment step. Defaults to 0.1.

intensity_step (float, optional): Intensity adjustment step. Defaults to 1.0.

falloff_step (float, optional): Falloff/radius adjustment step. Defaults to 1.0.

Copilot · 2026-02-14T07:53:06Z

embodichain/lab/sim/utility/keyboard_utils.py

+
+    marker = None
+    if vis_pose:
+        from embodichain.lab.sim import SimulationManager


The import statement for 'SimulationManager' at line 318 is redundant. It was already imported at line 268 within the same function scope. Remove this duplicate import to improve code cleanliness.

Suggested change

from embodichain.lab.sim import SimulationManager

Copilot · 2026-02-14T07:53:06Z

embodichain/lab/sim/utility/keyboard_utils.py

+        sim = SimulationManager.get_instance()
+        marker = sim.draw_marker(


The variable 'sim' is reassigned at line 323, but it was already assigned at line 270. This redundant assignment adds unnecessary code. Remove line 323 since 'sim' is already initialized and available from the earlier scope.

Suggested change

sim = SimulationManager.get_instance()

marker = sim.draw_marker(

marker = SimulationManager.get_instance().draw_marker(

Copilot · 2026-02-14T07:53:07Z

embodichain/lab/sim/utility/keyboard_utils.py

+        return
+
+    if isinstance(light, str):
+        light: Light = sim.get_light(uid=light)


Missing null check after 'sim.get_light(uid=light)'. The 'get_light' method returns 'Light | None' and logs a warning if the light is not found, but the code proceeds without checking if the returned value is None. This could lead to an AttributeError when trying to access 'light.num_instances' on the next line. Add a null check and return early if the light is not found, similar to how it's done in 'run_gizmo_robot_control_loop' for robots.

Suggested change

light: Light = sim.get_light(uid=light)

light_obj = sim.get_light(uid=light)

if light_obj is None:

return

light = light_obj

Copilot · 2026-02-14T07:53:07Z

embodichain/lab/sim/utility/gizmo_utils.py

    sim = SimulationManager.get_instance()

+    if isinstance(robot, str):
+        robot = sim.get_robot(uid=robot)


Missing null check after 'sim.get_robot(uid=robot)'. The 'get_robot' method returns 'Robot | None' and logs a warning if the robot is not found, but the code proceeds without checking if the returned value is None. This could lead to an AttributeError when trying to access robot methods on the next lines. Add a null check and return early if the robot is not found.

Suggested change

robot = sim.get_robot(uid=robot)

robot_uid = robot

robot = sim.get_robot(uid=robot_uid)

if robot is None:

log_error(f"Robot with uid '{robot_uid}' not found; aborting gizmo control loop.")

return

Copilot · 2026-02-14T07:53:07Z

embodichain/lab/sim/utility/keyboard_utils.py

+        except:
+            pass


Bare except clause on line 506 suppresses all exceptions without logging or handling them. This makes debugging difficult if terminal restoration fails. Consider catching specific exceptions (e.g., 'termios.error', 'OSError') or at least log the exception before passing.

Suggested change

except:

pass

except (termios.error, OSError) as exc:

log_warning(f"Failed to restore terminal settings: {exc}")

Copilot · 2026-02-14T07:53:07Z

embodichain/lab/sim/utility/keyboard_utils.py

+
+
+def run_keyboard_control_for_light(
+    light: object | str,


The type hint for the 'light' parameter is 'object | str', but the docstring correctly states 'Light | str'. The type hint should be updated to match the docstring for better type checking. Change the type hint from 'object' to 'Light', which requires importing the Light class at the top of the file or using a string literal type hint 'Light | str' in quotes.

yuecideng added 3 commits February 13, 2026 16:07

wip

e0de921

wip

c600b48

wip

0d9b22c

Copilot AI review requested due to automatic review settings February 14, 2026 07:25

yuecideng added the interaction The interaction simulation features label Feb 14, 2026

Copilot started reviewing on behalf of yuecideng February 14, 2026 07:26 View session

Copilot AI reviewed Feb 14, 2026

View reviewed changes

yuecideng added 2 commits February 14, 2026 15:38

wip

e24a360

Merge remote-tracking branch 'origin/main' into yueci/add-light-control

ab49f6a

Copilot AI review requested due to automatic review settings February 14, 2026 07:47

Copilot started reviewing on behalf of yuecideng February 14, 2026 07:48 View session

wip

48d864c

Copilot AI reviewed Feb 14, 2026

View reviewed changes

yuecideng merged commit 8333ae2 into main Feb 14, 2026
5 checks passed

yuecideng deleted the yueci/add-light-control branch February 14, 2026 08:15

		intensity_step (float, optional): Intensity adjustment step. Defaults to 0.1.
		falloff_step (float, optional): Falloff/radius adjustment step. Defaults to 0.1.

-        except:
-            pass
+        except (termios.error, OSError) as e:
+            log_warning(f"Failed to restore terminal settings: {e}")

-        light: Light = sim.get_light(uid=light)
+        light_name = light
+        light: Light | None = sim.get_light(uid=light_name)
+        if light is None:
+            log_error(f"Light '{light_name}' not found. Aborting keyboard control.")
+            return


		init_marker_pose = light.get_local_pose(to_matrix=True).squeeze().numpy()

		sim = SimulationManager.get_instance()

		sim = SimulationManager.get_instance()
		marker = sim.draw_marker(

	sim = SimulationManager.get_instance()
	marker = sim.draw_marker(
	marker = SimulationManager.get_instance().draw_marker(

-        light: Light = sim.get_light(uid=light)
+        light_obj = sim.get_light(uid=light)
+        if light_obj is None:
+            return
+        light = light_obj

-        robot = sim.get_robot(uid=robot)
+        robot_uid = robot
+        robot = sim.get_robot(uid=robot_uid)
+        if robot is None:
+            log_error(f"Robot with uid '{robot_uid}' not found; aborting gizmo control loop.")
+            return

-        except:
-            pass
+        except (termios.error, OSError) as exc:
+            log_warning(f"Failed to restore terminal settings: {exc}")

Comments

Conversation

yuecideng commented Feb 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

yuecideng commented Feb 14, 2026 •

edited

Loading