wpilibsuite / allwpilib

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

[ntcore] setRetained doesn't work #7399

Open spacey-sooty opened 4 hours ago

spacey-sooty commented 4 hours ago

Describe the bug When I use the function setRetained on a topic in Java it doesn't work.

To Reproduce Steps to reproduce the behavior:

  1. Clone https://github.com/spacey-sooty/nt-set-retained-repro
  2. Run in simulation
  3. Check glass to see topic Foo doesn't exist under retainedImage. I don't think this is a glass issue because when I disconnect and reconnect in AdvantageScope the values of any subtopics go away.

Expected behavior The Topic should have the retained property. This would cause glass to display it under the retained section and AdvantageScope to keep it between disconnects.

Desktop (please complete the following information):

PeterJohnson commented 3 hours ago

The reason for this is the publish() overwrites the properties since the topic had not yet been published. If you move the setretained to after the publish or pass the retained as part of the publish properties parameter, this will work. I agree this is confusing behavior that should be changed (or at least documented). On the server side, property sets to non-existent topics are ignored, but agreed this is confusing in an API sense.

If we are changing this behavior, we need to redefine what the properties parameter to publish() means. Currently what it is defined to be is the “initial” properties for a topic—the properties that are set if the topic does not already exist. We could change it to a “merge” behavior with previously locally set properties only if the topic didn’t already exist? Note this can still react in an unpredictable way if the topic gets published by someone else in between the setRetained() and publish() calls, so the “right” way will still be to set initial properties through the publish() call directly, but it will work in the likely common case. Not sure if that’s better or worse though.