Skip to content

Bind each created node to an instance its parameters live on [minor] - #446

Merged
matt-edmondson merged 2 commits into
mainfrom
claude/node-instance-bindings
Sep 23, 2026
Merged

matt-edmondson merged 2 commits into
mainfrom
claude/node-instance-bindings

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #438

The gap

AttributeBasedNodeFactory.CreateNode<T> read the NodeDefinition, extracted the pin display names, and called engine.CreateNode(...). Everything else the definition knew was dropped at that boundary — no T was ever constructed, and no map was kept from a node id back to the definition it came from.

So a tunable declared the way the shipped library declares every one of them — [InputPin] public double Threshold { get; set; } = 128.0; — had nowhere to live, and nothing could answer "which type is this node the user selected". A host wanting an editable threshold had to keep its own parallel dictionary of instances keyed by node id, re-deriving the mapping the factory already had in hand.

The change

CreateNode constructs the type once per node, writes each input pin's declared default onto it, and binds it to the id the engine issued. Three lookups read it back:

GetNodeDefinition(int) which type this node came from
TryGetNodeInstance(int, out object?) the object its values live on
GetBinding(int) both, as a NodeBinding record

PinDefinition.GetValue/SetValue already worked and are unchanged; what was missing was the instance to point them at.

The default written is the attribute's DefaultValue where there is one, otherwise the member's own C# initializer — the same precedence PinDefinition.DefaultValue already reports, so the instance and the pin cannot disagree about the same pin. That is the only reason the write exists: an initializer-derived default is written back unchanged and does nothing.

A method node is deliberately left without an instance. The library already models a non-static method's receiver as an Instance input pin, so one arrives over a link from whichever node produced it. Manufacturing a second here would give the node two receivers that disagree, so TryGetNodeInstance answers false and the definition still resolves. The same answer covers a type with no parameterless constructor.

Why the engine gained two events

A node id only means something while the engine still holds that node, and the engine is the only thing that knows when it stops. NodeRemoved and Cleared are what let the bindings be dropped rather than aged out.

Cleared is not NodeRemoved repeated, and that is the subtle half. Clear() also resets nextNodeId to 1, so the next node created is handed an id a previous node used. Without dropping on Cleared, an unrelated node silently inherits a cleared node's definition and parameter values — which is the failure mode that would be hardest to diagnose from a host, since nothing looks wrong until a value is read.

Testing

Seven tests, 106 → 113, and each one mutation-checked rather than merely watched to pass. Every row is a separate edit to AttributeBasedNodeFactory.cs, run against the full suite:

mutation failures caught by
declared default never written (assignability guard inverted) 1 CreateNode_ConstructsAnInstanceCarryingTheDeclaredDefaults
one instance shared by every node of a type 1 CreateNode_GivesEachNodeItsOwnInstance
node removal not observed 1 RemoveNode_DropsTheBinding
clear not observed 1 Clear_DropsEveryBindingSoAReissuedNodeIdIsNotInherited
method node given a manufactured receiver 1 CreateMethodNode_BindsTheDefinitionWithoutManufacturingAReceiver
a created type node never bound at all 5 the four above it, plus GetNodeDefinition_MapsANodeIdBackToTheTypeItCameFrom
unmutated 0 113/113

The Clear test asserts the id really was reissued before asserting the binding did not come with it, so it cannot pass vacuously if that premise ever stops holding.

Verified on Linux, .NET SDK 10.0.401: 113/113 in Debug and Release. NodeGraph.Tests unchanged at 106/106, and ImGui.sln builds clean in Release under the analyzers-as-errors with 0 warnings.

A doc correction

ImGui.NodeEditor/README.md said:

registering a type gives you a node that draws, and nothing here constructs the type or invokes the method

Half of that is now false. It is corrected rather than left to mislead, and the line that still matters is restated where the new behaviour is documented: constructing the type is not executing it, and nothing here calls [NodeExecute]. The parameter-editing round trip is written up alongside it, which is also the concrete answer to #437.

Not included

This is storage and identity, not execution or UI. It does not run a graph, and it does not draw an inspector panel — PinDefinition.DataType is what a host would switch on to pick a widget, and that already exists. #437 asks which of three patterns is intended; this makes the "node properties edited through an inspector panel" one actually implementable, but choosing between them is a maintainer's call rather than something to settle in a PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Upt48c89uvSGUWF7Yjx7xk


Generated by Claude Code

Fixes #438

AttributeBasedNodeFactory read a NodeDefinition, extracted the pin display
names, and dropped everything else at the engine boundary. No T was ever
constructed, and no map was kept from a node id back to the definition it came
from, so a declared parameter such as a Threshold had nowhere to live and
nothing could answer which type a selected node was.

