search

clearpathrobotics / occupancy_grid_utils

Forked from https://kforge.ros.org/gridutils/git with Catkinizing changes. The package contains utilities for dealing with occupancy grids, represented as nav_msgs::OccupancyGrid objects, including coordinate conversions, shortest paths, ray tracing, and constructing from laser scans.
42 stars 48 forks source link

change combine_grid API #10

Closed hrnr closed 7 years ago

hrnr commented 8 years ago

Hi again,

I have another changelist for parts I use.

The current API for combine grids takes shared_ptr, which is not only theoreticaly and semanticaly wrong as not ownership is passed, but also prevents you completely to safely manage your occupancy grids without shared_ptrs.

I added 2 backward compatible functions using shared pointers.

Cheers, Jiri

efernandez commented 8 years ago

@hrnr Could you provide a use case (example code) using the new methods?

What if someone wants to have the OGMs on a vector owning them, so they're destroyed automatically?

hrnr commented 8 years ago

I see your point. I have choosen this aproach so it would be possible to implement backward compatible functions. But you're right it won't work in C++ this way.

The obvious proper way is to take (begin, end) iterators, but then it might be tricky to implement backward compatible functions. I'll give it a try. With std::reference_wrapper it should be possible.

btw Do you even care for backward compatibility here?

hrnr commented 8 years ago

Ok. I have changed that to accept iterators and used some trickery for compat functions. This seems to me as the most generic concept, same as functions.

Some code needed to go to header so it's now much biger, but this is C++, right?

hrnr commented 8 years ago

Hi,

what do you think about latest changes?

The new API follows standad C++ semantics and as a bonus I was able to implement compat functions. This API is much more flexible for my needs.

efernandez commented 8 years ago

FYI @afakihcpr

hrnr commented 7 years ago

I'm no longer interested in this. I have realized I can do the same job with ~100 SLOC. But thanks for maintaining this legacy code.