Skip to content
This repository has been archived by the owner on Mar 3, 2019. It is now read-only.

Obstacle and CircularObstacle #12

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

Obstacle and CircularObstacle #12

wants to merge 37 commits into from

Conversation

PFGimenez
Copy link
Member

@PFGimenez PFGimenez commented May 20, 2018

Issue #2
Method bool isColliding(RectangularObstacle obs) is missing because RectangularObstacle is missing.

@PFGimenez
Copy link
Member Author

We should write some unit tests before merging the branch

float CircularObstacle::squaredDistance(const Vector2D &pos) const
{
// TODO : is "distance" really necessary ? (it uses a std::sqrt computation)
float out = std::max(0.f, pos.distance(rotation_center_) - radius_);
Copy link
Collaborator

@Hindi Hindi Jun 11, 2018

Choose a reason for hiding this comment

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

We should consider implementing an approximation of the euclidean distance that does not uses sqrt and add an entry in the config for this. (this has nothing to do with the merging request)

Copy link
Collaborator

@Hindi Hindi left a comment

Choose a reason for hiding this comment

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

auto should be used in a lot of places here, I didn't mention everything. I probably missed some constexpr and noexcept too.
Can we force operator&& on std::vector<Obstacle> in CompoudObstacle ?

if (typeid(*this) != typeid(rhs))
return false;

return static_cast<const CompoundObstacle &>(rhs).obstacles_list_ == obstacles_list_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as CircularObstacle::operator==(const Obstacle &rhs) const

if (typeid(*this) != typeid(rhs))
return false;

return radius_ == static_cast<const CircularObstacle &>(rhs).radius_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be factorised? Something like:

return Obstacle::operator==(rhs) && typeid(*this) != typeid(rhs)
  && radius_ == static_cast<const CircularObstacle &>(rhs).radius_; 


Vector2D RectangularObstacle::toObstacleCoordinateSystem(const Vector2D &point) const
{
return Vector2D(getXToObstacleCoordinateSystem(point), getYToObstacleCoordinateSystem(point));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The return type is already specified as Vector2D, a braced initializer is enough here.
return { getXToObstacleCoordinateSystem(point), getYToObstacleCoordinateSystem(point) };


Vector2D RectangularObstacle::toTableCoordinateSystem(const Vector2D &point) const
{
return Vector2D(cos_ * point.getX() - sin_ * point.getY() + rotation_center_.getX(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as RectangularObstacle::toObstacleCoordinateSystem(const Vector2D &point) const

Vector2D pointA = corners[i];
Vector2D pointB = corners[(i + 1) % 4];
float distance = pointA.distance(pointB);
int nbPoints = static_cast<int>(std::ceil(distance / longestAllowedLength));
Copy link
Collaborator

Choose a reason for hiding this comment

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

auto can be used here because the types are obvious:
auto distance
auto nbPoints


float RectangularObstacle::squaredDistance(const Vector2D &v) const
{
Vector2D in = toObstacleCoordinateSystem(v);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be const auto in (but with a better name than in ?)


bool RectangularObstacle::isInObstacle(const Vector2D &pos) const
{
Vector2D in = toObstacleCoordinateSystem(pos);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be const auto in (but with a better name than in ?)

if (typeid(*this) != typeid(rhs))
return false;

RectangularObstacle ro_rhs = static_cast<const RectangularObstacle &>(rhs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

const auto again

right_upper_corner_ == ro_rhs.right_upper_corner_;
}

bool RectangularObstacle::test_separation(const float &a, const float &b, const float &a2, const float &b2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

RectangularObstacle::test_separation can have the noexcept and constexpr specifiers

Copy link
Collaborator

Choose a reason for hiding this comment

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

std::min and std::max are not declared with noexcept... So i don't know if it's really a good idea.
In C++11: " constexpr functions may contain no more than a single executable statement: a return" Effective Modern C++ Scott Meyers, p99, but, i'm fine with C++14 ;-)

obs.getYToObstacleCoordinateSystem(right_upper_corner_rotate_));
}

float RectangularObstacle::getHalfDiagonal() const
Copy link
Collaborator

Choose a reason for hiding this comment

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

RectangularObstacle::getHalfDiagonal can be declared with noexcept

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

Successfully merging this pull request may close these issues.

3 participants