Skip to content

Conversation

Nisarg236
Copy link
Member

@Nisarg236 Nisarg236 commented Aug 26, 2025

@Nisarg236 Nisarg236 changed the title initial commit feat: add visualisations to debug NO_PATH_FOUND Aug 29, 2025
@Nisarg236 Nisarg236 marked this pull request as ready for review August 31, 2025 06:06
@Nisarg236 Nisarg236 requested a review from renan028 August 31, 2025 06:06
throw std::runtime_error("Start must be set before goal.");
}

const std::shared_ptr<costmap_2d::Costmap2DROS> costmap_2d_ros = _collision_checker->getCostmapROS();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: can we use const here?

Suggested change
const std::shared_ptr<costmap_2d::Costmap2DROS> costmap_2d_ros = _collision_checker->getCostmapROS();
const std::shared_ptr<const costmap_2d::Costmap2DROS> costmap_2d_ros = _collision_checker->getCostmapROS();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not required, I changed something here and then forgot to revert this.
But we can't change that to const because resetObstacleHeuristic doesnot accept that as parameter

Comment on lines +369 to +375
unsigned int map_x = static_cast<unsigned int>(this->pose.x + 0.5f);
unsigned int map_y = static_cast<unsigned int>(this->pose.y + 0.5f);
bool coordinates_in_bounds = (map_x < NodeHybrid::footprint_collision_map.info.width &&
map_y < NodeHybrid::footprint_collision_map.info.height);

unsigned int index = map_y * NodeHybrid::footprint_collision_map.info.width + map_x;
bool index_valid = coordinates_in_bounds && (index < NodeHybrid::footprint_collision_map.data.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: can't we use the costmap method here? (worldToMap, and in bounds check)

Comment on lines +378 to +380
if (index_valid) {
NodeHybrid::footprint_collision_map.data[index] = 100; // Mark as occupied
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R: I think we should process this only if there is subscriber to the collision publisher
handle this in a cleaner way, i.e., do not add publisher checks all around


void NodeHybrid::initializeFootprintCollisionMap(const std::shared_ptr<costmap_2d::Costmap2DROS>& costmap_ros)
{
const costmap_2d::Costmap2D* costmap = costmap_ros->getCostmap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q/R: You are copying the whole global costmap here again? if yes, this might be very expensive in some cases (e.g. in large maps).
Isn't this the same problem we had before and you handle it by creating a new map with the desired size (the search space bounds), instead of using all the map?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants