-
Notifications
You must be signed in to change notification settings - Fork 5
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
Level0 #23
base: extensions/KHR_behavior
Are you sure you want to change the base?
Level0 #23
Conversation
Some comments:
Also in terms of execution model, Logic and Queries do not have "Flow" inputs or outputs, rather they are evaluated based on pull. I call them "ImmediateNodes" in my reference implementation. "Events" only have "Flow" outputs and no "Flow" inputs. "Actions" and "Flow Control" nodes have "Flow" inputs and outputs. |
If you would like @elalish we could do a call and I could explain some of the complexities of the Blueprints execution model including how Async work, and how Sequence/For-Loops work. It is relatively complex in the details. Specifically, Sequence node actually waits for the downstream sync execution (but not async) for each of its output nodes to complete before it fires the next one. I implemented this in behave-graph via passing in a callback when triggering an output flow socket: https://github.com/bhouston/behave-graph/blob/main/packages/core/src/Profiles/Core/Flow/Sequence.ts#L43. And here is the for-loop behavior for the loop body that has similar behavior - it waits for the loop body to complete execution before firing again:https://github.com/bhouston/behave-graph/blob/main/packages/core/src/Profiles/Core/Flow/ForLoop.ts#L40 And then I implemented this in the execution engine such that each sync execution thread has a stack of callbacks of which the top one will fire when there is nothing scheduled to execute. I implement async behavior by having separate fibres. Each node that has async execution I merely create a new fiber to handle the execution of that flow output socket: https://github.com/bhouston/behave-graph/blob/main/packages/core/src/Execution/Engine.ts#L40. You can see the "Delay" node makes use of async execution here when it fires: https://github.com/bhouston/behave-graph/blob/main/packages/core/src/Profiles/Core/Time/Delay.ts#L45 |
Also proposed in this PR is the removal of the need for deterministically ordered execution. The lack of order of execution can be inferred by the specification on flow input sockets of which nodes they connect to.
This means that if multiple flow nodes connect to the same predecendor node, it isn't clear which subsequent node will execute first. I would suggest that we need links between flow nodes to be fully determinist in terms of sequencing. I know for layer 0, there is no state, but if there is mutable state, deterministic execution is a must because when you set state and then check it is incredibly important. As such I recommend that when one flow node links to multiple subsequent nodes, we always use a sequence node that deterministically orders the execution of the subsequent nodes. Even if your user facing tool does not care about ordering, it can automatically put in sequence nodes that specify the ordering without affecting anything. This way the execution model remains the same between layer 0 and layer 1. Otherwise, we should split the specification into two -- because layer 0, if it is no longer deterministically ordered -- is not a subset of layer 1. Rather it is a different execution model with relaxed constraints, and it will have a different JSON format for the nodes graphs. |
@bhouston Thanks, very helpful feedback! I think the main disconnect is that I failed to write up a section for immediate nodes (since Level 0 may not need them). My idea wasn't to wedge them into Part of my reasoning here is the separate sections for "value" vs "flow" inputs and outputs in the original spec. What I saw is that flow, actions, and async all go together, while separately values, parameters, immediate nodes, queries, and deterministic ordering go together. I think making that categorization explicit in the format will make it easier to spec. Before I make any changes, I'm going to add a section on "functions" (actual name TBD, but representing math nodes, immediate nodes, queries, etc), because I want to make sure we're all on the same page about what this proposal is before I start incorporating feedback. |
|
||
### Security requirements for Level 0 | ||
|
||
Replacing constant parameters with `functions` is allowed in general across Level 0, as long as all functions are pure, stateless, and side-effect-free. Like `actions`, `functions` must only reference indices less than their own to prevent recursion. They do not impact the fundamental restriction that the Turing machine can only advance via explicit user activation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this means Level 0 can incorporate most of what we've been describing for Level 1: it comes down to just a restriction on recursion (by index) and a requirement for user activation.
|
||
Each `function` is synchronous and a given `function` graph can be thought of as a call stack, beginning with an `action` parameter and calling each parameter `function` in turn up the stack until it terminates at either a constant or a query. The stacks are then unwound down to produce the final requested parameter `value`. | ||
|
||
Execution is deterministic because the `functions` are all synchronous and pure, meaning they have no state and no side-effects. They cannot affect the scene states they are querying directly: only `action` nodes can do that. A `function` graph runs before the `action` that calls it via its parameter list, and after that `action`'s input `action` completed. This means if both of these actions are changing the same scene state value, that value will be deterministically in the final state of the input `action` when queried. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bhouston Is there any piece of order determinism I've missed here?
// Door activation behavior | ||
{// 0 | ||
"name": "Activate Door", | ||
"type": "active/select", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to leave the category as part of the type simply because I want to make it easy to inspect that the Level 0 rules are followed. This way a verifier doesn't have to go back to the spec (and any extensions) to create of list of allowed types. I'm not attached to any of the names though.
], | ||
"parameters": { | ||
"if": { | ||
"function": 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like the $operation
syntax below. I'm using different names mostly to disambiguate until we come to consensus.
|
||
The `internal` actions are categorized into `stateful` and `stateless`. Here the meaning of `stateful` is that the action maintains internal state that changes the flow of the graph from one invocation to the next. This includes geometric and visibility properties of the scene, like animations, because those changes could cause `passive` `trigger` actions to fire automatically. On the other hand, `stateless` actions may only affect the visual state of the glTF scene (material properties), which is not readable by any Level 0 actions. | ||
|
||
The `input` to `internal` actions is a list, where any `action` in the list completing will start this `action`. Additionally, some `stateful` actions have multiple outputs, only one of which is fired upon completion for a given invocation. The desired output of an action can be selected with the optional `output` parameter of the `input` item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bhouston I agree on using implicit anyOf
; I've updated that here.
Not sure the best way to collaborate on changes, so figured PR into a PR? Anyway, here is my first shot at a Level 0 spec, but really an adjustment to what was already written. Tried to incorporate the various things we've been discussing and simplify as much as possible. I think all the Level 1 stuff should fit fine in this framework too, but would love to know if there's a conflict.
Let's try to keep comments in the code-review section so we can keep track of separate threads; the main comment section becomes a mess far too easily.