RSDK-13982 - construct model table from urdf by nfranczak · Pull Request #645 · viamrobotics/viam-cpp-sdk

nfranczak · 2026-05-20T15:41:19Z

as follow up will do sva

lia-viam · 2026-05-21T21:44:33Z

Mentioned this in the previous PR but I'm not convinced we need Eigen? I would be in favor of reusing Vector3 from linear_algebra.hpp or an appropriate xtensor type

lia-viam

lgtm, try running bin/run_clang_format.sh if linter still isn't passing

acmorrow

This looks good, I have some comments to consider though.

acmorrow · 2026-05-28T15:50:25Z

        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)


Is this still true? Ditto below?

acmorrow · 2026-05-28T15:56:26Z

+
+/// @brief One row of the model table: the per-joint URDF fields.
+/// @note `xyz`/`rpy`/`axis` are taken directly from the URDF.
+struct JointRow {


You could put JointType in here, so you had JointRow::[T|t]ype::k_[revolute,continuous,prismatic,fixed].

I am not a fan of this.
It feels like there is less cognitive overhead with keep it this way.
edit: and with your proposal the jointtype is hidden one level deeper right?

I think it comes down to this question: Is JointType something intrinsically useful, or is it just a discriminator to understand how to interpret the fields of JointRow. If it is intrinsically valuable, then I'm more OK with making it a top-level type on its own. If it is just the descriminator for JointRow, I'd be in favor of nesting it.

FYI now that they are both at least in ModelTable I'm less inclined to push JointType into JointRow though I do think the argument holds.

I'm resisting the urge to say that JointRow should be some sort of variant, because if I see it correctly, the only real structural variant is for the fixed case.

acmorrow · 2026-05-28T16:04:28Z

+    if (row.type == JointType::fixed) {
+        row.axis = Vector3{0, 0, 0};
+    } else if (parsed.axis_opt) {
+        if (magnitude(*parsed.axis_opt) < 1e-12) {


Why this value and not some other?

…0514-RSDK-13982-construct-model-table

acmorrow

Similar thoughts as the UR Arm review about pushing harder to make these classes, rather than structs with helper functions.

acmorrow · 2026-06-01T16:15:26Z

+    k_prismatic = 2,
+    k_fixed = 3,
+};
+


Are there any connections between the set of possible values of these vectors, and the types of joints?

You could consider adding static factory methods on JointRow, like JointRow::fixed(std::string name, ...).

xyz and rpy just tell us the position and orientation of "things", eg. links.
axis is our biggest hint here.
If it's (0, 0, 0) then we can deduce that it's a fixed joint with certainty.
But on the other hand if it's a non-zero unit vector we do not have a way of knowing if the joint type is revolute, continuous or prismatic.

acmorrow · 2026-06-01T16:15:39Z

+
+/// @brief One row of the model table: the per-joint URDF fields.
+/// @note `xyz`/`rpy`/`axis` are taken directly from the URDF.
+struct JointRow {


acmorrow · 2026-06-01T16:30:15Z

@@ -0,0 +1,43 @@
+/// @file referenceframe/private/kinematics_model_table_internals.hpp


I don't think you should doc comment it if it is under private, since otherwise doxygen docs will I think end up picking it up, even though consumers of the SDK can't use it.

You still ahve the rest of this file doc commented though.

It should be easy, just change /// to //

Sorry a bit confused by this: in the current version of the code I just removed all the lines which were prefixed with : ///

… be un-ordered

…m:nfranczak/viam-cpp-sdk into 20250514-RSDK-13982-construct-model-table

acmorrow

LGTM with a few lingering comments. @lia-viam can you please also take a look?

acmorrow · 2026-06-02T11:21:49Z

@@ -0,0 +1,43 @@
+/// @file referenceframe/private/kinematics_model_table_internals.hpp


You still ahve the rest of this file doc commented though.

It should be easy, just change /// to //

acmorrow · 2026-06-02T11:24:31Z

+
+/// @brief One row of the model table: the per-joint URDF fields.
+/// @note `xyz`/`rpy`/`axis` are taken directly from the URDF.
+struct JointRow {


FYI now that they are both at least in ModelTable I'm less inclined to push JointType into JointRow though I do think the argument holds.

I'm resisting the urge to say that JointRow should be some sort of variant, because if I see it correctly, the only real structural variant is for the fixed case.

acmorrow · 2026-06-02T11:27:49Z

+    /// @brief URDF joint type, restricted to arm-relevant joints.
+    /// @note Underlying values are stable and form the wire encoding for
+    /// column 9 of the tensor produced by `to_tensor()`.
+    enum class JointType {


I suspect the linter is going to want you to give this a smaller integral type.

A bit confused by this, the linter did not complain.

OK, maybe we don't do that anymore.

…0514-RSDK-13982-construct-model-table

lia-viam

mostly minor/optional stuff but still lgtm

lia-viam · 2026-06-02T15:10:08Z

+std::vector<ParsedJoint> parse_urdf(const KinematicsDataURDF& urdf) {
+    namespace pt = boost::property_tree;
+    pt::ptree tree;
+    const std::string text(reinterpret_cast<const char*>(urdf.bytes.data()), urdf.bytes.size());


we have bytes_to_string for this

lia-viam · 2026-06-02T15:14:47Z

+    row.xyz = parsed.xyz;
+    row.rpy = parsed.rpy;
+
+    if (parsed.type_str == "revolute") {


I wonder if we want a public to_string or joint_type_from_string ?

I am not convinced that we need a public to_string or joint_type_from_string.
My reasoning is that users always get a parsed JointType values values via from(urdf) / from(tensor) and never see the strings.
The to_string has no user anywhere in this flow.

lia-viam · 2026-06-02T15:37:20Z

+    xt::xarray<double> out = xt::zeros<double>({rows_.size(), std::size_t{10}});
+    for (std::size_t i = 0; i < rows_.size(); ++i) {
+        const auto& r = rows_[i];
+        out(i, 0) = r.xyz.x();


I don't love this but struggling to come up with an alternative

int idx = 0; for (const auto& vec3 : {std::cref(r.xyz), std::cref(r.rpy), std::cref(r.axis)}) { for (const auto& val : arr.data) { out(i, idx++) = val; } }

honestly not sure if this is preferable, your call

while more compact/abstract, I think that what we have now is slightly clearer to read.

in that case maybe just a line break between the blocks where you're operating on the different vec3's

acmorrow · 2026-06-02T19:07:42Z

+    /// @brief URDF joint type, restricted to arm-relevant joints.
+    /// @note Underlying values are stable and form the wire encoding for
+    /// column 9 of the tensor produced by `to_tensor()`.
+    enum class JointType {


OK, maybe we don't do that anymore.

acmorrow · 2026-06-02T19:08:03Z

+namespace sdk {
+namespace impl {
+
+/// @brief Per-joint parse result, pre-validation.


Here, for instance, there is still a doxygen style comment on an internal header.

ok I can change.
does that mean that this for example is incorrect?
https://github.com/viamrobotics/viam-cpp-sdk/blob/main/src/viam/sdk/rpc/private/viam_grpc_channel.hpp#L14

urdf to model table, model table to tensor, tensor to model table

d987e67

nfranczak requested a review from a team as a code owner May 20, 2026 15:41

nfranczak requested review from lia-viam and njooma and removed request for a team May 20, 2026 15:41

Merge branch 'main' into 20250514-RSDK-13982-construct-model-table

d64711b

nfranczak added 2 commits May 26, 2026 11:14

no more eigen

79a4d08

should pass CI now

2404c5a

lia-viam reviewed May 26, 2026

View reviewed changes

nfranczak and others added 2 commits May 26, 2026 15:37

liiiint

96d2b60

Merge branch 'main' into 20250514-RSDK-13982-construct-model-table

1ff3322

nfranczak requested a review from acmorrow May 26, 2026 19:39

lets try now?

35be728

nfranczak mentioned this pull request May 27, 2026

RSDK-14030 - update to cpp-sdk v37 viam-modules/trajex#16

Merged

rename

7132201

nfranczak mentioned this pull request May 27, 2026

RSDK-13546- compute jacobian from model table viam-modules/trajex#17

Draft

linter

41207a1

acmorrow requested changes May 28, 2026

View reviewed changes

nfranczak added 2 commits May 29, 2026 16:37

requested changes

dbe6b9b

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into 2025…

57560da

…0514-RSDK-13982-construct-model-table

nfranczak force-pushed the 20250514-RSDK-13982-construct-model-table branch from 60f7024 to 57560da Compare May 29, 2026 20:38

nfranczak added 2 commits May 29, 2026 16:45

pass ci

dcb17e1

no number

0aea0e1

nfranczak requested a review from acmorrow May 29, 2026 20:48

acmorrow requested changes Jun 1, 2026

View reviewed changes

nfranczak and others added 4 commits June 1, 2026 16:41

introduce ModelTable class, allow by_parents, all_childre, visited to…

db6ace0

… be un-ordered

Merge branch 'main' into 20250514-RSDK-13982-construct-model-table

f92fa0f

remove

8098d3a

Merge branch '20250514-RSDK-13982-construct-model-table' of github.co…

7f45e51

…m:nfranczak/viam-cpp-sdk into 20250514-RSDK-13982-construct-model-table

nfranczak requested a review from acmorrow June 2, 2026 00:06

acmorrow approved these changes Jun 2, 2026

View reviewed changes

Merge branch 'main' of github.com:viamrobotics/viam-cpp-sdk into 2025…

cc96c67

…0514-RSDK-13982-construct-model-table

lia-viam reviewed Jun 2, 2026

View reviewed changes

nfranczak added 2 commits June 2, 2026 13:47

bytes to string

ea3c36a

spaces

6681731

acmorrow reviewed Jun 2, 2026

View reviewed changes

nfranczak added 3 commits June 2, 2026 15:18

change /// to // in comments

3263b20

whoops

ccd4230

whitespace

09975de

nfranczak merged commit 1460e45 into viamrobotics:main Jun 3, 2026
5 checks passed

		@@ -0,0 +1,43 @@
		/// @file referenceframe/private/kinematics_model_table_internals.hpp

Conversation

nfranczak commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lia-viam commented May 21, 2026

Uh oh!

lia-viam left a comment

Choose a reason for hiding this comment

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

acmorrow May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfranczak Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lia-viam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nfranczak commented May 20, 2026 •

edited

Loading

acmorrow May 28, 2026 •

edited

Loading

nfranczak Jun 1, 2026 •

edited

Loading