-
Notifications
You must be signed in to change notification settings - Fork 109
Add ray with normal #940
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
Add ray with normal #940
Conversation
|
thanks @StafaH! |
erikfrey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Just a few drive-by christmas nits.
mujoco_warp/_src/ray.py
Outdated
| x = all[i] | ||
|
|
||
| return x | ||
| normal_local = wp.vec3(0.0, 0.0, 0.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about:
normal_local = wp.vec3(float(i == 1) - float(i == 0), float(i == 3) - float(i == 2), 0.0)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a very nifty one-liner! Beautiful. Replaced.
mujoco_warp/_src/ray.py
Outdated
| or (planar_01 < 0.0 and planar_11 < 0.0 and planar_21 < 0.0) | ||
| ): | ||
| return float(wp.inf) | ||
| return float(wp.inf), wp.vec3(0.0, 0.0, 0.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: wp.vec3() already inits to zeros
(comment applies here and elsewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
mujoco_warp/_src/ray.py
Outdated
| def _ray_triangle( | ||
| v0: wp.vec3, v1: wp.vec3, v2: wp.vec3, pnt: wp.vec3, vec: wp.vec3, b0: wp.vec3, b1: wp.vec3 | ||
| ) -> Tuple[float, wp.vec3]: | ||
| """Returns the distance at which a ray intersects with a triangle.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's update the docstring to mention that the normal (?) is also returned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
mujoco_warp/_src/ray.py
Outdated
| @wp.func | ||
| def _ray_plane(pos: wp.vec3, mat: wp.mat33, size: wp.vec3, pnt: wp.vec3, vec: wp.vec3) -> float: | ||
| def _ray_plane(pos: wp.vec3, mat: wp.mat33, size: wp.vec3, pnt: wp.vec3, vec: wp.vec3) -> Tuple[float, wp.vec3]: | ||
| """Returns the distance at which a ray intersects with a plane.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's update the docstring to mention that the normal is returned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
mujoco_warp/_src/ray.py
Outdated
| @wp.func | ||
| def _ray_sphere(pos: wp.vec3, dist_sqr: float, pnt: wp.vec3, vec: wp.vec3) -> float: | ||
| def _ray_sphere(pos: wp.vec3, dist_sqr: float, pnt: wp.vec3, vec: wp.vec3) -> Tuple[float, wp.vec3]: | ||
| """Returns the distance at which a ray intersects with a sphere.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
thowell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a few more comments. otherwise lgtm.
great work and thank you for this contribution @StafaH!
| normal_np = normal.numpy()[0, 0] | ||
| _assert_eq(geomid_np, -1, "geom_id") | ||
| _assert_eq(dist_np, -1, "dist") | ||
| _assert_eq(normal_np, wp.vec3(0.0, 0.0, 0.0), "normal") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wp.vec3 -> 0.0
Modify ray functions to return normal information as well.
Follows the C MuJoCo implementation: https://github.com/google-deepmind/mujoco/blob/main/src/engine/engine_ray.c
Currently waiting on the C MuJoCo mju_rayNormal to become enabled, which will allow ray_test to compare normals with the engine implementation.