RSDK-13982 - construct model table from urdf#645
Conversation
nfranczak
requested review from
lia-viam and
njooma
and removed request for
a team
|
Mentioned this in the previous PR but I'm not convinced we need Eigen? I would be in favor of reusing |
lia-viam
left a comment
lia-viam
left a comment
There was a problem hiding this comment.
lgtm, try running bin/run_clang_format.sh if linter still isn't passing
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
This looks good, I have some comments to consider though.
| self.requires(self._grpc_requires()) | ||
| self.requires('protobuf/[>=3.17.1 <6.30.0]') | ||
| self.requires(self._xtensor_requires(), transitive_headers=True) | ||
| self.requires('eigen/[>=3.3.0]', transitive_headers=True) |
There was a problem hiding this comment.
Is this still true? Ditto below?
| inline bool operator==(const Vector3& a, const Vector3& b) { | ||
| return a.data == b.data; | ||
| } | ||
| inline bool operator!=(const Vector3& a, const Vector3& b) { |
There was a problem hiding this comment.
I'm surprised the formatter didn't do it for you, but please throw a blank line inbetween these definitions.
| namespace sdk { | ||
|
|
||
| /// @brief URDF joint type, restricted to arm-relevant joints. | ||
| enum class JointType { revolute, continuous, prismatic, fixed }; |
There was a problem hiding this comment.
These should be k_revolute, etc.
|
|
||
| /// @brief Convert a model table to a double tensor of shape (n, 10). | ||
| /// Columns: 0..2 xyz, 3..5 rpy, 6..8 axis, 9 joint type as | ||
| /// underlying enum value (0=revolute, 1=continuous, 2=prismatic, 3=fixed). |
There was a problem hiding this comment.
If you want to do that, I'd suggest giving the JointType enum values matching explicit values.
|
|
||
| /// @brief One row of the model table: the per-joint URDF fields. | ||
| /// @note `xyz`/`rpy`/`axis` are taken directly from the URDF. | ||
| struct JointRow { |
There was a problem hiding this comment.
You could put JointType in here, so you had JointRow::[T|t]ype::k_[revolute,continuous,prismatic,fixed].
|
|
||
| namespace viam { | ||
| namespace sdk { | ||
| namespace urdf_model_table_internals { |
There was a problem hiding this comment.
Elsewhere, where we have had private headers, we have gathered the inner namespace as impl (we can't call it private since that is a keyword). It might be better to stick to that convention.
| if (row.type == JointType::fixed) { | ||
| row.axis = Vector3{0, 0, 0}; | ||
| } else if (parsed.axis_opt) { | ||
| if (magnitude(*parsed.axis_opt) < 1e-12) { |
There was a problem hiding this comment.
Why this value and not some other?
| for (std::size_t i = 0; i < tensor.shape()[0]; ++i) { | ||
| const double type_v = tensor(i, 9); | ||
| const int type_i = static_cast<int>(type_v); | ||
| if (static_cast<double>(type_i) != type_v || type_i < 0 || type_i > 3) { |
There was a problem hiding this comment.
Probably want to compare against the enum values here, not constants.
…0514-RSDK-13982-construct-model-table
nfranczak
force-pushed
the
20250514-RSDK-13982-construct-model-table
branch
from
60f7024 to
57560da
Compare
3 participants
as follow up will do sva