Clean up gradient based traversability estimation#149
Conversation
nathanhhughes
left a comment
nathanhhughes
left a comment
There was a problem hiding this comment.
Thanks for putting this together! This is almost there, but we should actually clean up how the configs work / it would be good to confirm or rule out a bug in spatial_hash
| //! Thresholds for classifyTraversabilityVoxel(), set by derived class constructors. | ||
| float min_confidence_ = 1.0f; | ||
| float min_traversability_ = 1.0f; | ||
| bool pessimistic_ = true; |
There was a problem hiding this comment.
The base TraversabilityEstimator can have a config with these that the other estimators inherit from (that would be my preference)
| // Build a 2D height map from a TSDF layer by scanning each vertical column of | ||
| // voxels for the highest surface voxel within the robot's vertical scan window. | ||
| // Uses block/local index access directly to avoid the signed/unsigned division | ||
| // bug in spatial_hash::blockIndexFromGlobalIndex that corrupts getVoxelPtr | ||
| // lookups for negative world coordinates. |
There was a problem hiding this comment.
It would be good to verify that this is actually a bug. I ran an issue with this in a different implementation but was not able to replicate it with spatial_hash (so I think blockIndexFromGlobalIndex is actually correct)
There was a problem hiding this comment.
(we talked about this earlier and we can punt on this for this PR)
| } | ||
| return height_map; | ||
| } | ||
| // Where to put these helper functions? |
There was a problem hiding this comment.
this is correct, you can drop this comment
| : config(config::checkValid(config)) {} | ||
| : config(config::checkValid(config)) { | ||
| min_confidence_ = config.min_confidence; | ||
| min_traversability_ = config.min_traversability; | ||
| pessimistic_ = config.pessimistic; | ||
| } |
There was a problem hiding this comment.
Related to my earlier comment, I'd use the base-class constructor to pass this config directly
| : config(config::checkValid(config)) {} | ||
| : config(config::checkValid(config)) { | ||
| min_confidence_ = config.min_confidence; | ||
| min_traversability_ = config.min_traversability; | ||
| pessimistic_ = config.pessimistic; | ||
| } |
There was a problem hiding this comment.
ditto on passing the config to the base class
|
@nathanhhughes Thanks for the feedback. In addition, to cleaning up this PR, I feel like it's better to put the helper functions in some util files becuase they share the same logic to construct height map and gradient maps. Should I move the functions under the |
|
I'd actually put them in a traversability directory (and maybe move the rest of the code there as well), the 2D place directory is for the mesh places |
MultyXu
changed the title
nathanhhughes
left a comment
nathanhhughes
left a comment
There was a problem hiding this comment.
LGTM, thanks for cleaning this up! No rush but you're good to merge whenever now that the configs are updated
| protected: | ||
| explicit TraversabilityEstimator(const Config& config) : config(config) {} |
There was a problem hiding this comment.
It's okay for this to not be protected, it's clear that you can't directly instantiate this because of the pure abstract method (protected constructors are useful for CRTP and singletons but not here)
| // Build a 2D height map from a TSDF layer by scanning each vertical column of | ||
| // voxels for the highest surface voxel within the robot's vertical scan window. | ||
| // Uses block/local index access directly to avoid the signed/unsigned division | ||
| // bug in spatial_hash::blockIndexFromGlobalIndex that corrupts getVoxelPtr | ||
| // lookups for negative world coordinates. |
There was a problem hiding this comment.
(we talked about this earlier and we can punt on this for this PR)
|
Now this should be read with this PR, I've merged the functions in both places and use a dedicated sink. But I'm not quite sure if the sink is in the correct place since currently it is very specific to GradientTraversabilityPlaces. |
|
@MultyXu commented in the other PR, but where you have the sink works for me |
2 participants
Clean up the gradient-based traversability function. Mostly factor out functions to be general helper functions.