Parse category bitmask SDF element in Surface DOM#1630
Conversation
Signed-off-by: Ian Chen <ichen@openrobotics.org>
Signed-off-by: Ian Chen <ichen@openrobotics.org>
ahcorde
left a comment
There was a problem hiding this comment.
Minor fix, Otherwise LGTM
Signed-off-by: Ian Chen <ichen@openrobotics.org>
|
|
||
| /// \brief Get the category bitmask parameter. | ||
| /// \return The category bitmask parameter. | ||
| public: std::optional<uint16_t> CategoryBitmask() const; |
There was a problem hiding this comment.
does it make sense to return a const reference?
| public: std::optional<uint16_t> CategoryBitmask() const; | |
| public: const std::optional<uint16_t> &CategoryBitmask() const; |
There was a problem hiding this comment.
after doing some reading online and asking AI, it seems that for simple primitive types like uint16_t it's more efficient to just return by value, i.e.std::optional<uint16_t> instead of const ref.
I do see some places in sdformat that returns const std::optional & (there are also other places that just return by value), e.g.
sdformat/include/sdf/InterfaceElements.hh
Line 102 in dcd3c2d
so I'm ok to match the style for consistency or clarity reasons.
There was a problem hiding this comment.
CPP core guideline F.15 suggests preferring simple and conventional ways of passing information, and the example indicate returning by value rather than a const reference. In the case where data is expensive to move, it recommends passing an input/output reference to the function
so I think you can ignore my earlier suggestion and keep it as is
I remember seeing lint warnings recommending more use of const references and I took it a little farther than necessary
|
oh I just remembered, consider updating the python bindings |
Signed-off-by: Ian Chen <ichen@openrobotics.org>
ah yes, thanks for the reminder. Just added. a1bbbb4 |
🎉 New feature
Summary
Parses the
category_bitmaskSDF element:https://sdformat.org/spec/1.12/collision/#contact_category_bitmask
In short, collision occurs if
(category_bitmask1 & collide_bitmask2) | (category_bitmask2 & collide_bitmask1)evaluates to non-zero.
Currently in gazebo, collision occurs if
(collide_bitmask1 & collide_bitmask2)evaluates to non-zero. To preserve backward compatibility, here's a proposed implementation (to be implemented in gz-physics):collide_bitmaskis set andcategory_bitmaskis not set, thencategory_bitmaskis set to the same value ascollide_bitmaskExample 1:
collide_bitmaskis set butcategory_bitmaskis not for both collision entities (existing behavior in gz-physics):collide_bitmask1: 0x1category_bitmask1: (not set, so defaults to 0x1 )collide_bitmask2: 0x2category_bitmask2: (not set, so defaults to 0x2 )Result of
(category_bitmask1 & collide_bitmask2) | (category_bitmask2 & collide_bitmask1): No collision. Preserves existing behavior.Example 2:
collide_bitmaskandcategory_bitmaskare set for both collision entitiescollide_bitmask1: 0x1category_bitmask1:0x2collide_bitmask2: 0x2category_bitmask2: 0x1Result of
(category_bitmask1 & collide_bitmask2) | (category_bitmask2 & collide_bitmask1): Collision occurs!Example 3:
collide_bitmaskandcategory_bitmaskare set for one collision entity while onlycollide_bitmaskis set for the other collision entitycollide_bitmask1: 0x1category_bitmask1: (not set, so defaults to 0x1)collide_bitmask2: 0x2category_bitmask2: 0x1Result of
(category_bitmask1 & collide_bitmask2) | (category_bitmask2 & collide_bitmask1): Collision occurs!The category bitmask field in Surface DOM uses
std::optionaltype so we are able to know if the value is set or not.Checklist
codecheckpassed (See contributing)Generated-by: Remove this if GenAI was not used.
Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-byandGenerated-bymessages.Backports: If this is a backport, please use Rebase and Merge instead.