bonsai-rx / bonsai

The compiler, IDE, and standard library for the Bonsai visual programming language for reactive systems
https://bonsai-rx.org
MIT License
137 stars 29 forks source link

Disable editing of properties of nodes inside included workflows in the property editor #899

Open glopesdev opened 2 years ago

glopesdev commented 2 years ago

Although included workflows are read-only and cannot be modified, the property grid still allows editing of internal properties. For consistency and to prevent accidental misunderstanding all internal operators of included workflows should be marked with the read-only attribute in the property grid.

glopesdev commented 2 years ago

Following subsequent testing and evaluation of this feature during rapid-prototyping of real applications, there is a case to be made that it might be premature to close this possibility right now. Although only externalized properties are serialized in the workflow, it might be necessary on occasion to change temporary parameters inside the nested workflow for calibration purposes.

Relevant cases requiring this capability:

  1. Property editors for computer vision modules, such as ROI calibration or warp, often require as their input context the original image stream, and might not work when externalized and manipulated from outside. There is a case to be made here for allowing reversion of editors to internal node sources (c.f. #863), but this will inevitably be a longer discussion.
  2. When the included workflow encapsulates cold-sources such as FileCapture, it might be useful to temporarily manipulate video position for calibration purposes of other downstream properties, even though we might not want to ultimately expose such properties in the reusable include workflow interface.

Possible workarounds and design considerations:

glopesdev commented 2 years ago

Another possibility to resolve 1. would be to revert all property editors to always use the immediately proximal context even when externalized in a group workflow. While this would guarantee the applicability of the editor, it does limit severely the existing possibility of expanding the editing context.

Another existing related approach to expand editing context is to create property sources from the relevant properties, connect the desired context input to them, and map their output to the original node. Since editors are carried over from original properties to property sources, the editor is still available while the input is effectively redirected. The redirected input in this case can be used by the editor even if it only relies strictly on proximal context.

The main disadvantage of relying on such an approach is that one has to pay the cost of constantly assigning properties even when not editing, since value propagation depends on mapping the output of the property source to the original node, and the property source has to be triggered by the context input for the editor trick to work. However, this might only be relevant in a minority of cases, and there is room for future language feature improvements that would make it possible to provide the context without necessarily triggering value propagation.

glopesdev commented 1 year ago

There is an easier criterion to adopt that already would reduce the most significant structural problems with editing include workflow implementations, which is to assume that properties marked with the DesignOnlyAttribute are not possible to edit when nested inside an include workflow.

While it does require manual annotation of properties, it already would prevent changing the path of included nodes inside included workflows for example, which can be really confusing to debug if spread across different instances of the same include. This already works well for design vs runtime editing so it shouldn't be hard to generalize.

jerlich commented 1 year ago

FYI - I think we experienced a related issue today. We didn't notice that a workflow was a read-only extension and tried to edit a property that was not externalised. We could edit the property in the editor, but there was no change to any file on saving. This led to some confusion.

glopesdev commented 1 year ago

@jerlich Indeed I agree this discussion is not closed and leads to confusion, so will reopen.