Skip to content

Add tl::expected wrapping#36

Merged
sea-bass merged 10 commits into
mainfrom
tl-expected-wrapping
Aug 4, 2025
Merged

Add tl::expected wrapping#36
sea-bass merged 10 commits into
mainfrom
tl-expected-wrapping

Conversation

@sea-bass

@sea-bass sea-bass commented Jul 25, 2025

Copy link
Copy Markdown
Collaborator

Resolves #9.

  • Adds a copy-pasted version of tl::expected to the repo
  • Adds generic templates to the bindings to support "unwrapping" tl return types
  • Converts RRT::Plan, getPathLengths, and getNormalizedPathScaling to use tl::expected instead of optionals
  • Binds the functions using the templates

@sea-bass sea-bass changed the title [WIP`tl::expected wrapping [WIP] tl::expected wrapping Jul 25, 2025
@sea-bass
sea-bass requested a review from eholum July 25, 2025 02:27
@sea-bass

sea-bass commented Jul 25, 2025

Copy link
Copy Markdown
Collaborator Author

@eholum I switched to using a copy-pasted version of tl::expected to support the non-ROS workflows.

Tried my hand at the macro, and even with the help of ChatGPT I couldn't quite figure it out. I kept getting a nanobind error about overloaded functions.

Something like this:

  #define UNWRAP_EXPECTED(func_ptr) \
    [](auto&& obj, auto&&... args) -> decltype(auto) { \
       return unwrap_expected((std::forward<decltype(obj)>(obj).*func_ptr)(std::forward<decltype(args)>(args)...)); \
  }

  nanobind::class_<RRT>(m_rrt, "RRT")
      .def("plan_expected", UNWRAP_EXPECTED(&RRT::plan_expected))

yielded

  /home/sebastian/.local/lib/python3.12/site-packages/nanobind/include/nanobind/nb_func.h:351:11: error: ‘decltype’ cannot resolve address of overloaded function
    351 |     using am = detail::analyze_method<decltype(&std::remove_reference_t<Func>::operator())>;
        |           ^~
  /home/sebastian/.local/lib/python3.12/site-packages/nanobind/include/nanobind/nb_func.h:354:14: error: ‘decltype’ cannot resolve address of overloaded function
    354 |         std::make_index_sequence<am::argc>(), extra...);
        |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ninja: build stopped: subcommand failed.

Maybe you can figure it out, but I think I'm now leaning towards writing the lambdas by hand.


EDIT: BTW this doesn't seem to be fault of the macro, but rather the structure that makes this generic. For instance, this fails with the same error:

  nanobind::class_<RRT>(m_rrt, "RRT")
      .def("plan_expected",
           [](RRT& self, auto&&... args) {
             return unwrap_expected(self::plan_expected(std::forward<decltype(args)>(args)...));
           })

@sea-bass
sea-bass force-pushed the tl-expected-wrapping branch from d5364f6 to b837c29 Compare July 25, 2025 02:39
@eholum

eholum commented Jul 25, 2025

Copy link
Copy Markdown
Collaborator
  #define UNWRAP_EXPECTED(func_ptr) \
    [](auto&& obj, auto&&... args) -> decltype(auto) { \
       return unwrap_expected((std::forward<decltype(obj)>(obj).*func_ptr)(std::forward<decltype(args)>(args)...)); \
  }

idk who thinks that C++ can be hard to read because this is as straightforward as it comes.

@eholum

eholum commented Jul 25, 2025

Copy link
Copy Markdown
Collaborator

6941c5e

Maybe this could go horribly wrong if we end up with a ton of templates... but does that matter? Is that in any way worse than a macro?

This one sparks joy:

.def("plan_expected", unwrap_expected(&RRT::plan_expected))

This one does not spark joy:

  nanobind::class_<RRT>(m_rrt, "RRT")
      .def("plan_expected",
           [](RRT& self, auto&&... args) {
             return unwrap_expected(self::plan_expected(std::forward<decltype(args)>(args)...));
           })

@sea-bass

Copy link
Copy Markdown
Collaborator Author

That's beautiful! Please commit it to this branch.

I was also thinking about the "what if the unexpected type is not a string?". For now, I would say we require it. If we ever need to use non-string types, then we can basically require that the type implements the ostream<< operator, which is sensible, but I really don't think we're gonna need any time soon.

@eholum

eholum commented Jul 25, 2025

Copy link
Copy Markdown
Collaborator

we can basically require that the type implements the ostream<< operator

Oooooh yeah that's a good idea and encompasses strings out of the box.

@eholum

eholum commented Jul 30, 2025

Copy link
Copy Markdown
Collaborator

@sea-bass how far did you get with this PR? And how far do you want it to get to call it "ready"? Happy to help out here if you're moving on with TOTG.

@sea-bass

sea-bass commented Jul 30, 2025

Copy link
Copy Markdown
Collaborator Author

@sea-bass how far did you get with this PR? And how far do you want it to get to call it "ready"? Happy to help out here if you're moving on with TOTG.

Yeah I don't think it's ready and I don't have a whole lot of time the next few days (moving), so go ahead!

I don't think this PR is very far away, though. As long as the mechanism is there to express expecteds, and it works well, it's fine. Standard usage across all the functionality can come later.

@eholum
eholum marked this pull request as ready for review August 4, 2025 13:13
@eholum

eholum commented Aug 4, 2025

Copy link
Copy Markdown
Collaborator

Ok, converted two of the path_utils functions to use tl::expected instead of std::optional since the bindings I added above were specific to class functions. I had to add a second template to handle the free function case so hopefully it's obvious. In any case, the wrappings are still the same:

  // Free function
  m_core.def("getPathLengths", unwrap_expected(&getPathLengths));

  // Class function
  nanobind::class_<RRT>(m_rrt, "RRT")
    .def(nanobind::init<const std::shared_ptr<Scene>, const RRTOptions&>())
    .def("plan", unwrap_expected(&RRT::plan))

@eholum eholum self-assigned this Aug 4, 2025
@eholum eholum changed the title [WIP] tl::expected wrapping Add tl::expected wrapping Aug 4, 2025
@sea-bass

sea-bass commented Aug 4, 2025

Copy link
Copy Markdown
Collaborator Author

Oh. I submitted the initial PR so I can't approve my own. Time to send it.

@sea-bass
sea-bass merged commit 004ac93 into main Aug 4, 2025
9 of 18 checks passed
@sea-bass
sea-bass deleted the tl-expected-wrapping branch August 4, 2025 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tl::expected : yay or nay?

2 participants