combine clusters on same ball

[dargem]

Sometimes the visual mesh can detect multiple clusters on the same ball, this aims to resolve that

• Add ball candidate struct inside ball detector as intermediary between clustering points and adding balls
• Create ball candidates for each cluster, and create ball candidates that are made up of multiple clusters when clusters are nearby and combination improve circularity
• Add new circularity score using number of bounded points in the ball candidate observed divide by expected number of bounded points for that cone
• Move some finding central axis and radius logic into visualmesh.hpp
• Remove prior discarding of ball measurements when emitting invalid ball messages in debug mode (if discarded they aren't visible for debugging as their position isn't saved)

Pre-PR Checklist:

Have you

• Updated NUbook if necessary (add link to NUbook PR here)
• Added/updated tests for your changes, including regression tests for bug fixes
• Updated relevant module READMEs
• Added/modified documentation directives in relevant code
• Added a descriptive title and relevant labels to the PR

Reviewers, Note

Tested in webots and can see its merging clusters properly when playing back the NBS file in nusight. Needs balldetector log level to debug or thrownout balls aren't emitted so they won't be visible in nusight. Jabulani texture seemed to split more often when testing it, can send the nbs file also. Thrownout balls are only emitted when ball detector has debug log level, originally measurements were also cleared so balls wouldn't be visible in nusight. I assume they were thrown out so ball localisation doesn't consider invalid balls, unsure if I should add something to the ball.proto that notes whether the balls valid, just keep the measurements as thrown out or if its not an issue as it'll only have effects when detector is in debug.

Fairly new to c++ and my first PR so I'm sure there's issues, feedback on anything is very appreciated.

Things left to do

• Can change merge buffer scalar and merge preference to make it merge more often
• Change balls.proto to note throwouts /or revert change to discarding measurements

[JohanneMontano]

Have you tested on the real robot?

[dargem]

Have you tested on the real robot?

I tested it last week on a bot and it was working but I've updated the logic since with the new fill based detection which I've only tested out on webots. I can try it out later on a robot

[JohanneMontano]

Have you tested on the real robot?

I tested it last week on a bot and it was working but I've updated the logic since with the new fill based detection which I've only tested out on webots. I can try it out later on a robot

Itd be nice to get some photos of results running in the robot as well. Images from the real robot tend to be noisier Try running a role with your module vs without then compare. I will try and review the code some time later

[dargem]

image from sarah, orange balls were the 2 cluster on the same ball which would've originally been displayed, white is the new emitted ball created from them

video is kindof lagged here in these images but it breaks up a lot when ball is close to the camera and it can cluster them decently

and a full video for it, when recording I changed down minimum ball distance since the current configs only had it accept balls more than 2.5m away and I turned down points needed for a cluster to 10 just so it wouldn't throw out some of the smaller detections to make the merging more visible.

merging.mp4

[JohanneMontano]

image from sarah, orange balls were the 2 cluster on the same ball which would've originally been displayed, white is the new emitted ball created from them

video is kindof lagged here in these images but it breaks up a lot when ball is close to the camera and it can cluster them decently

and a full video for it, when recording I changed down minimum ball distance since the current configs only had it accept balls more than 2.5m away and I turned down points needed for a cluster to 10 just so it wouldn't throw out some of the smaller detections to make the merging more visible.
merging.mp4

hmmmm can we get a better video? The problem with this is if it thinks that some other rogue detection somewhere is a part of the cluster, that can skew your centre pretty badly. I cant confirm if this is the case or not here since the video has like 0.0000001 fps. Is the nbs file still in sarah's? If so a screen recording of nusight scrubbing through the nbs file much better.

[ysims]

Nice, and good job with testing using NUsight and the ball visualisations. Nice use of ternaries, good commenting throughout, including readme. I didn't go into the algorithm itself in this review, just some consistency and best-practice points.

[JohanneMontano]

Some changes need to be addressed

[dargem]

used a vector over vector everywhere cos bool vectors have this weird template specialisation where it packs 8 bools in a byte which leads to some overhead vs char vector as it needs to do some bit manipulation to access or write to the specific bit. does use more memory this way but doubt that's much of a factor for this code

[jpptm]

used a vector over vector everywhere cos bool vectors have this weird template specialisation where it packs 8 bools in a byte which leads to some overhead vs char vector as it needs to do some bit manipulation to access or write to the specific bit. does use more memory this way but doubt that's much of a factor for this code

Are you sure that you're not worrying about something that the compiler might already be optimising for under the hood? I'd say just write it as simple as you can. Someone misunderstanding why we used chars and having a bad time debugging when something goes wrong later on is much more of a concern than wasting 7 bits if we do actually end up wasting that

[dargem]

bool vec would have 8 bools per byte but since memory gets addressed by the byte when accessing an element from a vector of bools it has to some bit manipulation to get out the specific value which makes it a bit slower to access elements. vector char just wastes 7 bits in order to not do any bit manipulation, so vector of bools came out a bit slower though it depended on the case when I was benchmarking with -O3 enabled. I read my og comment which is pretty unclear which is probably related to me being a bit drunk then. Its not much of a speed improvement and maintainability would be more important so I'll change it back.

[JohanneMontano]

bool vec would have 8 bools per byte but since memory gets addressed by the byte when accessing an element from a vector of bools it has to some bit manipulation to get out the specific value which makes it a bit slower to access elements. vector just wastes 7 bits in order to not do any bit manipulation, so vector of bools came out a bit slower though it depended on the case when I was benchmarking with -O3 enabled. I read my og comment which is pretty unclear which is probably related to me being a bit drunk then. Its not much of a speed improvement and maintainability would be more important so I'll change it back.

As I said earlier, either you find some compiler flag to let it optimise this on its own, or just waste 7 bits and make the next maintainer's life a bit easier since using a char as a bool just isnt making a lot of sense. In fact, this isn't where we should be optimising for speed, it would be in the algorithm that is used to find clusters and determine the heuristics to merge the clusters. Similar with the vector of raw pointers to vectors, one should first and foremost be looking at the cppref docs to see if there already is a method for what one is looking for.

[dargem]

hey fixed everything that was requested. Ended up just adding a flag to the ball.proto message which marks if a balls been invalidated so that nusight can still get the positions of the discarded balls for debugging and I just changed ball localisation which was the only consumer of the vision balls message to not consider invalidated balls. For the vector of raw pointers to vectors I ended up just changing it up so the helper functions just accept an iterator and the caller just makes a view of what it needs to iterate over. Feel a bit iffy about it and saw ranges weren't used much in the codebase though if you've got suggestions.