wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.05k stars 611 forks source link

[ntcore] nt::Topic::SetProperty does not support infinity value in C++, but works in Java #5965

Closed virtuald closed 9 months ago

virtuald commented 9 months ago

Reported by RobotPy user in https://github.com/robotpy/mostrobotpy/issues/20.

To verify this is a C++ bug, I wrote a C++ function that directly did Topic.SetProperty("foo", -INFINITY) and on the remote side it displayed the property value as null as reported by the user.

Any fixes for this should probably also test other special floating point values such as NaN.

PeterJohnson commented 9 months ago

I’m not sure how it directly “works” in Java, since setProperty() in Java only accepts a string (which must be json encoded by the user), not a double.

PeterJohnson commented 9 months ago

Doing a bit more digging, JSON does not support infinity or NaN values (https://stackoverflow.com/questions/13581843/php-how-to-encode-infinity-or-nan-numbers-to-json). They’re passed through by Java because it’s just dumping in the raw string into the properties value, but C++ is actually processing them as “real” (checked) json and thus rejects infinity values. As properties are json, I think this should be closed as a wontfix.

virtuald commented 9 months ago

FWIW some JSON implementations accept infinity as a valid value (for example, Python will happily encode/decode infinity/NaN).

>>> import json, math
>>> json.dumps(math.inf)
'Infinity'
>>> json.dumps(math.nan)
'NaN'

It is weird to me that the dump_float function decides that null is an acceptable substitute for an invalid floating point number. I would have thought it would throw? https://github.com/wpilibsuite/allwpilib/blob/31cd015970cb19e8b191840708c85ed39960cc38/wpiutil/src/main/native/thirdparty/json/include/wpi/detail/output/serializer.h#L812

If you want to close as a wontfix then I'll adjust the python wpi::json converter to not accept invalid floating point numbers. I do wonder what result they got with java now... is ntcore not actually validating the json string at all?

PeterJohnson commented 9 months ago

It’s in accordance with the nlohmann json docs: https://json.nlohmann.me/features/types/number_handling/#nan-handling which serializes as null, and is apparently consistent with JavaScript.

When called from Java, we do parse the passed argument and throw it it’s not valid: https://github.com/wpilibsuite/allwpilib/blob/31cd015970cb19e8b191840708c85ed39960cc38/ntcore/src/main/native/cpp/jni/NetworkTablesJNI.cpp#L724

auscompgeek commented 9 months ago

FWIW some JSON implementations accept infinity as a valid value (for example, Python will happily encode/decode infinity/NaN).

Yes, this is explicitly documented as violating the JSON spec. This is not interoperable.

If allow_nan is false (default: True), then it will be a ValueError to serialize out of range float values (nan, inf, -inf) in strict compliance of the JSON specification. If allow_nan is true, their JavaScript equivalents (NaN, Infinity, -Infinity) will be used.

https://docs.python.org/3/library/json.html#json.dump