HULKs / hulk

All your base are belong to us!
https://hulks.de/hulk
GNU General Public License v3.0
61 stars 50 forks source link

subscribe_json required for (some?) additional outputs instead of direct deserialisation with subscribe_value #1448

Closed tuxbotix closed 1 month ago

tuxbotix commented 1 month ago

I encoutered this in #1447,

I'm subscribing to an Option<calibration::corrections::Corrections> value at Control.additional_outputs.last_calibration_corrections (current main). In my understanding, subscribe_value should work and directly give me the deserialisation.

For example, these seems to be working?

// `tools/twix/src/panels/image_color_select.rs`
let field_color:  BufferHandle<FieldColorParameters>= nao.subscribe_value(format!(
    "parameters.field_color_detection.{cycler_path}",
    cycler_path = cycler.as_snake_case_path()
));

// OR `tools/twix/src/panels/ball_candidates.rs`
let ball_candidates: BufferHandle<Option<Vec<CandidateEvaluation>>> =
    nao.subscribe_value(format!("{cycler_path}.additional_outputs.ball_candidates"));

But it didn't for Corrections! While there were no compilation or runtime errors, the deserialised values were garbage, easily verified when comparing the values in a Text panel.

let calibration_corrections: BufferHandle<Option<Corrections>>=
            nao.subscribe_value("Control.additional_outputs.last_calibration_corrections");

if let Some(value) = calibration_corrections.get_last_value().ok().flatten().flatten() {
    // type of value -> calibration::corrections::Corrections
    println!("{:?}", value);
}

But this works:

let calibration_corrections =
            nao.subscribe_json("Control.additional_outputs.last_calibration_corrections");

if let Some(value) = self
    .calibration_corrections
    .get_last_value()
    .ok()
    .flatten()
    .and_then(|value| serde_json::from_value::<Corrections>(value).ok())
{
    // type of value -> calibration::corrections::Corrections
    // works but ugly AF IMHO
    println!("{:?}", value);
}

So it looks like (some?) additional outputs are sent as JSON? or what's going on O.o , is there something wrong with the type itself?

schmidma commented 1 month ago

Just had a brief look, but do I see it correctly, that you subscribe to an Option<Option<T>> and not an Option<T>. If so, you have to specify the correct type (double wrapped Option). JSON is able to flatten this nesting, bincode in the current implementation is not.

tuxbotix commented 1 month ago

Huh I'm a bit confused now, the behaviour you explained seems to be the case, but I assumed the semantics differently:

The node has below additional output:

last_calibration_corrections:
        AdditionalOutput<Option<Corrections>, "last_calibration_corrections">,

so I assumed that I need to have a BufferHandle<Option<Corrections>> to subscribe. Am I mistaken? or should it be BufferHandle<Corrections> instead?

schmidma commented 1 month ago

It should be BufferHandle<Option<Option<Corrections>>>, as AdditionalOutput itself already introduces a layer of "optionality". As it is an additional output the node may decide not to produce the output, which in turn is reflected in having a None as output.

IIRC, there is the missing feature (some might call it a bug) that new cycles do not reset additional outputs to Nones. But that might be a different topic

tuxbotix commented 1 month ago

Aha now I understand what's going on! I'll change the node to remove the extra Option and see how that works out.