v4r-tuwien / grasping_pipeline

1 stars 4 forks source link

Merge new table extractor and table collision stuff #13

Closed lexihaberl closed 1 year ago

jibweb commented 1 year ago

Minor point, it would be nice to make the placement an optional thing for the main startup (make the action client timeout with a clear message). I did not see an addition to the state machine (might not make sense right now given the upcoming cleanup) but it would be nice to keep some flexibility :-)

Down the line, it would probably also make sense not to recreate from scratch the moveit collision env. in the action callback when we have access to a map but just update the correct part and trust the localization to handle the rest (maybe even further down the line to always be localized, either using a map, or, during demos out of the lab, using a marker to get position relative to the table)

lexihaberl commented 1 year ago

The placement package is needed to obtain the table collision object, so I don't really see why we should make it optional. Regarding the rest: I just made slight adjustments to Christian's code so that it runs with the new table plane extractor, but I agree that we should probably rework parts of it, once we update the statemachine.

jibweb commented 1 year ago

:thinking: Why is the code necessary to obtain the table collision object in the placement? We may need to better define the boundaries of each package :grimacing:

lexihaberl commented 1 year ago

Because currently the placement_collision_environment actionserver from the placement package is used to create the collision environment. But it might be worth to move that part into its own package.

jibweb commented 1 year ago

probably yes, at least if it makes sense to re-use the same one everywhere. How much does it differ from the move_it execute_grasp_server from this repo? It might also make sense to merge there? Feel free to merge btw, those are just side discussions

lexihaberl commented 1 year ago

Seems like the placement_collision_environment actionserver is the one that actually creates the collision environment out of collision objects while the stuff in the execute_grasp_server only creates and sends the collision objects to the placement_collision_environment server. They also share big parts of the same code, so it's currently highly redundant. Maybe just add a new state for creating the collision environment and move all neccessary code (+action server) for that into the grasping_pipeline repo (as we probably only need that in here)?

jibweb commented 1 year ago

That sounds reasonable! It's probably worth discussing with Christian too!