setzer22 / egui_node_graph

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

Fix #26 - use lambdas/a struct instead of macros #28

Closed philpax closed 2 years ago

philpax commented 2 years ago

As discussed in #26 - using lambdas / structures instead of macros. I think this is much easier for someone to reason about at a glance, especially as it doesn't introduce any new syntax or semantics. The first block was trivial, but the second block required me to introduce a temporary struct Evaluator to hold all of the relevant state.

I think the first block is uncontentious, but the second one might be a bit harder of a sell due to the increase in LoC. I think it's worth it, but you might not 😅

setzer22 commented 2 years ago

Hey @philpax :wave:! Thanks for the PR

I just realized CI was on PRs was not being triggered, so I fixed it and made a very tiny grammar change to force github to run it again.

philpax commented 2 years ago

Awesome! It's also possible to reduce the instantiation of the Evaluator down to once, instead of per case:

diff --git a/egui_node_graph_example/src/app.rs b/egui_node_graph_example/src/app.rs
index 0fcd684..85a673c 100644
--- a/egui_node_graph_example/src/app.rs
+++ b/egui_node_graph_example/src/app.rs
@@ -431,64 +431,39 @@ pub fn evaluate_node(
     }

     let node = &graph[node_id];
+    let mut evaluator = Evaluator::new(graph, outputs_cache, node_id);
     match node.user_data.template {
         MyNodeTemplate::AddScalar => {
-            // Calling `evaluate_input` recursively evaluates other nodes in the
-            // graph until the input value for a paramater has been computed.
-            // This first call doesn't use the `Evaluator` to illustrate what
-            // is going on underneath.
-            let a = evaluate_input(graph, node_id, "A", outputs_cache)?.try_to_scalar()?;
-            let b = evaluate_input(graph, node_id, "B", outputs_cache)?.try_to_scalar()?;
-
-            // After computing an output, we don't just return it, but we also
-            // populate the outputs cache with it. This ensures the evaluation
-            // only ever computes an output once.
-            //
-            // The return value of the function is the "final" output of the
-            // node, the thing we want to get from the evaluation. The example
-            // would be slightly more contrived when we had multiple output
-            // values, as we would need to choose which of the outputs is the
-            // one we want to return. Other outputs could be used as
-            // intermediate values.
-            //
-            // Note that this is just one possible semantic interpretation of
-            // the graphs, you can come up with your own evaluation semantics!
-            let out = MyValueType::Scalar { value: a + b };
-            populate_output(graph, outputs_cache, node_id, "out", out)
+            let a = evaluator.input_scalar("A")?;
+            let b = evaluator.input_scalar("B")?;
+            evaluator.output_scalar("out", a + b)
         }
         MyNodeTemplate::SubtractScalar => {
-            // Using the evaluator, the code gets as succint as it gets
-            let mut evaluator = Evaluator::new(graph, outputs_cache, node_id);
             let a = evaluator.input_scalar("A")?;
             let b = evaluator.input_scalar("B")?;
             evaluator.output_scalar("out", a - b)
         }
         MyNodeTemplate::VectorTimesScalar => {
-            let mut evaluator = Evaluator::new(graph, outputs_cache, node_id);
             let scalar = evaluator.input_scalar("scalar")?;
             let vector = evaluator.input_vector("vector")?;
             evaluator.output_vector("out", vector * scalar)
         }
         MyNodeTemplate::AddVector => {
-            let mut evaluator = Evaluator::new(graph, outputs_cache, node_id);
             let v1 = evaluator.input_vector("v1")?;
             let v2 = evaluator.input_vector("v2")?;
             evaluator.output_vector("out", v1 + v2)
         }
         MyNodeTemplate::SubtractVector => {
-            let mut evaluator = Evaluator::new(graph, outputs_cache, node_id);
             let v1 = evaluator.input_vector("v1")?;
             let v2 = evaluator.input_vector("v2")?;
             evaluator.output_vector("out", v1 - v2)
         }
         MyNodeTemplate::MakeVector => {
-            let mut evaluator = Evaluator::new(graph, outputs_cache, node_id);
             let x = evaluator.input_scalar("x")?;
             let y = evaluator.input_scalar("y")?;
             evaluator.output_vector("out", egui::vec2(x, y))
         }
         MyNodeTemplate::MakeScalar => {
-            let mut evaluator = Evaluator::new(graph, outputs_cache, node_id);
             let value = evaluator.input_scalar("value")?;
             evaluator.output_scalar("out", value)
         }

but that obviously gets rid of the manual example (can't have two mutable references to the same object), so I'm not sure if it's a worthwhile trade-off. I leave it to you to decide 🚀

setzer22 commented 2 years ago

Somehow it feels like the manual expansion is less important now that the code is not coming from a macro. With a method, you can always jump to its definition and it's easier to see what's going on. So I think we can apply your last suggestion.

setzer22 commented 2 years ago

Many thanks!