[FEATURE] Contact Sensor with Kinematic Probe#2292
[FEATURE] Contact Sensor with Kinematic Probe#2292YilingQiao wants to merge 9 commits intoGenesis-Embodied-AI:mainfrom
Conversation
|
🔴 Benchmark Regression Detected ➡️ Report |
|
|
| probe_positions_local=shared_metadata.probe_positions.contiguous(), | ||
| probe_normals_local=shared_metadata.probe_normals.contiguous(), | ||
| probe_sensor_idx=shared_metadata.probe_sensor_idx.contiguous(), | ||
| links_pos=links_pos.contiguous(), | ||
| links_quat=links_quat.contiguous(), | ||
| radii=shared_metadata.radii.contiguous(), | ||
| stiffness=shared_metadata.stiffness.contiguous(), | ||
| links_idx=shared_metadata.links_idx.contiguous(), | ||
| contypes=shared_metadata.contypes.contiguous(), | ||
| conaffinities=shared_metadata.conaffinities.contiguous(), | ||
| n_probes_per_sensor=shared_metadata.n_probes_per_sensor.contiguous(), | ||
| sensor_cache_start=shared_metadata.sensor_cache_start.contiguous(), | ||
| sensor_probe_start=shared_metadata.sensor_probe_start.contiguous(), |
There was a problem hiding this comment.
Remove continuous(). If this is currently necessary, the implementation should be refactored so that it is no longer necessary but applying transpose and changing the expected memory layout in the kernel.
| constraint_state=solver.constraint_solver.constraint_state, | ||
| equalities_info=solver.equalities_info, | ||
| support_field_info=solver.collider._support_field._support_field_info, | ||
| output=shared_ground_truth_cache.contiguous(), |
There was a problem hiding this comment.
Same. Find a way to remove contiguous.
| links_pos = solver.get_links_pos(links_idx=shared_metadata.links_idx) | ||
| links_quat = solver.get_links_quat(links_idx=shared_metadata.links_idx) | ||
|
|
||
| if solver.n_envs == 0: | ||
| links_pos = links_pos[None] | ||
| links_quat = links_quat[None] | ||
|
|
||
| links_pos = links_pos.reshape(B, n_sensors, 3) | ||
| links_quat = links_quat.reshape(B, n_sensors, 4) |
There was a problem hiding this comment.
Using these public getters is not very efficient. I would recommend passing shared_metadata.links_idx to _kernel_kinematic_contact_probe_support_query, along with solver.links_state.
| ) | ||
| self._shared_metadata.contypes = concat_with_tensor( | ||
| self._shared_metadata.contypes, | ||
| torch.tensor([self._options.contype], dtype=torch.int32, device=gs.device), |
There was a problem hiding this comment.
self._options.contype is good enough, no need to cast as tensor. This will allow to keep everything on one line and focus on what matters.
| ) | ||
| self._shared_metadata.conaffinities = concat_with_tensor( | ||
| self._shared_metadata.conaffinities, | ||
| torch.tensor([self._options.conaffinity], dtype=torch.int32, device=gs.device), |
There was a problem hiding this comment.
Same. No need to cast as tensor.
| contypes: torch.Tensor = make_tensor_field((0,), dtype_factory=lambda: torch.int32) | ||
| conaffinities: torch.Tensor = make_tensor_field((0,), dtype_factory=lambda: torch.int32) | ||
|
|
||
| probe_sensor_idx: torch.Tensor = make_tensor_field((0,), dtype_factory=lambda: torch.int32) | ||
| probe_positions: torch.Tensor = make_tensor_field((0, 3)) | ||
| probe_normals: torch.Tensor = make_tensor_field((0, 3)) | ||
|
|
||
| n_probes_per_sensor: torch.Tensor = make_tensor_field((0,), dtype_factory=lambda: torch.int32) | ||
| sensor_cache_start: torch.Tensor = make_tensor_field((0,), dtype_factory=lambda: torch.int32) | ||
| sensor_probe_start: torch.Tensor = make_tensor_field((0,), dtype_factory=lambda: torch.int32) |
There was a problem hiding this comment.
I'm replacing dtype_factory=lambda: [...] in favour of simply dtype=[...], which is now possible because sensors cannot be imported before initializing Genesis anymore.
| self._n_probes = sensor_options.n_probes | ||
| super().__init__(sensor_options, sensor_idx, data_cls, sensor_manager) |
There was a problem hiding this comment.
Skip one line between defining derived class attributes and calling base init.
|
|
||
| self._shared_metadata.n_probes_per_sensor = concat_with_tensor( | ||
| self._shared_metadata.n_probes_per_sensor, | ||
| torch.tensor([n_probes], dtype=torch.int32, device=gs.device), |
There was a problem hiding this comment.
Casting to tensor manually is not necessary, and enforcing 1D / sequence is not necessary either.
Just do:
self._shared_metadata.n_probes_per_sensor = concat_with_tensor(
self._shared_metadata.n_probes_per_sensor, n_probes, expand=(1,), dim=0
)| current_cache_start = sum(self._shared_metadata.cache_sizes[:-1]) if self._shared_metadata.cache_sizes else 0 | ||
| self._shared_metadata.sensor_cache_start = concat_with_tensor( | ||
| self._shared_metadata.sensor_cache_start, | ||
| torch.tensor([current_cache_start], dtype=torch.int32, device=gs.device), |
| current_probe_start = self._shared_metadata.total_n_probes | ||
| self._shared_metadata.sensor_probe_start = concat_with_tensor( | ||
| self._shared_metadata.sensor_probe_start, | ||
| torch.tensor([current_probe_start], dtype=torch.int32, device=gs.device), |
| positions_tensor = torch.tensor(probe_positions, dtype=gs.tc_float, device=gs.device) | ||
| self._shared_metadata.probe_positions = torch.cat( | ||
| [self._shared_metadata.probe_positions, positions_tensor], dim=0 | ||
| ) |
There was a problem hiding this comment.
Why are you concatenating manually instead of using our dedicated utility for this?
| normals_tensor = torch.tensor(probe_normals, dtype=gs.tc_float, device=gs.device) | ||
| norms = normals_tensor.norm(dim=1, keepdim=True).clamp(min=1e-8) | ||
| normals_tensor = normals_tensor / norms | ||
| self._shared_metadata.probe_normals = torch.cat([self._shared_metadata.probe_normals, normals_tensor], dim=0) |
There was a problem hiding this comment.
Why are you concatenating manually instead of using our dedicated utility for this?
| if not self._manager._has_delay_configured(type(self)): | ||
| self._manager.update_sensor_type_cache(type(self)) |
There was a problem hiding this comment.
Don't do this. You are abusing the software design. It is the responsibility of the manager to determine whether to update a given sensor type or not.
There was a problem hiding this comment.
Move all these changes outside of this PR. It is completely unrelated and requires further discussion. First, we need to make sure this lazy-update thing is really necessary, next we need to implement this feature in a way that does not delegate more responsibility that expected to sensors.
| if geom_type == gs.GEOM_TYPE.PLANE: | ||
| g_pos = geoms_state.pos[i_g, i_b] | ||
| g_quat = geoms_state.quat[i_g, i_b] | ||
| geom_data = geoms_info.data[i_g] | ||
| plane_normal_local = gs.ti_vec3([geom_data[0], geom_data[1], geom_data[2]]) | ||
| plane_normal_world = gu.ti_transform_by_quat(plane_normal_local, g_quat) | ||
| dist_to_plane = (probe_pos - g_pos).dot(plane_normal_world) | ||
| support_pos = probe_pos - dist_to_plane * plane_normal_world |
There was a problem hiding this comment.
Why are you handling if geom_type == gs.GEOM_TYPE.PLANE: separately from other cases defined in _func_support_point_for_probe ? I don't say why they could not be unified. Just rename the function if you think it is no longer appropriate.
| @ti.func | ||
| def _func_point_in_expanded_aabb( | ||
| i_g: ti.i32, | ||
| i_b: ti.i32, | ||
| geoms_state: array_class.GeomsState, | ||
| point: ti.types.vector(3, gs.ti_float), | ||
| expansion: gs.ti_float, | ||
| ): | ||
| aabb_min = geoms_state.aabb_min[i_g, i_b] - expansion | ||
| aabb_max = geoms_state.aabb_max[i_g, i_b] + expansion | ||
| return (point >= aabb_min).all() and (point <= aabb_max).all() |
There was a problem hiding this comment.
We already have a function for this, just import it from rigid solver, you are allowed to do this.
There was a problem hiding this comment.
this is a specific variants of aabb only used in this sensor, we don't need to accommodate this function in the main code?
| @ti.func | ||
| def _func_check_collision_filter( | ||
| i_g: ti.i32, | ||
| i_b: ti.i32, | ||
| sensor_link_idx: ti.i32, | ||
| sensor_contype: ti.i32, | ||
| sensor_conaffinity: ti.i32, | ||
| geoms_info: array_class.GeomsInfo, | ||
| rigid_global_info: array_class.RigidGlobalInfo, | ||
| constraint_state: array_class.ConstraintState, | ||
| equalities_info: array_class.EqualitiesInfo, | ||
| ): |
There was a problem hiding this comment.
We don't already have a function that is doing exactly this? If we do, just import it directly instead of copy-pasting it here.
There was a problem hiding this comment.
this part introduced affinity for sensor, and only used in this sensor. We don't need this plugin to affected the main physics solver code
| @property | ||
| def n_probes(self) -> int: | ||
| """Return the number of probes defined for this sensor.""" | ||
| return len(self.probe_local_pos) | ||
|
|
||
| def get_probe_positions(self) -> list[Tuple3FType]: | ||
| """Return probe positions as a list of tuples.""" | ||
| return [tuple(pos) for pos in self.probe_local_pos] | ||
|
|
||
| def get_probe_normals(self) -> list[Tuple3FType]: | ||
| """Return probe normals as a list of tuples.""" | ||
| return [tuple(normal) for normal in self.probe_local_normal] |
There was a problem hiding this comment.
I don't think this should be part of the options.
| scene.build(n_envs=0) | ||
| scene.step() | ||
|
|
||
| assert probe.read().penetration[0] > 0.0 | ||
|
|
||
| mount.set_pos(torch.tensor([0.0, 0.0, 2.0], dtype=gs.tc_float)) | ||
| scene.step() | ||
| assert probe.read().penetration[0] == 0.0 |
There was a problem hiding this comment.
This is clearly not sufficient. You should check that the result is analytically valid on a synthetic scenario that is simple enough to do so.
There was a problem hiding this comment.
I just noticed your example script. I think it is perfectly suitable for analytical validation already. You need to convert this in a unit test and do more comprehensive validation of the sensor measurement.
|
|
||
| assert probe.read().penetration[0] > 0.0 | ||
|
|
||
| mount.set_pos(torch.tensor([0.0, 0.0, 2.0], dtype=gs.tc_float)) |
There was a problem hiding this comment.
You can just do mount.set_pos([0.0, 0.0, 2.0]). No need to force casting to tensor. It just makes what you are doing harder to read.
|
|
||
| scene.add_entity(gs.morphs.Box(size=(0.5, 0.5, 0.5), pos=(0.0, 0.0, 0.25), fixed=True)) | ||
|
|
||
| mount = scene.add_entity(gs.morphs.Sphere(radius=0.02, pos=(0.25, 0.25, 0.06), fixed=True, collision=False)) |
There was a problem hiding this comment.
Use multiple lines systematically for scene declaration with very few exceptions. And no need to skip lines between the declaration of each entity.
| def test_kinematic_contact_probe_box_support(show_viewer, tol): | ||
| """Test BOX geometry detection via support function.""" | ||
| scene = gs.Scene( | ||
| sim_options=gs.options.SimOptions(dt=1e-2, substeps=1, gravity=(0.0, 0.0, 0.0)), |
There was a problem hiding this comment.
One line per argument. Moreover, remove them if they are not absolutely necessary for your test.
| assert probe.read().penetration[0] > 0.0 | ||
|
|
||
| mount.set_pos(torch.tensor([0.0, 0.0, 2.0], dtype=gs.tc_float)) | ||
| scene.step() | ||
| assert probe.read().penetration[0] == 0.0 |
There was a problem hiding this comment.
Just (vaguely) checking penetration is not good enough. What about all the other outputs?! See what we did for the IMU sensor, it is much more comprehensive.
It's for resolving this request as in the PR description. |
Description
Add KinematicContactProbe sensor for contact detection without physics side effects.
probe_local_posandprobe_local_normallistsread(), not every step (unless delay configured)To solve #2254
How It Works
Algorithm for each probe:
probe_posandprobe_normalfrom link-local to world framesupport_pos = support(-probe_normal)distance(support_pos, probe_pos) <= radius, compute:penetration = dot(probe_pos - support_pos, probe_normal)Force Output (Not Physical)
The returned
force = stiffness * penetration * probe_normalis a user-defined estimate, not derived from the physics solver (because this probe is only at kinematic level). Genesis is not a mass-spring model. Thestiffnessparameter is for convenience to convert penetration into a force-like quantity.Return Values
sensor.read()returns aKinematicContactProbeDataNamedTuple:penetration(n_envs, n_probes)or(n_probes,)position(n_envs, n_probes, 3)or(n_probes, 3)normal(n_envs, n_probes, 3)or(n_probes, 3)probe_local_normalwhen contact detected, zero otherwise.force(n_envs, n_probes, 3)or(n_probes, 3)stiffness * penetration * probe_normal(NOT physical).Try
kinematic_probe.mp4
Test plan
Checklist:
Submitting Code Changessection of CONTRIBUTING document.