[ROS] Implemented Camera Renderer #23
Conversation
|
|
||
| if (!request.use_provided_tf) { | ||
| // TF not provided. Look it up. | ||
| // TODO @zrene. This is hardcoded. Can i use global_frame_name_? |
There was a problem hiding this comment.
Just to be sure, global_frame_name_ should be fine here, right?
There was a problem hiding this comment.
Yes, global frame name would be better here. In principle, you'd still have to do the transformation of the submap to global frame (or easier of the requested pose to the submap). I'd specify in the comments in which frames lookups happen but global is the most intuitive.
| bool use_depth | ||
| # Flag if custom tf is provided for the map lookup. | ||
| # If false, transform will be calculated using tf lookups from the sensor frame | ||
| bool use_provided_tf |
There was a problem hiding this comment.
Documentation for these fields is not entirely clear. E.g. when is the TF/transform used and which params to supply when.
| @@ -17,6 +17,7 @@ cs_add_library(${PROJECT_NAME} | |||
| src/visualization/single_tsdf_visualizer.cpp | |||
| src/visualization/planning_visualizer.cpp | |||
| src/visualization/tracking_visualizer.cpp | |||
| src/visualization/camera_renderer.cpp | |||
There was a problem hiding this comment.
There's already a map renderer (quite ugly one though) in panoptic_mapping package. This is essentially a single_tsdf_renderer, right? Would be nicer to have this as part of the library and keep the ROS interface separate.
| const Config config_; | ||
|
|
||
| // Members. | ||
| std::shared_ptr<Globals> globals_; |
There was a problem hiding this comment.
On the one-hand it's nice to have these stored here only once, however if e.g. a new map is loaded the renderer will no longer work. Maybe passing the camera and submap collection as const ref when needed could be an option? Also these should likely be const pointers since you don't want the renderer to change the map, right?
panoptic_mapping_ros/include/panoptic_mapping_ros/visualization/camera_renderer.h
Outdated
Show resolved
Hide resolved
|
|
||
| namespace panoptic_mapping { | ||
|
|
||
| void CameraRenderer::Config::checkParams() const { /* No params */ |
There was a problem hiding this comment.
If there's no checks you can also not implement that method, however I also usually do it like you did, since it's easier to later on add checks.
| &depth_image, method); | ||
| } else { | ||
| // Use 1000 as default depth value | ||
| depth_image = cv::Mat(globals_->camera()->getConfig().height, |
There was a problem hiding this comment.
Why 1000? Zero or so might be more intuitive (e.g. what you get from a failing depth camera I think), in particular if the output images are to be further processed.
| case CameraRenderer::UNCERTAINTY_METHOD::PROBABILITY: | ||
| return panoptic_mapping::classVoxelEntropy(voxel); | ||
| case CameraRenderer::UNCERTAINTY_METHOD::VOXEL_ENTROPY: | ||
| return panoptic_mapping::classVoxelBelongingProbability(voxel); |
There was a problem hiding this comment.
entropy and probability are mixed.
|
|
||
| for (const Submap& submap : *submaps_) { | ||
| // Filter out submaps. | ||
| if (!submap.isActive()) { |
There was a problem hiding this comment.
Do these filters always hold? In the single_tsdf case I think the only submap is of type free_space. Alternatively these could also be params/part of the config?
| } | ||
|
|
||
| if (!camera_->projectPointToImagePlane(p_C, &u, &v) || u < 0 || | ||
| u >= cam_config_.width || v < 0 || v >= cam_config_.height) { |
There was a problem hiding this comment.
project point to plane already checks whether u and v are in bound I think.
| *uncertainty_image, cv::Point(u_min, v_min), | ||
| cv::Point(u_max, v_max), | ||
| get_voxel_uncertainty_value(*c_voxel, uncertainty_method), -1); | ||
| } |
|
Moved to issue #32 |
Implemented Camera Renderer to Project Semantic Classes / Uncertainty Values and Depth for a given Camera Pose