Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ROS 2][grid_map_core] Code enhancements from master b7293f5 #494

Open
wants to merge 4 commits into
base: rolling
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions grid_map_core/include/grid_map_core/BufferRegion.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ class BufferRegion
constexpr static unsigned int nQuadrants = 4;

BufferRegion();
BufferRegion(
const Index & startIndex, const Size & size,
const BufferRegion::Quadrant & quadrant);
BufferRegion(Index startIndex, Size size, BufferRegion::Quadrant quadrant);
virtual ~BufferRegion() = default;

const Index & getStartIndex() const;
Expand All @@ -50,7 +48,7 @@ class BufferRegion

private:
//! Start index (typically top-left) of the buffer region.
Index staretIndex_;
Index startIndex_;

//! Size of the buffer region.
Size size_;
Expand Down
2 changes: 1 addition & 1 deletion grid_map_core/include/grid_map_core/CubicInterpolation.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ using FunctionValueMatrix = Eigen::Matrix4d;
* @param[in] nElem - number of elements in the container
* @return index that is within [0, nElem-1].
*/
unsigned int bindIndexToRange(int idReq, unsigned int nElem);
unsigned int bindIndexToRange(unsigned int idReq, unsigned int nElem);


/*!
Expand Down
20 changes: 10 additions & 10 deletions grid_map_core/include/grid_map_core/TypeDefs.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@
namespace grid_map
{

typedef Eigen::MatrixXf Matrix;
typedef Matrix::Scalar DataType;
typedef Eigen::Vector2d Position;
typedef Eigen::Vector2d Vector;
typedef Eigen::Vector3d Position3;
typedef Eigen::Vector3d Vector3;
typedef Eigen::Array2i Index;
typedef Eigen::Array2i Size;
typedef Eigen::Array2d Length;
typedef uint64_t Time;
using Matrix = Eigen::MatrixXf;
using DataType = Matrix::Scalar;
using Position = Eigen::Vector2d;
using Vector = Eigen::Vector2d;
using Position3 = Eigen::Vector3d;
using Vector3 = Eigen::Vector3d;
using Index = Eigen::Array2i;
using Size = Eigen::Array2i;
using Length = Eigen::Array2d;
using Time = uint64_t;

/*
* Interpolations are ordered in the order
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#ifndef GRID_MAP_CORE__EIGEN_PLUGINS__FUNCTORSPLUGIN_HPP_
#define GRID_MAP_CORE__EIGEN_PLUGINS__FUNCTORSPLUGIN_HPP_

#include <math.h>
#include <cmath>

template<typename Scalar>
struct scalar_sum_of_finites_op
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ class CircleIterator
* @param center the position of the circle center.
* @param radius the radius of the circle.
*/
CircleIterator(const GridMap & gridMap, const Position & center, const double radius);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like know what the motivation is for this change. It goes against const correctness.
https://isocpp.org/wiki/faq/const-correctness

Can you share what compile warning this solves?

Copy link
Author

Choose a reason for hiding this comment

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

Ah to be honest I didn't get these warnings myself. The title is a bit confusing, but this was just part of the effort to make rolling as up to date as possible as master.

Is it not advisable to fix these if we're not seeing them ourselves during colcon build? Then I would just port the major changes from these, but it's a bit confusing to know what was a needed fix and what was just a code clean up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea, especially if the change breaks ABI, I don't see any strong reason to fold it in. Perhaps it's worth starting with any bugfixes that have associated tests?

CircleIterator(const GridMap & gridMap, const Position & center, double radius);

/*!
* Assignment operator.
* @param iterator the iterator to copy data from.
* @return a reference to *this.
*/
CircleIterator & operator=(const CircleIterator & other);
CircleIterator & operator=(const CircleIterator & other) = default;

/*!
* Compare to another iterator.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,14 @@ class EllipseIterator
*/
EllipseIterator(
const GridMap & gridMap, const Position & center, const Length & length,
const double rotation = 0.0);
double rotation = 0.0);

/*!
* Assignment operator.
* @param iterator the iterator to copy data from.
* @return a reference to *this.
*/
EllipseIterator & operator=(const EllipseIterator & other);
EllipseIterator & operator=(const EllipseIterator & other) = default;

/*!
* Compare to another iterator.
Expand Down Expand Up @@ -88,7 +88,7 @@ class EllipseIterator
* @param[out] bufferSize the buffer size of the submap.
*/
void findSubmapParameters(
const Position & center, const Length & length, const double rotation,
const Position & center, const Length & length, double rotation,
Index & startIndex, Size & bufferSize) const;

//! Position of the circle center;
Expand Down
18 changes: 9 additions & 9 deletions grid_map_core/src/BufferRegion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,29 @@ namespace grid_map
{

BufferRegion::BufferRegion()
: staretIndex_(Index::Zero()),
: startIndex_(Index::Zero()),
size_(Size::Zero()),
quadrant_(BufferRegion::Quadrant::Undefined)
{
}

BufferRegion::BufferRegion(
const Index & index, const Size & size,
const BufferRegion::Quadrant & quadrant)
: staretIndex_(index),
size_(size),
quadrant_(quadrant)
Index index, Size size,
BufferRegion::Quadrant quadrant)
: startIndex_(std::move(index)),
size_(std::move(size)),
quadrant_(std::move(quadrant))
{
}