CreateNode now constructs the type once per node, writes each input pin's
declared default onto it, and binds it to the id the engine issued.
GetNodeDefinition(int), TryGetNodeInstance(int, out object?) and GetBinding(int)
read it back; PinDefinition.GetValue/SetValue already did the rest.

A method node is deliberately left without one. The library models a non-static
method's receiver as an Instance input pin, so it arrives over a link;
manufacturing a second here would give the node two receivers that disagree.

A node id only means something while the engine still holds that node, so the
engine gained NodeRemoved and Cleared and the factory drops bindings on both.
Cleared is not NodeRemoved repeated: Clear() restarts the id counter, so without
it an unrelated node inherits a cleared node's values when it is reissued the
same id.

Seven tests, each mutation-checked rather than watched to pass:

  mutation                                        | failures
  declared default never written                  | 1
  one instance shared by every node of a type     | 1
  node removal not observed                       | 1
  clear not observed                              | 1
  method node given a manufactured receiver       | 1
  created type node never bound at all            | 5
  unmutated                                       | 0

106 -> 113 in ImGui.NodeEditor.Tests, Debug and Release; NodeGraph.Tests 106/106
unchanged; the solution builds clean in Release under analyzers-as-errors.

The README said "nothing here constructs the type or invokes the method". Half
of that is now false, so it is corrected rather than left to mislead, and the
parameter-editing round trip is documented alongside it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Upt48c89uvSGUWF7Yjx7xk
SonarQube S4136 on the PR: the new GetNodeDefinition(int) sat with the binding
helpers, split from GetNodeDefinition(Type) and GetNodeDefinition(MethodInfo) by
CreateBackingInstance and ApplyConnectionCapacities. The three overloads are now
adjacent, with GetBinding and TryGetNodeInstance following them so every
read-back on the factory reads in one place.

A pure move: 36 lines out, the same 36 back in, no behaviour change. Release
build clean at 0 warnings under analyzers-as-errors; 113/113 in Release.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Upt48c89uvSGUWF7Yjx7xk
@sonarqubecloud

Copy link
Copy Markdown

@matt-edmondson
matt-edmondson merged commit 3ef3e70 into main Sep 23, 2026
14 checks passed
@matt-edmondson
matt-edmondson deleted the claude/node-instance-bindings branch September 23, 2026 08:27
matt-edmondson added a commit that referenced this pull request Sep 23, 2026
main landed #446, which gives each created node a backing instance its
parameters live on, while this branch gives each pin a value in the
engine's store. Three files conflicted.

- DomainModels.cs: both sides added a declaration at the same place.
  PinSpec and NodeRemovedEventArgs are unrelated, so both are kept.
- AttributeBasedNodeFactory.cs: main's CreateNode registers a NodeBinding
  and then calls ApplyConnectionCapacities. The binding is kept; the
  capacity post-pass is not, because this branch deleted that method and
  carries each pin's capacity in its PinSpec instead. CreateBackingInstance
  and ApplyDeclaredDefault come across unchanged.
- README.md: both sides documented "editing node parameters" and meant
  different stores. Both sections are kept, retitled for which store they
  describe, and a third says plainly that a declared [InputPin] is seeded
  into both and that the two do not track each other afterwards.

Whether the two stores should stay separate is left to the PR, not decided
here. 157 tests pass in ImGui.NodeEditor.Tests — this branch's 150 plus the
7 binding tests from main — and the solution builds with no warnings.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VM2ZwXwJCAGP3StQPgGwB2
matt-edmondson added a commit that referenced this pull request Sep 24, 2026
PR #446 landed on main and closed #438 with a different answer to the same
question: it constructs the decorated type once per node and binds the instance
to the node id, so a declared parameter lives on a property. This branch stores
values pin-keyed in PinValueStore instead.

Both halves are kept here and reconciled in the commit that follows. The
conflicts themselves were narrow:

- DomainModels.cs was a false conflict. PinSpec and NodeRemovedEventArgs are
  unrelated types that happened to land adjacent.
- AttributeBasedNodeFactory.cs kept main's instance bindings and this branch's
  ToSpecs, and dropped ApplyConnectionCapacities from both sides: PinSpec now
  carries each pin's capacity to the engine, so the order-matched post-pass has
  nothing left to do.
- README.md took main's parameter-editing section, since that is what the code
  currently does. The unification rewrites it.

Leaves two homes for a parameter value, which is the thing the next commit
fixes. Recorded rather than hidden because the merge is what created it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node instances are never created or tracked, so parameter state has nowhere to live

2 participants