DYN-5125: Fix for modified input nodes that are assigned non-primitive values

[aparajit-pratap]

Purpose

Fixes a regression caused by #12835 where an input selection node wasn't reflecting updated values when the selection was changed. This was because there was a force re-execution on selection change, not a modified delta execution and the node was assigned a non-primitive value and the original design in #12835 didn't account for cases where an input node would be force re-executed instead of re-executed because of modification and neither did it account for modifications of non-primitive values. In the case of a primitive value, since the new value can be directly obtained from the AST, the register is updated in situ without actually executing instructions to push the updated value and pop it to the register. However, in the absence of a primitive value in the AST, the updated stackvalue can only be obtained from the updated VM run. Without this, the register dictionary (DSASM.Executive.updateRegisters) is also not updated with the new stackvalue and leads to an obsolete result. The fix is to skip the instructions only for the case where the input node is being assigned a primitive value and not otherwise.

Declarations

Check these if you believe they are true

• The codebase is in a better state after this PR
• Is documented according to the standards
• The level of testing this PR includes is appropriate
• User facing strings, if any, are extracted into *.resx files
• All tests pass using the self-service CI.
• Snapshot of UI changes, if any.
• Changes to the API follow Semantic Versioning and are documented in the API Changes document.
• This PR modifies some build requirements and the readme is updated

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

[mjkkirschner]

I'd like to understand this change better, but I'm not familiar with much of this code - could you expand on what the updateRegisters are used for?

is there a way to transform the force update of input nodes into a mock modification? What are the big differences between a force update and a modification?

[aparajit-pratap]

I'd like to understand this change better, but I'm not familiar with much of this code - could you expand on what the updateRegisters are used for?

@mjkkirschner the updateRegisters dictionary is a dynamic data structure to hold the values of input nodes that are modified in delta execution. They are not real registers but represent the concept of a VM register that stores a stackvalue. The following is an example of the instructions generated for an input node:

e.g. [a = 1] where a represents an input
---------------------------------------------
push 1
pop lx	(register)
-------------	⇒ No graphnode
push lx
pop a
dep a               ⇒ graphnode
---------------------------------------------
=> [a = 2] (delta update to the input)
---------------------------------------------
update register to 2 directly instead of emitting new instructions to push the value of 2 to lx and update it that way

Since we can have multiple input nodes at a time, I cannot use a single register (called lx in this example), so I decided to use a dictionary with key = AST ID of the node, value = stackvalue. Now, whenever an input node with a unique AST ID, executes, its stackvalue is either added to the dictionary or updated in the dictionary if it's an update.

[aparajit-pratap]

is there a way to transform the force update of input nodes into a mock modification? What are the big differences between a force update and a modification?

In a delta modification, the modified subtrees/ASTs are identified by the change set computer. Among those modified ASTs, I was identifying the input nodes, and directly updating the update-registers to the RHS of the modified AST. This is because, in #12835, I assumed that the RHS of input nodes will only be primitives, so I can do this update by directly looking at the AST and getting the value, then updating the register.
Then this FormIt case came along where the input node is a FormIt selection node (NodeModel node) and is internally a call to Geometry.ImportSAT(filename). Therefore, the RHS of its AST is not a primitive but a return value from a function call, which is only known at runtime. In this case, when the selection was being changed, it wasn't being identified as a modified AST by the change set computer because the SAT file name is unchanged by FormIt's selection node. FormIt simply swaps out the geometry from underneath keeping the same file name. However, the node was still executing since it was marked for force-execute. In neither the "force-re-execute" cycle, nor the modification cycle, can I check for the graphnode's updated value unless it's actually computed in the VM and returned, and only then can I update the input register. Therefore I'm marking this graphnode ahead of time, then tracking its execution, and finally updating the register in the pop instruction when its value is popped. Then in the subsequent push instruction when the register is pushed for assigning to downstream symbols, I can ensure that the register has the updated value. To fix this, I no longer skip executing the instructions to push the updated value and pop it to the register. These are skipped only for primitive values.

[mjkkirschner]

In this case, when the selection was being changed, it wasn't being identified as a modified AST by the change set computer because the SAT file name is unchanged by FormIt's selection node.

Is there a way to mark all nodes that are force re-executed as changed in the change set computer?

[aparajit-pratap]

In this case, when the selection was being changed, it wasn't being identified as a modified AST by the change set computer because the SAT file name is unchanged by FormIt's selection node.

Is there a way to mark all nodes that are force re-executed as changed in the change set computer?

I don't think it really matters in this case. Even if we consider these force-reexecuted nodes as modified nodes, we'll still have to track the graphnode identified at this stage all the way up to the pop instruction where we can get the stackvalue returned and update the register. This is why I had a TODO in the PR description to do the same thing I'm doing for force-reexecute for modified nodes.

[mjkkirschner]

@aparajit-pratap awesome! Is there a way to test this?

[mjkkirschner]

LGTM - test would be good though for the formit regression case.