Hi Stéphane,

thanks for your feedback!

On 2011-02-10 09:42, Stéphane Magnenat wrote:
> - constructor with double: Add const to double. In general, it is good
> to add const to scalar arguments (also appears in insertScan()).
Is there an immediate benefit to that? I'm aware of avoiding copies of 
large objects by passing them as const references, but I can't think of 
a reason to 'const' every scalar value... would that also apply to any 
bool, then?


> - constructor with tree: The tree is copied isn't it? Why not passing a
> const reference?
It was supposed to be a shallow copy in order to initialize the wrapper 
with an already existing Ocree (the trees are usually quite large, so a 
deep copy should be avoided). Now that you mentioned it, I think it just 
creates confusion in the ROS interface, so I deleted it. The default use 
case should be to use the ROS class only (but since the wrapped tree is 
public anyone who knows what to do can swap it out).

> - for castRay()/insertRay(), it would be nice to templatize them on a
> functor performing per-cell operation (of course, this implies adding
> this feature to the underlying octomap implementation). This would allow
> to decouple the map book-keeping operations from the functional ones.
> Probably in the case of an octree it is not trivial as one must allocate
> cells if the functor changes the map, but it is certainly generic and
> useful.
That would be indeed nice to have as a clean solution, but as it 
requires some rewriting of OctoMap itself it probably won't happen soon.

Cheers,
Armin

-- 
Armin Hornung
Humanoid Robots Lab, Albert-Ludwigs-Universität Freiburg
Contact: http://www.informatik.uni-freiburg.de/~hornunga