-
Notifications
You must be signed in to change notification settings - Fork 277
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
Physics: Ray intersections #2514
base: gz-sim9
Are you sure you want to change the base?
Physics: Ray intersections #2514
Conversation
815c56b
to
e9c3a9a
Compare
Thanks for the PR! However, as stated in https://community.gazebosim.org/t/feature-freeze-for-gazebo-ionic/2900/2, we are now in feature freeze and will not review/merge any new PRs until after Ionic is released at the end of September. I hope you'll be able to iterate on the PR then. |
Thanks for the message, @azeey . One question: Will all new features be redirected to Ionic, or will we maintain backward compatibility with Harmonic, since the latter is LTS? |
Once Ionic is released, we'll ask folks to target PRs to Ionic branches and the core team will backport to older release on a best-effort basis. Of course, you are welcome to backport it yourself. See https://community.gazebosim.org/t/gazebo-policy-update-new-backporting-policy/2880 for more details. |
I want this function because I want to realize the livox 360 radar simulation. This radar is spiral scanning, and gpu_ray cannot realize this. So when will this function be implemented? @azeey @romulogcerqueira e1161b76-f3eb-4491-bd6c-2a62b4d7aa21.2._an.1.mp4 |
It also can be useful for our use-cases I suppose. I guess this branch should be re-targeted on a last branch |
@romulogcerqueira Will this feature be pushed forward to the official version? |
I installed gazebosim with this repository to use this feature, however when attempting to use MultiRay in my plugin, colcon build cannot find the MultiRay.hh file. (I checked and the file is there, and VSCode has no issues with the import statement.) Do I need to adjust my CMakeLists.txt, or is there something specific I need to do to include the file? My code compiles fine otherwise. |
Hi @azeey, With the release of Gazebo Ionic and the increasing interest from the Gazebo community in this feature (thanks @huqin-RM @ntfshard @AliceCryer), I’d like to move forward with this PR. Could you please advise on the best way to proceed here, given that this PR targets |
Per #2514 (comment), could you target the And thanks for your patience on this. It's on my queue to review, but I haven't had a chance yet. |
e9c3a9a
to
a1471bd
Compare
Signed-off-by: Rômulo Cerqueira <[email protected]>
1a98be5
to
ca55c1b
Compare
Thanks! This PR is now targeting the Since I'm using Harmonic, I'm also interested in the backport of this feature later. |
Signed-off-by: Rômulo Cerqueira <[email protected]>
Signed-off-by: Rômulo Cerqueira <[email protected]>
07a158f
to
9d66d09
Compare
Signed-off-by: Rômulo Cerqueira <[email protected]>
9d66d09
to
55e2798
Compare
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.
This is great work! Thanks again for the contribution. My main concern are:
- Naming of components
- Coordinate frames of quantities in components
- Usability: the need for two components
I would also ask you to add some documentation/tutorial that briefly shows how to use this and mentions that the Bullet collision engine must be used.
}; | ||
|
||
/// \brief A struct that holds the results of raycasting. | ||
struct RayIntersectionInfo |
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.
Can we also include the Entity that was hit?
{ | ||
GZ_PROFILE("PhysicsPrivate::UpdateRayIntersections"); | ||
// Quit early if the MultiRayIntersections component hasn't been created. | ||
// This means there are no systems that need contact information |
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.
// This means there are no systems that need contact information | |
// This means there are no systems that need raycasting information |
auto worldRayIntersectionFeature = | ||
this->entityWorldMap.EntityCast<RayIntersectionFeatureList>(worldEntity); |
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.
You should check if the cast was successful. If the feature isn't available in the physics engine, the pointer will be nullptr
.
}; | ||
|
||
/// \brief A component type that contains multiple rays from an entity. | ||
using MultiRay = |
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.
Does this essentially contain the requested rays to be cast? If so, can we change the name so that it's clear?
|
||
/// \brief A component type that contains the raycasting results from multiple | ||
// rays from an entity into a physics world. | ||
using MultiRayIntersections = |
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 at other raycasting APIs, the work "result" usually appears in the data structure used. The comment here also mentions result, so I'd recommend adding that to the name of the component.
_ecm.Each<components::MultiRay, | ||
components::MultiRayIntersections>( |
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.
Does this mean the user has to create two components on the entity to use this feature? Would it be possible to combine them into one? Or we can have the physics engine be responsible for creating the result component (components::MultiRayIntersections).
/// \brief A struct that holds the information of a ray. | ||
struct RayInfo | ||
{ | ||
/// \brief Starting point of the ray in world coordinates |
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.
If this component is attached to an entity, it's natural to assume the start and end points would be expressed relative to that entity. If the user wants to use world coordinates, they would then just attach the component to the world entity.
struct RayIntersectionInfo | ||
{ | ||
/// \brief The hit point in the world coordinates | ||
gz::math::Vector3d point; |
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.
Similar to my comment above, wouldn't make more sense to express these quantities relative to the frame of the entity this component is attached to?
for (size_t i = 0; i < results1.size(); ++i) { | ||
ASSERT_EQ(results1[i].point, math::Vector3d::Zero); | ||
ASSERT_EQ(results1[i].normal, math::Vector3d(0, 0, 1)); | ||
double exp_fraction = |
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: use const
, use camelCase
style
🎉 New feature
Related to gazebosim/gz-sensors#26 and gazebosim/gz-physics#641
Summary
This PR introduces components to hold rays and ray intersections data (
MultiRay
andMultiRayIntersections
, respectively), and updates the physics system to compute the ray castings duringUpdate
loop.Test it
Added a test case to physics system.
Build and run
./build/bin/INTEGRATION_physics_system --gtest_filter=PhysicsSystemFixture.RayIntersections
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.