const Index & BufferRegion::getStartIndex() const
{
return staretIndex_;
return startIndex_;
}

void BufferRegion::setStartIndex(const Index & staretIndex)
void BufferRegion::setStartIndex(const Index & startIndex)
{
staretIndex_ = staretIndex;
startIndex_ = startIndex;
}

const Size & BufferRegion::getSize() const
Expand Down
41 changes: 16 additions & 25 deletions grid_map_core/src/CubicInterpolation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,15 @@
namespace grid_map
{

unsigned int bindIndexToRange(int idReq, unsigned int nElem)
unsigned int bindIndexToRange(unsigned int idReq, unsigned int nElem)
{
if (idReq < 0) {
return 0;
if (idReq >= nElem) {
return nElem - 1;
}
if ((unsigned)idReq >= nElem) {
return static_cast<unsigned int>(nElem - 1);
}
return static_cast<unsigned int>(idReq);
return idReq;
}

double getLayerValue(const Matrix & layerMat, int rowReq, int colReq)
double getLayerValue(const Matrix & layerMat, unsigned int rowReq, unsigned int colReq)
{
const auto numCol = layerMat.cols();
const auto numRow = layerMat.rows();
Expand Down Expand Up @@ -96,7 +93,7 @@ bool assembleFunctionValueMatrix(
}

const Matrix & layerMatrix = gridMap.get(layer);
auto f = [&layerMatrix](int rowReq, int colReq) {
auto f = [&layerMatrix](unsigned int rowReq, unsigned int colReq) {
double retVal = getLayerValue(layerMatrix, rowReq, colReq);
return retVal;
};
Expand Down Expand Up @@ -142,10 +139,7 @@ bool getIndicesOfMiddleKnot(
const GridMap & gridMap, const Position & queriedPosition,
Index * index)
{
if (!gridMap.getIndex(queriedPosition, *index)) {
return false;
}
return true;
return gridMap.getIndex(queriedPosition, *index);
}

} // namespace bicubic_conv
Expand Down Expand Up @@ -275,10 +269,7 @@ bool getClosestPointIndices(
const GridMap & gridMap, const Position & queriedPosition,
Index * index)
{
if (!gridMap.getIndex(queriedPosition, *index)) {
return false;
}
return true;
return gridMap.getIndex(queriedPosition, *index);
}

bool computeNormalizedCoordinates(
Expand Down Expand Up @@ -307,8 +298,8 @@ bool getFunctionValues(const Matrix & layerData, const IndicesMatrix & indices,

void bindIndicesToRange(const GridMap & gridMap, IndicesMatrix * indices)
{
const int numCol = gridMap.getSize().y();
const int numRow = gridMap.getSize().x();
const unsigned int numCol = gridMap.getSize().y();
const unsigned int numRow = gridMap.getSize().x();

// top left
{
Expand Down Expand Up @@ -358,10 +349,11 @@ double firstOrderDerivativeAt(
const Matrix & layerData, const Index & index, Dim2D dim,
double resolution)
{
const int numCol = layerData.cols();
const int numRow = layerData.rows();
const auto numCol{static_cast<unsigned int>(layerData.cols())};
const auto numRow{static_cast<unsigned int>(layerData.rows())};

double left, right;
double left;
double right;
switch (dim) {
case Dim2D::X: {
left = layerData(bindIndexToRange(index.x() + 1, numRow), index.y());
Expand Down Expand Up @@ -394,9 +386,8 @@ double mixedSecondOrderDerivativeAt(
* the order doesn't matter. Derivative values are the same.
* Taken from https://www.mathematik.uni-dortmund.de/~kuzmin/cfdintro/lecture4.pdf
*/

const int numCol = layerData.cols();
const int numRow = layerData.rows();
const auto numCol{static_cast<unsigned int>(layerData.cols())};
const auto numRow{static_cast<unsigned int>(layerData.rows())};

const double f11 = layerData(
bindIndexToRange(index.x() - 1, numRow),
Expand Down
77 changes: 30 additions & 47 deletions grid_map_core/src/GridMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

#include <Eigen/Dense>

#include <math.h>
#include <cmath>
#include <algorithm>
#include <cassert>
#include <iostream>
Expand Down Expand Up @@ -82,17 +82,13 @@ const std::vector<std::string> & GridMap::getBasicLayers() const

bool GridMap::hasBasicLayers() const
{
return basicLayers_.size() > 0;
return !basicLayers_.empty();
}

bool GridMap::hasSameLayers(const GridMap & other) const
{
for (const auto & layer : layers_) {
if (!other.exists(layer)) {
return false;
}
}
return true;
return std::all_of(layers_.begin(), layers_.end(),
[&](const std::string & layer){return other.exists(layer);});
}

void GridMap::add(const std::string & layer, const double value)
Expand Down Expand Up @@ -283,12 +279,8 @@ bool GridMap::isValid(const Index & index, const std::vector<std::string> & laye
if (layers.empty()) {
return false;
}
for (const auto & layer : layers) {
if (!isValid(index, layer)) {
return false;
}
}
return true;
return std::all_of(layers.begin(), layers.end(),
[&](const std::string & layer){return isValid(index, layer);});
}

bool GridMap::getPosition3(
Expand Down Expand Up @@ -448,10 +440,10 @@ GridMap GridMap::getTransformedMap(
if (sampleRatio > 0.0) {
positionSamples.reserve(5);
positionSamples.push_back(center);
positionSamples.push_back(Position3(center.x() - sampleLength, center.y(), center.z()));
positionSamples.push_back(Position3(center.x() + sampleLength, center.y(), center.z()));
positionSamples.push_back(Position3(center.x(), center.y() - sampleLength, center.z()));
positionSamples.push_back(Position3(center.x(), center.y() + sampleLength, center.z()));
positionSamples.emplace_back(center.x() - sampleLength, center.y(), center.z());
positionSamples.emplace_back(center.x() + sampleLength, center.y(), center.z());
positionSamples.emplace_back(center.x(), center.y() - sampleLength, center.z());
positionSamples.emplace_back(center.x(), center.y() + sampleLength, center.z());
} else {
positionSamples.push_back(center);
}
Expand Down Expand Up @@ -507,10 +499,7 @@ bool GridMap::move(const Position & position, std::vector<BufferRegion> & newReg
if (abs(indexShift(i)) >= getSize()(i)) {
// Entire map is dropped.
clearAll();
newRegions.push_back(
BufferRegion(
Index(0, 0), getSize(),
BufferRegion::Quadrant::Undefined));
newRegions.emplace_back(Index(0, 0), getSize(), BufferRegion::Quadrant::Undefined);
} else {
// Drop cells out of map.
int sign = (indexShift(i) > 0 ? 1 : -1);
Expand All @@ -524,49 +513,43 @@ bool GridMap::move(const Position & position, std::vector<BufferRegion> & newReg
// One region to drop.
if (i == 0) {
clearRows(index, nCells);
newRegions.push_back(
BufferRegion(
Index(index, 0), Size(nCells, getSize()(1)),
BufferRegion::Quadrant::Undefined));
newRegions.emplace_back(
Index(index, 0), Size(nCells, getSize()(1)),
BufferRegion::Quadrant::Undefined);
} else if (i == 1) {
clearCols(index, nCells);
newRegions.push_back(
BufferRegion(
Index(0, index), Size(getSize()(0), nCells),
BufferRegion::Quadrant::Undefined));
newRegions.emplace_back(
Index(0, index), Size(getSize()(0), nCells),
BufferRegion::Quadrant::Undefined);
}
} else {
// Two regions to drop.
int firstIndex = index;
int firstNCells = getSize()(i) - firstIndex;
if (i == 0) {
clearRows(firstIndex, firstNCells);
newRegions.push_back(
BufferRegion(
Index(firstIndex, 0), Size(firstNCells, getSize()(1)),
BufferRegion::Quadrant::Undefined));
newRegions.emplace_back(
Index(firstIndex, 0), Size(firstNCells, getSize()(1)),
BufferRegion::Quadrant::Undefined);
} else if (i == 1) {
clearCols(firstIndex, firstNCells);
newRegions.push_back(
BufferRegion(
Index(0, firstIndex), Size(getSize()(0), firstNCells),
BufferRegion::Quadrant::Undefined));
newRegions.emplace_back(
Index(0, firstIndex), Size(getSize()(0), firstNCells),
BufferRegion::Quadrant::Undefined);
}

int secondIndex = 0;
int secondNCells = nCells - firstNCells;
if (i == 0) {
clearRows(secondIndex, secondNCells);
newRegions.push_back(
BufferRegion(
Index(secondIndex, 0),
Size(secondNCells, getSize()(1)), BufferRegion::Quadrant::Undefined));
newRegions.emplace_back(
Index(secondIndex, 0), Size(secondNCells, getSize()(1)),
BufferRegion::Quadrant::Undefined);
} else if (i == 1) {
clearCols(secondIndex, secondNCells);
newRegions.push_back(
BufferRegion(
Index(0, secondIndex),
Size(getSize()(0), secondNCells), BufferRegion::Quadrant::Undefined));
newRegions.emplace_back(
Index(0, secondIndex), Size(getSize()(0), secondNCells),
BufferRegion::Quadrant::Undefined);
}
}
}
Expand All @@ -579,7 +562,7 @@ bool GridMap::move(const Position & position, std::vector<BufferRegion> & newReg
position_ += alignedPositionShift;

// Check if map has been moved at all.
return indexShift.any() != 0;
return indexShift.any();
}

bool GridMap::move(const Position & position)
Expand Down
Loading