support modifying wrappers in user stages #253
Conversation
Codecov Report
@@ Coverage Diff @@
## master #253 +/- ##
==========================================
+ Coverage 51.63% 51.95% +0.33%
==========================================
Files 101 101
Lines 6590 6580 -10
==========================================
+ Hits 3402 3418 +16
+ Misses 3188 3162 -26
Continue to review full report at Codecov.
|
v4hn
force-pushed
the
modifying-wrapper
branch
3 times, most recently
from
f454c22
to
1ec5fec
Compare
|
I just pushed a set of commits that work for me now. Please have a look @rhaschke. I didn't touch the Merger because I don't use it and we don't have any tests for it. |
|
I won't have time before Thursday... |
core/src/container.cpp
Outdated
| const InterfaceState* external_from{ create_from ? new_from : internalToExternalMap().at(internal_from) }; | ||
| const InterfaceState* external_to{ create_to ? new_to : internalToExternalMap().at(internal_to) }; |
There was a problem hiding this comment.
I think, here you assume that internalToExternalMap() is only used to link external and internal states for reading interfaces.
If the interface is writing, i.e. create_* is true, but new_* is a nullptr (like in liftSolution() calls from SerialContainer) the resulting external_* state will be a nullptr as well. Further below, you will pass these null pointers to storeSolution, which passes them on to ContainerBasePrivate::onNewFailure.
There was a problem hiding this comment.
Just to add a motivation to avoid state bloat in interfaces:
We want to keep the number of states small to avoid extra planning steps in neighbouring states (propagators, connects). For each new interface state, they will trigger a new computation. Thus if the states don't differ (which is now the case for all states lifted from a serial container), we should reuse an existing state.
|
Sorry, I was on sick leave last week and am working through my backlog today. |
v4hn
force-pushed
the
modifying-wrapper
branch
3 times, most recently
from
07d8acb
to
6299110
Compare
|
I somewhat got stuck on a seemingly minor change I don't like to make. The options are either to modify the bimap to allow for 1:n relationships in either direction, or to split up start and end maps, because the direction of the actual 1:n relationship depends on the read/write direction. So I believe if we know whether the interface reads/writes, a single map for each interface could suffice. |
analogous to storeSolution
Possible features that require this interface might be - trajectory reparameterization, - trajectory optimization as post-processing (like MoveIt's CHOMP adapter), - multi-trajectory blending (similar to Pilz' sequence planner) Also remove invalid interfaces. Parallel containers must always forward solutions of their child stages, so simple spawning is not allowed without a child solution.
Doesn't make a difference in single-threaded planning, but still better. Also move to storeState helper.
and add a justification for doing so.
Required to lift a SolutionSequence, and modify its states (e.g., to add properties).
It's just a nicer interface than calling
`liftModifiedSolution(sol, InterfaceState{s}, InterfaceState{s}, child)` everywhere.
to simplify debugging.
Essential for SerialContainer that spawns multiple solutions with the same end state.
clang-tidy rightfully complained that moving rvalues doesn't make sense when the underlying method accepts const refs. The rvalue parameter interface, as it is used with spawn/etc. indicates that the solution is submitted to the system. But with the shared_ptr interface we need for `liftModifiedSolution` it does not make any sense, especially because we can't even move the passed pointer to the internal datastructures but have to copy...
v4hn
force-pushed
the
modifying-wrapper
branch
from
6299110
to
1ef46e2
Compare
Possible features that require this interface might be
Here is an example illustrating how difficult it is to do the same without the interface.
Fixes #208