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

Feature - Grow! #174

Closed
wants to merge 2 commits into from
Closed

Feature - Grow! #174

wants to merge 2 commits into from

Conversation

raghavakumar8
Copy link

Adds the ability to grow a gridmap instance efficiently (using Eigen functions) in different directions.

Resolves #132.

Eigen::MatrixXf like = Eigen::MatrixXf::Constant(size(0), size(1), value);
for (auto& data : data_)
{
data.second.conservativeResizeLike(like);
Copy link

Choose a reason for hiding this comment

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

Plain curiosity,
why are you using conservativeResizeLike rather than conservativeResize ??
The function conservativeResize allocates a temporary matrix under the hood, so I would imagine that conservativeResizeLike does the same. Thus isn't the temporary matrix like a waste of memory ?

Copy link
Author

Choose a reason for hiding this comment

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

conservativeResize doesn't let you initialize the values of new elements, conservativeResizeLike does. This is important when you want to be able to guarantee that every new grid cell is initalized to 0.0, for instance.

In principle I could use conservativeResize, loop through all new cells and manually initialize them, but I suspect that would be less efficient.

Copy link

@artivis artivis Aug 1, 2018

Choose a reason for hiding this comment

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

Ok I see your point.
Still I think a plain conservativeResize followed by manually setting the uninitialized values would be better as you don't really need allocating matrix like, especially as the matrix grows large.
Rather than looping over uninitialized values you can use the block functions, something along the lines :

data.second.conservativeResize(size(0), size(1));
// Assuming that resizing to larger dimension appends 
// rows at the bottom and cols at the right side.
// Set value for appended bottom rows
data.second.block(size_(0), 0, row_delta, size(1)).setConstant(value); 
// Set value for appended top-left block (remaining unintialized)
data.second.topRightCorner(size(0), col_delta).setConstant(value);

I haven't checked that the indices are actually correct ^^, but that's the general idea.

Edit: you could also use the block scheme to swap the whole original matrix over the new layout.

Choose a reason for hiding this comment

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

Is this feature working? What is it stopping from going to production?

Copy link
Author

Choose a reason for hiding this comment

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

Just waiting for an approving review. We have been using this feature extensively in production at our company.

Choose a reason for hiding this comment

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

this is very nice feature and exactly what I needed! A merge would be good.

Choose a reason for hiding this comment

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

+1 for useful feature and would be good to have merged

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, will import it.

JADC362 added a commit to Sunnybotics/grid_map that referenced this pull request Jul 6, 2021
@maximilianwulf
Copy link
Collaborator

@raghavakumar8 could you merge master back into your branch?

@maximilianwulf
Copy link
Collaborator

Closing in favor of #330 which has more up-to-date master.

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.

No possibility to resize a GridMap ?
6 participants