Is this a code bug?

[jeezrick]

bool GridCollisionChecker::inCollision(
  const float & x,
  const float & y,
  const float & angle_bin,
  const bool & traverse_unknown)
{
  // Check to make sure cell is inside the map
  if (outsideRange(costmap_->getSizeInCellsX(), x) ||
    outsideRange(costmap_->getSizeInCellsY(), y))
  {
    return false;
  }

...

bool NodeHybrid::isNodeValid(
  const bool & traverse_unknown,
  GridCollisionChecker * collision_checker)
{
  if (collision_checker->inCollision(
      this->pose.x, this->pose.y, this->pose.theta /*bin number*/, traverse_unknown))
  {
    return false;
  }

  _cell_cost = collision_checker->getCost();
  return true;
}

Isn't this should return true? When node x or y is outside range, this node is invalid, hence inCollision function should return true, then isNodeValid function should return false.

[SteveMacenski]

@jwallace42 that's part of your PR https://github.com/ros-planning/navigation2/commit/b232341504d9d6d6fdc1c71cfd8287d90f263579 -- thoughts?

I would tend to agree, but I want to make sure the original author agrees as well and there isn't something subtle he's relying on with that condition. @jeezrick can you open up a PR to invert that? Should be easy for me to merge if he's on the same page.

[jwallace42]

Yeah,

that should be flipped.

[SteveMacenski]

@jeezrick please submit a PR and I can merge! Thanks for pointing this out!

[SteveMacenski]

Its been a few weeks, so I just took care of it :-) https://github.com/open-navigation/navigation2/pull/4296

Thanks for the note @jeezrick !