search

NAMeC-team / CRAbE

GNU General Public License v3.0
4 stars 3 forks source link

Add filter side #92

Closed EtienneSchmitz closed 3 months ago

EtienneSchmitz commented 1 year ago

This issue has been addressed by #11, but it needs to be merged into the main branch and adjusted accordingly. Should we add 'Both' to the enum? Additionally, can we eliminate the option from clap and set a default value to 'Both' instead?

Wanchai290 commented 1 year ago

I had identified two more PRs linked to the side filter, one that took care of the ball and the other I forgot. Should they be part of this ?

21 and #25

Wanchai290 commented 1 year ago

Concerning your addition to the enum, I don't think it is necessary. If we do that, it means that the side filter will always stay in the pipeline no matter the situation. I find it better to add the filter to the pipeline solely when it's required.

Can't we just ignore the default value option for clippy ? This is one of those situations where there are no value we can consider as default here.

EtienneSchmitz commented 1 year ago

It appears that item #21 is related to this matter. The purpose of item #25 remains unclear to me. If we have no data on the ball, retaining the filter_data is essential. Otherwise, it will result in an problem. If there is no ball on the field, then the world structure will not contain a ball either.

Concerning your addition to the enum, I don't think it is necessary. If we do that, it means that the side filter will always stay in the pipeline no matter the situation. I find it better to add the filter to the pipeline solely when it's required.

It's not ideal to have an option here since 'None' could imply the absence of vision, rather than indicating both field side. Therefore, in my view, there should be four enumerations:

enum Side {
    Both, // (default)
    Left,
    Right, 
    None
}

The 'None' option could serve to remove the vision component while retaining only the strategic part, including the guarding mechanisms and so on. This setup could be beneficial for embedded systems, during challenges, or in other similar situations Maybe we need move this on common config instead on filter config.

Wanchai290 commented 1 year ago

25 seems unrelated and not necessary here