Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

account for node configuration in graph editor #237

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rlidwka
Copy link

@rlidwka rlidwka commented Oct 4, 2023

The objective of this commit is to correctly render graphs like these: https://github.com/bhouston/behave-graph/blob/3e6568caa9a46a6f6aee27b0192a478f30c9c541/graphs/core/flow/WaitAll.json

This is how that graph is displayed now (you can copy-paste it to verify that this PR is not useless):

image

When loading JSON, a node may have arbitrary configuration parameters, that previously got ignored (such as number of sequence outputs).

This commit tracks configuration for each rendered node, and accounts for it in all calculations (such as adding new edge).

In order to do this, instead of NodeSpecJSON (object) we now pass a NodeSpecGenerator (class) which generates node specs on demand based on passed configuration (and also memoizes, not sure if that's necessary). Note: instead of it, we can pass IRegistry around, and run createNode() whenever we need to access inputs. I don't know why I decided that memoization of node creation is a good idea.

New nodes will still be created from UI with no configuration, and no UI work to support any configuration editing has been done.


edit:

I've also added Unreal-like "Add socket" button in the second commit, that is intended to be a demonstration of how you can configure these nodes in UI:

image

This is not a comprehensive solution by any means, this is just a starting point for someone who wants to actually implement UI.

The objective of this commit is to correctly render graphs like these:
https://github.com/bhouston/behave-graph/blob/3e6568caa9a46a6f6aee27b0192a478f30c9c541/graphs/core/flow/WaitAll.json

When loading JSON, a node may have arbitrary configuration parameters,
that previously got ignored (such as number of sequence outputs).

This commit tracks configuration for each rendered node,
and accounts for it in all calculations (such as adding new edge).

In order to do this, instead of NodeSpecJSON (object) we now pass
a NodeSpecGenerator (class) which generates node specs on demand
based on passed configuration (and also memoizes, not sure if that's
necessary).

New nodes will still be generated with no configuration,
and no UI work to support it has been done.
@SYBIOTE
Copy link
Contributor

SYBIOTE commented Oct 4, 2023

so in theory , if one makes a config editor UI, then the nodes will reactively update as the config changes ,
can this also be extended to reactively change a node when we pull in a specific input to it?

@SYBIOTE
Copy link
Contributor

SYBIOTE commented Oct 4, 2023

the PR breaks node addition, as the data value in the new node added is null

@SYBIOTE
Copy link
Contributor

SYBIOTE commented Oct 4, 2023

I propose adding code responsible for filling the configuration when adding a new node with the default values in the handle add node method

@rlidwka
Copy link
Author

rlidwka commented Oct 5, 2023

the PR breaks node addition, as the data value in the new node added is null

I fixed it in the last force-push, please check again:
https://github.com/bhouston/behave-graph/compare/8c6deaae5461393b7f5d1c6c60bea426f7ac1a7c..3d2383d23869871bfea0f78f2fc61aa55427d263

@SYBIOTE
Copy link
Contributor

SYBIOTE commented Oct 5, 2023

Ok i will

@rlidwka
Copy link
Author

rlidwka commented Oct 8, 2023

here's all existing custom configurations:

  • customEvent/onTriggered: customEventId (string)
  • customEvent/trigger: customEventId (string)
  • flow/sequence: numOutputs (number)
  • flow/switch/integer: numCases (number)
  • flow/switch/string: numCases (number)
  • flow/waitAll: numInputs (number)
  • variable/get: variableId (number)
  • variable/set: variableId (number)

so still need some UI for variableId and customEventId

and use generic interface to add new sockets for all of them
@rlidwka
Copy link
Author

rlidwka commented Oct 8, 2023

I've renamed flow configuration (numInputs/numOutputs/numCases) to a single numSockets in order to avoid "hardcodedness" a little bit.

Now every node that has "numSockets" configuration of a "number" type gets a plus button.

There's still no custom UI for events and variables.

@SYBIOTE
Copy link
Contributor

SYBIOTE commented Oct 8, 2023

Hmm that's good, but then again, now we cannot distinguish between nodes which only need multiple input sockets, ones which need only multiple output sockets and ones which need both

@rlidwka
Copy link
Author

rlidwka commented Oct 9, 2023

Hmm that's good, but then again, now we cannot distinguish between nodes which only need multiple input sockets

We don't need to distinguish between those now. The UI is the same for both.

Yes, it's a bad practice to tune data structures to UI needs, so better ideas are certainly welcome, especially if they come with corresponding pull requests.

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.

None yet

2 participants