setzer22 / egui_node_graph

Build your node graph applications in Rust, using egui
MIT License
715 stars 134 forks source link

Consider using lambdas instead of macros for the example #26

Closed philpax closed 2 years ago

philpax commented 2 years ago

Hey there! I'm building off the example to work for my use case, and I noticed you're using macros to construct the node parameters:

macro_rules! input {
    (scalar $name:expr) => {
        graph.add_input_param(
            node_id,
            $name.to_string(),
            MyDataType::Scalar,
            MyValueType::Scalar { value: 0.0 },
            InputParamKind::ConnectionOrConstant,
            true,
        );
    };
    // ...
}

macro_rules! output {
    (scalar $name:expr) => {
        graph.add_output_param(node_id, $name.to_string(), MyDataType::Scalar);
    };
    // ...
}

// ...
match self {
    MyNodeTemplate::SubtractScalar => {
        input!(scalar "A");
        input!(scalar "B");
        output!(scalar "out");
    }
}

I'd suggest using lambdas, because they're easier to parameterise and (in my opinion) cleaner:

let input_scalar = |graph: &mut MyGraph, name: &str| {
    graph.add_input_param(
        node_id,
        name.to_string(),
        MyDataType::Scalar,
        MyValueType::Scalar { value: 0.0 },
        InputParamKind::ConnectionOrConstant,
        true,
    );
};

let output_scalar = |graph: &mut MyGraph, name: &str| {
    graph.add_output_param(node_id, name.to_string(), MyDataType::Scalar);
};

match self {
    MyNodeTemplate::SubtractScalar => {
        input_scalar(graph, "A");
        input_scalar(graph, "B");
        output_scalar(graph, "out");
    }
}
gmorenz commented 2 years ago

:+1: - this is how I wrote my actual code too, it seems like the right way to me.

setzer22 commented 2 years ago

The difference between a local macro and the lambda here is that the macro can "capture" the graph variable by borrowing at every invocation, unlike the closure which would borrow it for its whole lifetime. That's why the lambdas need the graph as an additional argument, to keep the same borrowing semantics.

That said, it may be clearer to avoid macros for this, and a single extra parameter on every call doesn't sound that bad. I'm open to a change like this if it makes the example clearer :)

I'm a bit short on time, but I would happily accept a PR for this :+1: