@nahueespinosa meta: why adding kNav2DifferentialModelName here? The AMCL node needs it to ensure parity with Nav2 AMCL, but the NDT node does not have such constraint.

We use nav2_amcl::DifferentialMotionModel in the default_ndt.ros2.yaml file. Let me change that.

@nahueespinosa meta: there is redundancy between beluga::Amcl and beluga_ros::Amcl. I recall we did not get as far as to deduplicate them, but that'd be the logical path. In which case, beluga_ros::Amcl is doing a lot more than it should, and with this change, beluga::Amcl would be doing less. Why remove the execution policy? Why remove (as opposed to generalize, if applicable) state jitter to avoid distribution collapse?

I don't think that's the kind of generalization that leads to maintainable code.

State jitter does not apply to NDT models. IIUC, the 2D case was sampling from the particle set, which has no effect; and the 3D case was adding zero-initialized states, which is simply wrong. We never created a map distribution compatible with the NDT map representation.

In any case, I think we should treat the NDT examples as a nice proof of concept, and avoid adding unnecessary features.

If we do want those features (e.g. configurable execution policies), we should do more thinking before generalizing things.

Also, having this amount of std::visit calls and templates of templates makes it really hard to play with explicit template instantiation. I really don't think that we should keep things as they are today.

The range pipeline refactor was meant to provide a way to leave behind (overly complicated) class templates and the mixin pattern. We are kind of going back to that if we try to provide an Amcl class that's general enough for any use case.

There are other approaches to composition that I think are worth exploring. I reckon I haven't had enough time to do so this year.

There's a lot to unpack there.

State jitter does not apply to NDT models.

But it does for AMCL, at least for this instantiation of it. Whether NDT filters should use this AMCL variant or a different MCL variant is a completely valid yet separate discussion IMO.

having this amount of std::visit calls and templates of templates makes it really hard to play with explicit template instantiation

That is a very good point. We ended up using std::visit to enable runtime configurations and to keep execution paths transparent to the compiler. We can push it to the edges, but I don't think we'll get to remove it. Can we?

If we do want those features (e.g. configurable execution policies), we should do more thinking before generalizing things.

I really don't think that we should keep things as they are today.

We are kind of going back to that if we try to provide an Amcl class that's general enough for any use case.

There are other approaches to composition that I think are worth exploring.

Hmm, all of that is valid, but that alternative design that is better than this doesn't exist yet. I give you that these AMCL classes are far from complete solutions, but walking back on them with nothing to show for in exchange seems like a regression to me.

But it does for AMCL, at least for this instantiation of it. Whether NDT filters should use this AMCL variant or a different MCL variant is a completely valid yet separate discussion IMO.

On second read, I didn't remove state jitter to avoid distribution collapse. That's taken care of by the motion model. I removed the recovery strategy based on injecting random particles from the map (which none of the NDT variants were actually doing).

"Adaptive" in AMCL originally stands for the fact that the sample size can change based on the KLD metric. The recovery strategy was not mentioned in the original paper, it comes from Prob Rob.

Can we?

Yes, with carefully implemented type erasure and benchmarking. I don't know how much benefit we get from inlining the sensor and motion models, we should measure that. I've also seen really interesting techniques to make type-erasure really efficient. The only drawback is not being able to inline, which might be a big deal in some scenarios. We should investigate.

Range pipelines on the other hand will suffer greatly without inlining. But that's not what we are trying to customize at runtime.

Hmm, all of that is valid, but that alternative design that is better than this doesn't exist yet.

Fair point.

I give you that these AMCL classes are far from complete solutions, but walking back on them with nothing to show for in exchange seems like a regression to me.

I understand your point, but it seems to me that we took a lot of shortcuts to get to this point and this is only one small step back. I'm not removing vital features, the two classes still implement (a version of) AMCL, and this unblocks experimentation to reduce memory usage at build time.

I removed the recovery strategy based on injecting random particles from the map

FWIW that still counts as roughening to cope with (particle depletion and) distribution collapse. Artificially increasing the variance of other distributions helps too but it is a different approach.

I've also seen really interesting techniques to make type-erasure really efficient. The only drawback is not being able to inline, which might be a big deal in some scenarios.

Hmm, the only path I see that avoids explicit combinatorial explosion (as std::visit already explodes but we don't see it) is type erasing within loops. Performance will take a hit, though I guess we could argue that you can't have it both ways: it's either performance or flexibility. I'm OK with that so long as top performance is still achievable.

it seems to me that we took a lot of shortcuts to get to this point

That's true.

this unblocks experimentation to reduce memory usage at build time.

Hmm, what's preventing explicit instantiation of the beluga::Amcl class in its current form? 🤔