[FEATURE] Add get_height_at() and get_normal_at() for terrain entities#2691
[FEATURE] Add get_height_at() and get_normal_at() for terrain entities#2691vlordier wants to merge 15 commits intoGenesis-Embodied-AI:mainfrom
Conversation
- Add history_length parameter to ContactForce sensor options - Override read() to return historical force readings from ring buffer - Update return format to include history dimension - Extend ring buffer size to accommodate history
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d4eaef3cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR adds world-space query helpers on terrain entities to retrieve the terrain height and surface normal at a given (x, y) position, and also introduces a history_length option for the contact force sensor’s output buffering.
Changes:
- Add
RigidEntity.get_height_at(x, y)andRigidEntity.get_normal_at(x, y)for terrain-backed entities. - Add
history_lengthtoContactForcesensor options and adjust sensor buffering to support longer histories. - Add a small terrain query script (
test_terrain.py) demonstrating usage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
genesis/engine/entities/rigid_entity/rigid_entity.py |
Adds terrain height/normal query APIs on RigidEntity. |
genesis/engine/sensors/contact_force.py |
Adds history-aware reads and metadata for contact force sensors. |
genesis/engine/sensors/sensor_manager.py |
Expands ring buffer length calculation to account for history_length. |
genesis/options/sensors/options.py |
Adds history_length option to ContactForce sensor configuration. |
test_terrain.py |
Adds a standalone script demonstrating terrain queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| Get terrain height at world position (x, y). | ||
|
|
||
| Uses bilinear interpolation from the height field. | ||
|
|
| @gs.assert_built | ||
| def get_height_at(self, x: float, y: float) -> float: | ||
| """ | ||
| Get terrain height at world position (x, y). | ||
|
|
||
| Uses bilinear interpolation from the height field. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| x : float | ||
| World x position. | ||
| y : float | ||
| World y position. |
| @gs.assert_built | ||
| def get_height_at(self, x: float, y: float) -> float: | ||
| """ | ||
| Get terrain height at world position (x, y). | ||
|
|
||
| Uses bilinear interpolation from the height field. | ||
|
|
| history_length = self._options.history_length | ||
|
|
||
| buffered_data = self._manager._buffered_data[gs.tc_float] | ||
| cache_slice = slice(self._cache_idx, self._cache_idx + 3) | ||
|
|
||
| if history_length == 1: | ||
| return self._get_formatted_data(self._manager.get_cloned_from_cache(self), envs_idx) | ||
|
|
||
| n_envs = self._manager._sim.n_envs | ||
| history_data = [] | ||
| for i in range(history_length): | ||
| hist = buffered_data.at(i, envs_idx, cache_slice) | ||
| if n_envs == 0: | ||
| hist = hist.reshape(3) | ||
| else: | ||
| hist = hist.reshape(n_envs, 3) | ||
| history_data.append(hist) | ||
|
|
||
| result = torch.stack(history_data, dim=1) | ||
| return result.squeeze(1) if n_envs == 0 else result | ||
|
|
| @gs.assert_built | ||
| def read(self, envs_idx=None) -> torch.Tensor: | ||
| """ | ||
| Read the sensor data (with noise applied if applicable). | ||
| """ | ||
| envs_idx = self._sanitize_envs_idx(envs_idx) | ||
| history_length = self._options.history_length | ||
|
|
| x0 = int(np.floor(x_idx)) | ||
| y0 = int(np.floor(y_idx)) | ||
| x1 = x0 + 1 | ||
| y1 = y0 + 1 | ||
|
|
||
| if x0 < 0 or y0 < 0 or x1 >= hf.shape[1] or y1 >= hf.shape[0]: | ||
| if 0 <= x0 < hf.shape[1] and 0 <= y0 < hf.shape[0]: | ||
| return hf[y0, x0] * v_scale | ||
| return 0.0 | ||
|
|
||
| tx = x_idx - x0 | ||
| ty = y_idx - y0 | ||
|
|
- Fix reshape crash in read() for batched env subsets: use n_query_envs instead of global n_envs when reshaping history slices - Restore read_ground_truth() contract: return noise-free ground truth cache instead of delegating to read() which returns processed data - Fix _draw_debug() env index: use cache[env_idx] instead of hardcoded cache[0] for correct environment in multi-env scenes Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Adds entity.get_height_at(x, y) to query terrain height at world position using bilinear interpolation from height field - Adds entity.get_normal_at(x, y) to compute surface normal at world position from height field gradient - Both methods handle boundary conditions gracefully Closes Genesis-Embodied-AI#2094
…se transform - Fix coordinate indexing: hf[y,x] -> hf[x,y] since heightfield is stored as [row, col] where row corresponds to x - Add pose transformation: convert world coords to terrain local frame using inv_transform_by_trans_quat(terrain_pos, terrain_quat) - Transform normals back to world frame with transform_by_quat - Height now includes terrain z-offset (terrain_pos[2]) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
9d4eaef to
331383b
Compare
|
@claude review |
|
|
||
| # hf is indexed as [row, col] where row corresponds to x and col to y | ||
| if x0 < 0 or y0 < 0 or x1 >= hf.shape[0] or y1 >= hf.shape[1]: | ||
| if 0 <= x0 < hf.shape[0] and 0 <= y0 < hf.shape[1]: | ||
| return hf[x0, y0] * v_scale + terrain_pos[2] | ||
| return terrain_pos[2] | ||
|
|
There was a problem hiding this comment.
🔴 In get_height_at(), when x0 or y0 is in-bounds but x1 or y1 is out-of-bounds (i.e., the query falls in the strip between the last grid node and the terrain boundary), the method returns the non-zero edge height hf[x0, y0]*v_scale + terrain_pos[2] instead of terrain_pos[2]. This contradicts the PR's stated behavior of 'return 0 for height outside terrain'. Remove the inner if 0 <= x0 < hf.shape[0] and 0 <= y0 < hf.shape[1] guard so any position where bilinear interpolation is impossible returns only terrain_pos[2].
Extended reasoning...
Bug: Edge-strip queries return non-zero height instead of 0
The boundary handling block in get_height_at() (rigid_entity.py ~line 4194) reads:
if x0 < 0 or y0 < 0 or x1 >= hf.shape[0] or y1 >= hf.shape[1]:
if 0 <= x0 < hf.shape[0] and 0 <= y0 < hf.shape[1]:
return hf[x0, y0] * v_scale + terrain_pos[2]
return terrain_pos[2]The outer if fires whenever any grid index is out of range — including the common case where x0 is valid but x1 = x0 + 1 is one past the last row. The inner if then tests only whether (x0, y0) are in-bounds (which they are in this case) and returns the edge height. The final return terrain_pos[2] (the correct out-of-bounds value) is only reached when x0 or y0 are themselves out of range.
Concrete proof
Terrain with shape (100, 100), h_scale=0.25:
- Last valid grid index: 99 → last valid world coordinate: 99 × 0.25 = 24.75 m
- Query at x = 24.8 m:
x_idx = 24.8 / 0.25 = 99.2x0 = 99,x1 = 100- Outer condition:
x1 >= hf.shape[0]→ True (enters outer if) - Inner condition:
0 <= x0 < 100→ True (enters inner if) - Returns:
hf[99, y0] * v_scale + terrain_pos[2]← non-zero edge value - Expected per documentation:
terrain_pos[2](height = 0 above terrain base)
Why existing code doesn't prevent it
The PR description explicitly states "Both methods handle boundary conditions (return 0 for height outside terrain)". The docstring is consistent. However, the implementation has a three-tier inconsistency:
(x0, y0)and(x1, y1)both in-bounds → bilinear interpolation (correct)(x0, y0)in-bounds but(x1, y1)out-of-bounds → returns edge height (wrong)(x0, y0)out-of-bounds → returns 0 (correct)
Tier 2 covers the strip x in [(N-1)*h_scale, N*h_scale), which is physically outside the last defined grid cell. A user relying on the documented 0-return contract for out-of-terrain queries will silently get wrong values in this narrow but real strip.
Fix
Remove the inner guard entirely:
if x0 < 0 or y0 < 0 or x1 >= hf.shape[0] or y1 >= hf.shape[1]:
return terrain_pos[2]This ensures every position where bilinear interpolation cannot be performed returns the documented out-of-bounds value.
🔬 also observed by copilot-pull-request-reviewer
…force sensor Terrain get_height_at / get_normal_at: - Remove inner edge-strip guard that returned non-zero height for positions just beyond the last grid node (x slightly > (N-1)*h_scale); now returns terrain_pos[2] consistently for all out-of-bounds positions - Switch from bilinear to triangle-based (barycentric) interpolation matching the physics mesh diagonal convention in convert_heightfield_to_watertight_trimesh: cell split along (x0,y0)-(x1,y1) diagonal, upper-left triangle when tx<=ty, lower-right when tx>ty; ensures queried heights/normals lie on simulated surface ContactForceSensor read() / read_ground_truth(): - Fix history tensor stacking: use dim=0 for non-batched (n_envs==0) to produce (history_length, 3) instead of (3, history_length); use dim=1 for batched to produce (n_envs, history_length, 3) — previous code transposed the result - Remove dead `if envs_idx is None` branch after _sanitize_envs_idx() which always returns a tensor - Clarify docstring: history buffer stores ground-truth values since the underlying ring buffer is written before noise/delay post-processing
…istory Terrain get_height_at / get_normal_at (test_rigid_physics.py): - test_terrain_get_height_flat: flat HF returns constant height+offset everywhere, normal is world-up - test_terrain_get_height_ramp_x: parametrized over 3 (shape, scale) combos; verifies linear height and correct slope direction for pure x-ramp - test_terrain_get_height_non_symmetric: non-square HF (10x20) with distinct slopes on each axis catches axis-swap bugs; includes analytic reference - test_terrain_get_height_triangle_consistency: at the diagonal (tx==ty==0.5) both triangle formulas must agree - test_terrain_get_height_out_of_bounds: 6 cases covering negative coords, positions just beyond last node (edge-strip), and far-outside; all must return pos_z, not edge height - test_terrain_get_height_pose_transform: parametrized over translated, yaw-45, and combined poses; world-space query of a known vertex must recover its height - test_terrain_get_normal_unit_length_and_direction: unit length + z>0 on random HF - test_terrain_triangle_selection_both_halves: direct validation of upper-left and lower-right triangle formulas using known corner heights; checks normals differ between triangles ContactForce history_length (test_sensors.py): - test_contact_force_history_length_1_shape: default history produces (3,) / (n,3) - test_contact_force_history_shape: history_length 2 and 5, n_envs 0 and 2; verifies both read() and read_ground_truth() shapes - test_contact_force_history_no_transpose: index [0] is most-recent, catches the dim=1 stacking bug (3, L) vs correct (L, 3) - test_contact_force_history_envs_idx_subset: requesting a 1-env subset of a batched scene must not raise and must return (1, L, 3) - test_contact_force_history_length_1_ground_truth_is_noiseless: with noise=5.0, read_ground_truth() must match physics; read() must differ - test_contact_force_history_before_and_after_contact: both history_length 1 and 3; GT force is zero before contact and non-zero after
rigid_entity.py — get_height_at / get_normal_at: - Add env_idx: int | None = None parameter to both methods; pass through to links[0].get_pos(env_idx).reshape((3,)) and get_quat(env_idx).reshape((4,)) so batched scenes can specify which env to query; reshape handles both the non-batched (3,) and batched (1,3) return shapes from get_pos/get_quat contact_force.py: - read_ground_truth(): move get_cloned_from_cache(is_ground_truth=True) clone inside the history_length==1 branch — was unconditionally allocating a GPU tensor even when history_length>1 where it was never used - Remove ContactForceSensorMetadata.history_length field: was updated in build() but read() / read_ground_truth() always use self._options.history_length directly, making the metadata field dead code - Remove corresponding self._shared_metadata.history_length update in build() tests/test_rigid_physics.py: - test_terrain_get_height_out_of_bounds: replace wrong edge-strip case (terrain_x_max - 0.01) which is actually IN bounds (x0=n_rows-2, x1=n_rows-1 are both valid) with (terrain_x_max, 0.5) which correctly triggers OOB (x0=n_rows-1, x1=n_rows >= hf.shape[0]); previous case would have failed - test_terrain_get_height_non_symmetric: remove dead variables h_swapped_x and h_swapped_y (same expression assigned twice, never read)
Dead import left after prior refactoring passes; cleaned up as part of ongoing PR review cycle.
- contact_force.py: quote the Mesh annotation (TYPE_CHECKING-only import) was `Mesh | None` (unquoted, NameError at runtime), now `"Mesh | None"` - sensor_manager.py: ring buffer must be delay_ts+2 deep for sensors with interpolate=True, because _apply_delay_to_shared_cache reads index delay_ts+1 during linear interpolation; previously delay_ts+1 caused modulo wrap-around returning stale data for the right interpolation point
_read_history() reads from _buffered_data (the ring buffer), but SensorManager skips _update_shared_cache (which writes to the ring buffer) when update_ground_truth_only=True for all sensors of a type. Combined with history_length > 1, this caused _read_history() to return uninitialised/stale data. Force _update_shared_cache when any sensor in the group has history_length > 1, ensuring the ring buffer is always populated. Note: _update_shared_cache writes ground truth to the ring buffer first, so _read_history() always returns ground-truth data regardless of noise.
duburcqa
left a comment
There was a problem hiding this comment.
This PR contains tons of unrelated changes.
Summary
get_height_at(x, y)method to query interpolated terrain height at world positionget_normal_at(x, y)method to compute surface normal at world positionChanges
genesis/engine/entities/rigid_entity/rigid_entity.py: Added two methods to RigidEntity class:get_height_at(x, y)- Returns interpolated height using bilinear sampling from terrain height fieldget_normal_at(x, y)- Returns unit normal vector computed from height field gradient@gs.assert_builtdecorator and include detailed docstringsUsage
Backward Compatible
Closes #2094