Add height field support to mjx.ray#3378
Open
elliot-at-liminalnook wants to merge 1 commit into
Open
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
b9294b8 to
1fe3796
Compare
`mjx.ray` had no intersection function for HFIELD geoms, so rays silently passed through terrain and reported a miss. Add `_ray_hfield`, a port of `mj_rayHfield`: the base box, the two surface triangles per grid cell, and the four prism side faces with the hit point checked against the interpolated terrain edge. Also warn when candidate geoms have a type with no ray function, so remaining unsupported types (e.g. CYLINDER, SDF) fail loudly instead of being invisible, and update the MJX feature table accordingly. Tests check 9 rays against `mj_ray` bitwise on a bumpy 9x11 height field: straight down, oblique, side-face hits below the terrain edge, base box from below, occlusion by a sphere above the terrain, and misses.
1fe3796 to
742ee2c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mjx.rayhas no intersection function forHFIELDgeoms, so the dispatch loop silently skips height fields: every ray that should hit terrain instead returns(dist=-1, geomid=-1)while the same ray hits withmujoco.mj_ray. Issue #2155 tracked this and was closed once the Warp backend gained hfield rays, but the JAX backend still lacks it.This adds
_ray_hfield, a port ofmj_rayHfield: the base box, the two surface triangles per grid cell, and the four prism side faces, where a side hit only counts if the hit point lies below the terrain edge interpolated between the neighboring data points. It also makesraywarn when a candidate geom has a type with no registered ray function, instead of silently treating it as a miss, and updates the one-line MJX feature-support entry.I want to be upfront that there is an open in-flight PR for the same feature, #2804, which I only found after writing this. I'm posting this as complementary rather than competing. The overlap is the core port; what this adds on top is the silent-miss warning for unsupported geom types, an exact-parity test battery (9 rays checked bitwise against
mj_ray: straight-down and oblique surface hits, side-face hits below the terrain edge, the base box from below, occlusion by a sphere above the terrain, and misses), and the doc-table line. The implementations differ in shape: this one composes per-face candidate distances with a runningminimumand a per-face validity mask, whereas #2804 uses a compiled-for-fixed-size early-out. I'm happy to converge on whichever implementation the maintainers prefer, drop mine in favor of #2804's, or fold these tests and the warning onto #2804; the tests are implementation-agnostic and should transfer either way.Separately, while testing I hit a pre-existing crash unrelated to this change:
mjx.rayraises when the model has no materials (nmat == 0), because the visibility filter indexesmat_rgba[geom_matid]withgeom_matid == -1into an empty array. All the tests here put a material on every geom to avoid it. I'm happy to send a follow-up that guards that filter if useful.I prepared this change with AI assistance (Claude); I have not added it as a commit co-author, since it cannot sign the CLA, and I've reviewed the change and take responsibility for it.