Add additional parameters to geometry field size

[g3force]

The geometry field size message contains the dimension for the penalty area since recently, but some other dimensions are still missing:

• the penalty mark is missing completely
• the center circle radius still needs to be retrieved from the list of arcs.
• the line thickness needs to be retrieved from any/all lines/arcs and might be inconsistent
• goal height, ball radius and robot radius are missing

Some additional thoughts:

Redundancy of parameters: Some of the parameters can also be retrieved from the lines and arcs, so they would be redundant, but reading them from the lines/arcs is not a nice solution. Add adds extra complexity to the teams code, and they may also want to verify that the line dimensions are consistent for both teams. The lines/arcs are primarily used by ssl-vision for calibration and depending on the field-setup, one might want to add/remove lines/arcs, if they are not painted on the field.

Constant values: Some parameters could be considered constant. The advantage of having them together with the rest is, that teams can retrieve all necessary dimensions from one place.

[tobiasheineken]

For documentation: I would not want to merge this PR.

I think that adding the redundant fields is bad, as teams now have to read both values and check them for consistency. Otherwise you might end up in a situation where both teams (or teams and the autorefs) disagree about the geometry, which could be very bad and not easy to spot as a human during the game. And deprecating the old information (and no longer sending it out) forces every team to rewrite their geometry parsing now.

So instead of making it easier on teams to retrieve this data, we are making it harder for teams to retrieve this data - which I would consider an own goal.

Instead of adding redundant information, I would prefer to add code-snippets that teams can use to retrieve them from the already published information. This has the added benefit that it works retroactively on old logs, too.

[rolfvdhulst]

I mostly agree with Tobias regarding this PR. More duplicate fields is not a good thing, because it means teams need to check for validity of the given data as it can suddenly become ambiguous. Deprecation is also not really a viable option.

Adding the penalty mark is fine, and the goal height is also relevant information. The ball radius is also a nice addition in my opinion.

Robot radius is ambiguous, as it may vary per team and even per robot. This is an uniformative field at best, and at worst misleading; I would expect this information in the Detectionframes, where currently the robot height is passed as well; it makes sense for me to have those two in the same place, at least. Line thickness and the center-circle radius are particularly a problem as they are already contained within the message (albeit differently). Having multiple fields is in my opinion a bad idea due to ambiguity; if one would add these, I think the old ways of communicating them should be deprecated.

[g3force]

Thx for the feedback @rolfvdhulst

Regarding the robot radius: Would it help, if it was called max_robot_radius? That would better reflect the meaning and intention of this field.

[g3force]

The main point of concern from you both regarding redundant parameters is the validation. That was actually one of the reasons I added the additional parameters. Doing a validation of the data in each application that reads those data is annoying, so I'd prefer one unique value in the long term so that future teams do not need to care anymore.

Providing code snippets sounds really annoying and unpractical to me. I'd rather suggest to add a validation check into the quality-inspector and make sure it is always used and supervised my the vision expert.

[rolfvdhulst]

I think max_robot_radius would be more appropiate name if that's what you intend it for, and can see why you would add that.

I understand the need for removing those parameters. If you implement it so that it is impossible to configure SSL-Vision to send ambiguous messages by ensuring the sent linewidth is only defined from one place, then I think having duplicate fields is fine for the purpose of backwards compatibility (though I'd personally still prefer to see it deprecated, but this is unfair to the teams as Tobias argued). As it is now, any change in the line width in the interface would lead to an ambiguous message.

[g3force]

In the current implementation, the lines and arcs are generated based on the configured field dimensions. So by default, they are all consistent. However, even today someone could just change any line/arc and the values would not be consistent anymore.

Technically, I could set the generated lines or some fields to read-only. That's pretty easy. But I think it is valid that someone would want to change it (not for an official tournament, but for a test field).

I also do not want to set the lines/arcs to deprecated or even remove them to keep backwards compatibility.

Btw, the generation of the lines and arcs is only some years old. Before, all lines were edited manually. That's probably the reason why people started implementing validations ;)

[g3force]

Well, you could indeed think about generating a specific protobuf file for all rule-based parameters and include it everywhere. You would than also need to include the values as well. After all, the constants in ssl-vision are only the defaults, so as soon as you have setup ssl-vision, the default values are not touched anymore. So I doubt it would really help that much. What might be helpful is a button for setting the default parameters for DivA and DivB.