Creating a Shared Utility File for rj_strategy Positions

[rhkoulen]

Description

This PR introduces src/rj_strategy/include/rj_strategy/agent/position_utils.hpp. I'm hoping to address something that has really bothered me about strategy for a while.

In strategy files, we often rewrite the same function in so many different places. Not only that, but we also make different meta-decisions in different places. What counts as an open shot? What shots should be evaluated in shot selection? What field geometry is accessible? How hard should we kick the ball?

With a single source of truth, hopefully this will be more simple. If we change our approach to shot acquisition, every class that chooses to use the central shot planning function calculate_best_shot will all share that improved approach. Moreover, it attempts to tackle the huge amount of duplicated code. This file should contain functions about meta-strategy, common calculations, etc.

I've already adapted Offense and SoloOffense to use this utils file. I'm going to need to crusade to get people to use this, but I hope this becomes the most carefully maintained file.

Testing

You can simply build and try out Offense or SoloOffense. Their behavior should not have changed.

[rhkoulen]

One thing I'm not sure about in C++ syntax is namespaces. All of our position files are wrapped in a namespace strategy block. I initially did that in my file as well, but commenting it out didn't break anything, so I figured I would leave it out, to prevent my functions from polluting the namespace.

Basically, it's an hpp with a bunch of inline functions. Should those inline functions be defined inside namespace strategy? I don't see a reason, since any Position subclass that wants to use these can just include the hpp.

See lines 39 and 328. Should I uncomment those so all of the funcs are in the namespace? I don't know precisely what that does, so idk

[CameronLyon]

I love it 👍, this is needed badly.

1. Comments are extremely helpful and point out future TODOs on this part of this implementation
2. Some small syntax issues (unnecessary elses, combining if statements, etc), but not enough to change them tbh. I thinking leaving them makes the code easier to read
3. Definitely need to make another PR request implementing this utility in other strategy positions, expanding the position_utils a little more (defense, free_kicker, penalty_player, goalie, etc). That can be for later though since this PR already tackles one of the most bloated files (offense).

I plan on testing this code soon and I'll approve the PR if there are no issues, good stuff.

[CameronLyon]

Replying to your comment, namespace does two things from my experience:

1. Prevents name collisions with similarly named classes elsewhere in the project or in dependencies.
2. Keeps strategy-related code logically grouped, so Position, Offense, Defense, Goalie, all live under strategy.

This is more preference than a requirement on whether or not you want to include it, since removing it doesn't brick the codebase. I think including it would cause all of your functions to require something like "position_utils::FUNCTION_NAME" rather than simply saying "FUNCTION_NAME" to reference the position_utils. So do you want to:

• Leave it how it is and simplify how we reference position_utils? Might cause more confusion on where certain functions are being called but will avoid using "position_utils::" everytime we want to call it
• Bring namespace in and require all references to it to use "position_utils::FUNCTION_NAME"? While slightly annoying, does make it more understandable of where the function came from

I'd say leave it how it is, but if I'm wrong about how I see this and Aalind has a better reason to switch, then we can go that path. I don't mind either tbh

[rhkoulen]

I mean, it builds and runs with and without the namespace. Like, when I had namespace strategy { ... } before I commented it out, I didn't need to do position_utils::FUNCTION_NAME, I just did FUNCTION_NAME. And it built just the same after commenting it out. I really don't know how that's possible, because that's what I thought it was doing as well. Is it something to do with the fact that I defined them all as inline functions?

It may also be inappropriate for these to be inline functions. To my understanding, the compiler deals with all that, but inline is supposed to be faster because there's no linking, it just shoves in the code. But some of these functions loop a lot so idk

Need someone who knows C++ semantics to look at this.

[CameronLyon]

Tested it, everything works as intended.