Allow letting value field empty for numeric ports

[corot]

Unlike on v3, on v4 you cannot let the value field empty for numeric ports. You get this parsing error:

The port with name "angle_tolerance" and value "" can not be converted to double

I think v3 behavior was nicer cause it gives you a clean way to make your numeric fields optional: empty string -> null_optional value -> use your default

On v4 you need to provide a value that you know is a default (e.g. nan), but nobody else will know. This is awkward and different from string values, where empty string is allowed.

Disclaimer: I didn't dig deep into the code, so not sure that this is the best implementation. It just replicate v3 behavior.

[tony-p]

Note this functionality was definitely available on v4.4.2 and has broken somewhere since then

[facontidavide]

Unit tests are failing. Please remember to run the tests before submitting a PR :)

[corot]

Unit tests are failing. Please remember to run the tests before submitting a PR :)

@facontidavide :bow: my bad.. I was expecting that CI runs automatically.

@tony-p I saw that this behavior change was introduced with commit 6c9929c99e0546fcb51d9a0e2dc1febe5c7a624b, following the discussion on issue #768, opened by @KentaKato. I may be missing something, but I don't really get the point of the issue. The possibility of letting keys empty and get a std::nullopt as a result is both clean and convenient, imho. I saw some places where std::optional is used as the port type. But allowing empty strings you eliminate that complexity.

All that said, I have not used the complex type construction from string feature. So far I have only set numeric, boolean and string values. So it can be that I'm missing some important point.

[tony-p]

Looking at those issues, this seems like a backwards compatibility issue for behavior trees generated with groot2.

in older versions <1.5.2, an empty string would be assigned to all unassigned ports, whereas they should have been left out of the xml all together. As was the issue for complex types, an empty string is not always the same as a null pointer (empty string can have meaning).

Using the latest groot2, if you were to create a new tree with newly generated models, everything should work as expected (as now the ports are left out of the generated BT).

(As we are observing the same problem) there are kind of 2 solutions: 1) go back and manually fix all behavior tree xml files - less than ideal 2) create some backwards compatibility for specific types.

The first place I observe it is for output ports. I think an empty string for these can be safely assumed to be unassigned as values need to be assigned to a blackboard value for further use.

For input ports, maybe trying to carve out a difference in meaning for non nullable types is probably the best option

[corot]

in older versions <1.5.2, an empty string would be assigned to all unassigned ports, whereas they should have been left out of the xml all together. As was the issue for complex types, an empty string is not always the same as a null pointer (empty string can have meaning).

Using the latest groot2, if you were to create a new tree with newly generated models, everything should work as expected (as now the ports are left out of the generated BT).

:thinking: I'm not sure about this comment. I'm using 1.6.1 (updated some weeks ago) and it creates empty ports. True is that I don't create the models myself, but ask the factory to generate automatically.