`getClosestAngularBin()` returns unexpected bin number about navigation2 HOT 8 CLOSED

Sorry work got a bit busy last week. I should be able to get it done this week. Sorry about the delay!

from navigation2.

Am I correct in saying that simply casting the double back down to a float resolve the issue?

return static_cast<unsigned int>(floor(static_cast<float>(theta) / bin_size));

If so, can you submit a PR, that should be easy enough to merge. I'll also take a look when you submit that PR at other implicit double -> float conversions. I did a quick glance and I see a small number of other places that I should be more explicit about that conversion, but this is the only critical one.

From the 3 places that its used, it looks like double really is the valid input type, so we can squash it in the function itself. Two could be made into floats, but one can't/shouldn't, so its easier to just change it in the method instead.

from navigation2.

Am I correct in saying that simply casting the double back down to a float resolve the issue?

return static_cast<unsigned int>(floor(static_cast<float>(theta) / bin_size));

If so, can you submit a PR, that should be easy enough to merge. I'll also take a look when you submit that PR at other implicit double -> float conversions. I did a quick glance and I see a small number of other places that I should be more explicit about that conversion, but this is the only critical one.

From the 3 places that its used, it looks like double really is the valid input type, so we can squash it in the function itself. Two could be made into floats, but one can't/shouldn't, so its easier to just change it in the method instead.

Correct, casting the double to a float resolves the issue.

I can submit a PR but I wonder which branch should it go towards?

from navigation2.

Put it towards main, but I'll immediately backport to Humble/Iron.

Thanks for the report and soon-to-come PR! 😄

from navigation2.

My pleasure!

I had trouble building main on my machine but humble works flawlessly. Would it be ok if I PR into humble instead?

from navigation2.

All PRs need to be targeted to main and I'll backport to Humble using our tools. If the static cast works in Humble, there's no reason it shouldn't in rolling as well. I looked into this as well and I don't see an issue with that. If your tests pass in Rolling as well (add those unit tests) then we're all good :-)

from navigation2.

Any update? Should be a small change 😄

from navigation2.

Merging imminent, waiting on CI

Thanks @hsong-MLE

from navigation2.