Yes to getting rid of boost::noncopyable (hell, that was in there? 😕 ) and also bye bye createRobotModel (redundant method).

I would actually propose to just remove the PlanningScene(urdf_model, srdf_model) constructor right away instead of deprecating it. There is not much use in constructing a PlanningScene that way, because a RobotModel holds the same information and is the much more appropriate datastructure here.
Removing this constructor already suffices to get rid of the exceptions though and I don't think it's useful to move to a monadic factory method.

In theory I agree with the idea of monadic returns. I also agree with the idea of factories when constructors would need to "do some work". But in this case there is not much going on in the constructor and I do think

planning_scene::PlanningScene scene{ robot_model }

or even

auto scene{ planning_scene::PlanningScene{ robot_model } }

reads a lot better than

auto scene{ planning_scene::PlanningScene::create(robot_model).value() }

I do think

planning_scene::PlanningScene scene{ robot_model }

or even

auto scene{ planning_scene::PlanningScene{ robot_model } }

reads a lot better than

auto scene{ planning_scene::PlanningScene::create(robot_model).value() }

Agree with this! I'm not a fan of "always auto"

I would actually propose to just remove the PlanningScene(urdf_model, srdf_model) constructor right away instead of deprecating it.

Sure thing, will do!

The point of the create() function is that it's allowed to fail gracefully. With the constructor, we have no other choice but throwing an exception or flagging the instance invalid. I agree, however, that a plain constructor would be nice.

The point of the create() function is that it's allowed to fail gracefully. With the constructor, we have no other choice but throwing an exception or flagging the instance invalid.

I'm aware of that, but there are simply no things left in the constructor now that need graceful failure handling. 🙂
and adding the additional .value() everywhere is only worth the hassle when there are failure cases.

If you want to propose consistent factory methods for all classes as a standard interface, that's different (and a lot of rewriting). but then we should check how much that would blow up the caller side using moveit interfaces.

The point of the create() function is that it's allowed to fail gracefully. With the constructor, we have no other choice but throwing an exception or flagging the instance invalid.

I'm aware of that, but there are simply no things left in the constructor now that need graceful failure handling. slightly_smiling_face and adding the additional .value() everywhere is only worth the hassle when there are failure cases.

RobotModel could be null

This pull request is in conflict. Could you fix it @henningkayser?

This pull request is in conflict. Could you fix it @henningkayser?

Sadly this work seems to be abandoned. I'll open an issue to track continuing this work.