Inspector / Editor for Nodes about ryven HOT 13 CLOSED

Expanding undo-redo beyond the flow will require major changes (and probably higher complexity) so I think I would prefer shelving that for now.

Not really, unless I missed something. I've implemented this undo functionality. My changes are:

• A multi-selection system that:
  • Selects a node immediately if mouse pressed happens on it
  • Waits until mouse release if the press happened on canvas for multi-selection
  • Provides a SelectComponents_Command for undo / redo. I noticed that all the nodes are cached, which means "deletion" doesn't really remove memory. Adding nodes takes around ~0.5MB minimum, which I find quite a lot, but I do know that it's hard to "fix" this, if it ever is fixable
  • Ignores the qt selection state changes that happen when creating or deleting nodes
  • I also included a Delegate_Command for creating undo / redo via callbacks
  • Fixes a bug where deleting nothing added undo delete commands.
• An 'InspectorGUI' module that provides the basics for an inspector, such as an InspectorWidget where a BaseNodeInspector resides in. The implementation is (and should remain?) Ryven agnostic

Using traits, I built a simple example that I think showcases the possibilities. The inspector itself doesn't really serialize anything, or at least that's hidden under the traits library. Here's the code for the example:

from random import random, Random
from ryvencore import Node, NodeInputType, NodeOutputType, Data
from ryven.node_env import export_nodes, on_gui_load
from traits.api import HasTraits, Str, Int, Range, Enum, observe, Any, File, Instance, Float, Bool, Event


class RandNode(Node):
    """Generates scaled random float values"""

    class RandNodeConfig(HasTraits):
        # auto_set, enter_set allow the any_trait_changed to be called only when pressing enter
        # to play nicely with undo / redo
        scale: float = Float(1, auto_set=False, enter_set=True)
        use_seed: bool = Bool
        seed: int = Int(0, auto_set=False, enter_set=True)
        generate = Event
        
        _node = Instance(klass=Node, visible=False)
        traits_view = None  # must be included if one wants to use the observe decorator
        on_trait: list = []
        on_val: list = []
        ran_gen: Random = Random()

        # @observe('*') the decorator doesn't allow removal of notifications
        def any_trait_changed(self, event):
            node: RandNode = self._node
            # generates a new random when pressing the button
            if event.name == 'generate':
                prev = node.val
                node.update()
                for e in self.on_val:
                    e(prev, node.val)
            
            # state change events (scale, use_seed, seed)
            else:
                for e in self.on_trait:
                    e(event)

        def allow_notifications(self):
            self.observe(self.any_trait_changed, '*')

        def block_notifications(self):
            self.observe(self.any_trait_changed, '*', remove=True)

    title = 'Rand'
    tags = ['random', 'numbers']
    init_outputs = [NodeOutputType(label='out')]

    def __init__(self, params):
        self.config = RandNode.RandNodeConfig()
        self.config._node = self
        self.config.allow_notifications()
        self.val = None
        super().__init__(params)
    
    def place_event(self): # must be here (there should be one output value)
        self.update()
        
    def update_event(self, inp=-1):
        result = self.config.scale * (
            random() if self.config.use_seed else self.config.ran_gen.random()
        )
        self.val = result
        self.set_output_val(0, Data(result))

from .nodes import *
from ryven.gui_env import inspector_gui
from ryvencore_qt.src.flows.nodes.InspectorGUI import NodeInspector
from traitsui.api import View, Item, ButtonEditor, Group
from ryvencore_qt.src.flows.FlowCommands import Delegate_Command
from collections import deque

@inspector_gui(RandNode)
class RandNodeInspector(NodeInspector):
    @property
    def config(self):
        return self.node.config

    def attach_inspector(self, parent):
        self.node: RandNode = self.node  # help with auto-complete

        view = self.config.trait_view()
        # This could be None, but wanted to a label and a border
        view = View(
            Group(
                *tuple(Item(name) for name in self.config.visible_traits()),
                Item("generate", show_label=False, editor=ButtonEditor(label="Generate!")),
                label="Config",
                show_border=True,
            )
        )
        self.ui = self.config.edit_traits(
            parent=parent.layout(), kind='subpanel', view=view
        ).control
        self.config.on_trait.append(self.on_trait_changed)
        self.config.on_val.append(self.on_val_changed)
        parent.layout().addWidget(self.ui)

    def unload(self):
        self.config.on_trait.remove(self.on_trait_changed)
        self.config.on_val.remove(self.on_val_changed)
        self.ui.setParent(None)
        super().unload()

    def on_val_changed(self, prev_val, new_val):
        def undo_redo(value):
            def _undo_redo():
                self.node.set_output_val(0, Data(value))
            return _undo_redo
        
        self.flow_view._push_undo(
            Delegate_Command(self.flow_view, (undo_redo(prev_val), undo_redo(new_val)))
        )
    
    def on_trait_changed(self, trait_event):
        print(f'Trait "{trait_event.name}" changed from {trait_event.old} to {trait_event.new}')
        # otherwise an enter event for a text editor wouldn't stop text editing
        self.flow_view.setFocus()
        if trait_event.name == trait_event.new:
            return

        def undo_redo(value):
            def _undo_redo():
                self.config.block_notifications()
                setattr(self.config, trait_event.name, value)
                if trait_event.name == 'seed':
                        self.config.ran_gen.seed = value
                self.config.allow_notifications()

            return _undo_redo

        self.flow_view._push_undo(
            Delegate_Command(self.flow_view, (undo_redo(trait_event.old), undo_redo(trait_event.new)))
        )

This inspector could potentially be just a base class for any node that uses traits for configuration. The undo / redo are registered via callbacks and the Delegate_Command. Here's an example video as well.

Undo happens around the ~23s mark. Ignore the dots in the connections, it's something I'm testing for visualizing active data flows (for my own purposes, not Ryven). I could open a PR if this interests you in any way.

EDIT: I had implemented the RAND undoing wrongly, I uploaded a new one. Notice that even the rand node's output value is marked for undoing.

from ryven.

Thanks for opening this. I agree this is one of the most important missing pieces. Fist some high-level comments:

I'd like to find an implementation that prevents most misuse by design. I realize for most class-based approaches in Python this is generally rather difficult.

The system should be constrained enough to be able to serialize everything automatically IMO. I imagine exposed properties would change often, and offloading de-/serialization to the user might turn out to be a huge pain.

4. Allow for explicit creation of a gui. This will override the default generic editor, allowing someone to do anything they want.

Would be a nice addition I think, though not necessarily part of this very issue. Exposed node properties would be part of a node (and should exist in headless mode), while a custom widget wouldn't and should therefore only provide a more convenient interface to those properties.

This motivates the option of declaring properties as hidden (not exposed through a default GUI in inspector) though.

from ryven.

Now some implementation ideas. My naive approach would be literal JSON-compliant dicts. However, exposing various properties is only economical as long as there are fine-grained callbacks. Idea:

class ObservableDict:
    def __init__(self, data: dict = None):
        if data is None:
            data = {}
        self._data = {
            key: self._wrap_value(value) 
            for key, value in data.items()
        }
        self._callbacks = {}

    def _wrap_value(self, value):
        if isinstance(value, dict):
            return ObservableDict(value)
        elif isinstance(value, list):
            return [self._wrap_value(item) for item in value]
        else:
            return value

    def __getitem__(self, key):
        return self._data[key]

    def __setitem__(self, key, value):
        wrapped_value = self._wrap_value(value)
        self._data[key] = wrapped_value
        if key in self._callbacks:
            self._callbacks[key](wrapped_value)

    def attach_callback(self, key, callback):
        self._callbacks[key] = callback

    def remove_callback(self, key):
        if key in self._callbacks:
            del self._callbacks[key]



if __name__ == '__main__':

    def callback1(value):
        print('callback1', value)
    
    def callback2(value):
        print('callback2', value)
    
    def callback3(value):
        print('callback3', value)
    

    data = ObservableDict({
        'first': 1, 
        'second': {
            'second.first': [
                1, 2, 3
            ],
            'second.second': 4
        }
    })

    data.attach_callback('first', callback1)
    data['first'] = 2
    # -> callback1 2

    data['second'].attach_callback('second.first', callback2)
    data['second']['second.first'][0] = 5       # no callback!
    data['second']['second.first'].append(6)    # no callback!
    data['second']['second.first'] = [7, 8, 9]  # callback! because of assignment
    # -> callback2 [7, 8, 9]
    
    data['second']['second.second'] = 10

Advantages:

• very simple and we can easily make sure only valid data types are stored
• clearly, saves us complex serialization as Ryven saves in JSON format

Drawbacks:

• as you can see, mutating the list doesn't trigger callbacks, but I guess we could wrap all JSON-compliant types (that is dict, list, str, int, float, bool, None) analogously
• it is not so clear to me how to to nicely add callback connections to the data when serializing
• there are a few types that might be really useful (such as a set) which are not supported by JSON directly

from ryven.

Hello! As with any new feature that needs implementing, I believe the first thing to do is an exhaustive research of what already other open-source (or closed if need be) exist. Before diving into a custom implementation, I stumbled upon the open-source Traits and TraitsUI packages, which seem to do exactly what we want.

https://docs.enthought.com/traits/
https://docs.enthought.com/traitsui/

By utilizing them, we might as well have a system ready in a few (or even one) days! From what I can see:

1. Should be utilized in ryven, not ryvencore. If someone has used it to implement a nodes package, then it automatically translates to an inspector for ryven as well. If they don't, then the inspector / details section will display a message that there is no GUI for this node.
2. It allows for various views (in our cases, "details") to be easily implemented, along with all kinds of callbacks. We just have to decide on where we will export them from.
3. Might even allow automatic json serialization (this can be package specific, so I believe it isn't needed in ryvencore) This may come in handy as a default serialization utility.
4. I believe it could be used for checking connection validity as well, but I'm just mentioning it, haven't really checked it out.

The only "down-side" is that it slightly changes the way someone typically defines instance attributes of a class. Please check it out before delving into a more custom solution. For me it seems like a no brainer. ( I will definitely be using it to extend Ryven, so if you agree, we can decide on some guidelines on how to implement it properly for the main repository)

from ryven.

Thinking this through a bit more, I came to the conclusion that perhaps it's better to avoid defining a specific implementation of the above or rely on specific libraries and modules.

The way I see it, one can rely on the traits library, which has this implemented already. Another could attempt to create his own implementation from scratch, or by relying on other similar modules or libraries, such as the built-in dataclasses or pydantic (unlike traits, both are missing a UI implementation). All of these provide ways to declare "dataclasses" and allow for object nesting as well.

This can be done because an inspector is an isolated feature living in its own window / area. Ryven only needs to provide some kind of entry point to that area. All in all , I believe the API should simply allow registering some kind of function or class that attaches the inspector to that area. I propose something like this:

class Inspector(ABC):
    """Abstract class for an inspector. Requires the node."""
    
    def __init__(self, node):
        self.node = node
    
    @abstractmethod
    def create_inspector(self, parent: QWidget) -> QWidget | None: # don't mind 3.10+ union
        """
        Creates the inspector. The parent is also provided
        If the function returns None, it means parenting has been applied here
        Otherwise, it must be applied externally within Ryven
        """
        ...

@inspector_gui(SomeNode)
class SomeNodeInspector(Inspector):
    
    def create_inspector(self, parent):
        ...

If the inspector doesn't exist or some exception is thrown, we simply show a "No inspector gui provided" in the inspector area.

I believe this keeps it super simple but open to any implementation. Granted, Ryven can't test that the implementation is correct, but only allows for an entry point to be accessed. What are your thoughts?

from ryven.

And to provide an example using the traits library, we could have something like this:

class SomeNodeConfig(HasTraits):
    config_one: str = Enum('choice1', 'choice2')
    intensity: int = Range(low = 0, high = 15)
    ...

class SomeNode(Node):
    def __init__(self):
        super().__init__()
        self.config = SomeNodeConfig()
    ...

@inspector_gui(SomeNode)
class SomeNodeInspector(Inspector):
    
    def create_inspector(self, parent):
        self.node.config.edit_traits(parent=parent) # creates the gui and applies it to the parent
        return None

I chose to separate the config from the Node but I guess one could also do it with multiple inheritance.

from ryven.

the first thing to do is an exhaustive research of what already other open-source (or closed if need be) exist

side note ¹

While you opened this issue specifically for an inspector view, I would like to move away from the idea that this is only about in-editor UI. As I noted in my PR comment

The concept of a mutable tree of properties could be very useful beyond the UI.

On an abstract level I think we're trying to define some sort of structured, (partially) mutable property tree for nodes, which presents an interface to modify the node's behavior. I can think of various scenarios beyond an inspector UI

• a remote control UI for modifying node properties while Ryven is running headless somewhere else
• non-trivial data flow via data dependency between nodes through properties (bad idea I think, but it's an idea)
• standardized way of sharing metadata between nodes from different authors
  • some node properties of nodes in nodes package $A$ could provide metadata about the structure of the data generated, which nodes from package $B$ can use to handle inter-package compatibility
• machine-interpretable interface for automated testing and fuzzing
• ...

whichever library we may use or not use in the end, I'm trying to define the scope and requirements for this node properties idea first. We can move this to a new issue and keep it UI here, if you prefer.

from ryven.

A mutable tree of properties is well represented by a json structure.

standardized way of sharing metadata between nodes from different authors

I believe this is very important. By having a standard way of providing metadata, one could actually export packages' nodes, data and connections and feed them to another node-making UI. For example, we could have a web-page for creating a flow. Granted, it might not run and miss the Node GUI, but it might allow for creating the graph and sending it to a service for running. Writing this, I remembered that ryvencore doesn't currently have an API for checking for allowed port connections (i.e, allowing only a specific type of Data to connect to an input port). Such metadata could also help. This is static metadata though.

For configurable properties, there are a lot to consider. If a requirement such as this exists:

a remote control UI for modifying node properties while Ryven is running headless somewhere else

, then configurable data must be serialized with additional metadata (i.e a specific property is of type int) such that a configuration UI could be built outside of language and program restrictions, such as the web-page above.

non-trivial data flow via data dependency between nodes through properties (bad idea I think, but it's an idea)

I too think it's a bad idea to pass data between nodes in any other way than port connections. This is more typical of a game engine architecture.

side note 1

I'm trying to define the scope and requirements for this node properties idea first. We can move this to a new issue and keep it UI here, if you prefer.

Though I do partially agree, I had the requirements already set in my head when I started searching. Property configuration, serialization and deserialization isn't something new and has already been implemented across many domains and especially in game engines. Correct me if I'm wrong, but what you're proposing is an additional layer of metadata such that the properties can be configured AND validated outside of the python domain, by means of a generic API - such as a json DOM parser - with the help of the metadata.

Do you think we can separate the UI and the serialization? Can we allow the users to define their own Inspector UI for mutable properties with whatever library they are deeming correct and only require that those properties are serialized in a specific way when the project is saved? (essentially, property configuration and serialization might differ) Would that satisfy the requirements you have? Or do you think the two are tightly coupled?

If possible, I would like to separate the two, even though I find it adds a layer of complexity that might not be needed. At least, for my current requirements, an entry point for an Inspector UI is all I need and probably what most people would need most of the time.

from ryven.

I have been working on this and as a result, I've implemented redo-undo functionality for selecting a node as well, to be able to move back across selections. It took some time and effort. Would you be open to the idea that panning should work with the scroll button instead of the left? I just noticed that many times, left-clicking to open the nodes menu doesn't work because I've accidentally panned a bit by moving the mouse. I think it's more intuitive to handle panning and zooming through the same control (scroller).

Furthermore, I believe the self.mouse_event_taken isn't really needed inside the classes. We can simply check if there is an item where the event took place and set it directly in the view class. If there is, let the item handle the event. If I'm missing something, do tell.

All in all, I want this issue to result in a PR that:

• Provides an entry point for an inspector UI, as well as an example package using the traits library.
• Provides an undo-redo mechanism for item selections, as well as multi-selection when dragging.
• Provides an undo-redo delegate mechanism for connecting with node properties changes.
• Pan + Zoom done by scroll.
• Right clicking only opens the menu.
• Mouse press and double click events are guarded by checking if there is an item where the press happened, setting the mouse event taken flag.

from ryven.

By having a standard way of providing metadata, one could actually export packages' nodes, data and connections and feed them to another node-making UI.

Maybe there is a misconception, I didn't mean to fully standardize nodes, just properties that also an inspector would show.

Correct me if I'm wrong, but what you're proposing is an additional layer of metadata such that the properties can be configured AND validated outside of the python domain, by means of a generic API - such as a json DOM parser - with the help of the metadata.

I'm not entirely sure if we're talking about the same metadata, I consider it a node-individual interface, that ideally naturally translates with some standard format such as JSON.

Do you think we can separate the UI and the serialization?

The properties and the inspector UI should definitely be separated. Both need serialization. It's the same as nodes vs. node guis.

Can we allow the users to define their own Inspector UI for mutable properties with whatever library they are deeming correct and only require that those properties are serialized in a specific way when the project is saved? (essentially, property configuration and serialization might differ) Would that satisfy the requirements you have? Or do you think the two are tightly coupled?

I'm not sure I understand. Within Ryven the user will have to use the same Qt as the rest of the app.

I'll try to come up with a rough example of what I have in mind.

from ryven.

All in all, I want this issue to result in a PR that:

Are you sure you posted this in the right place? I don't see how this is related, maybe I'm misunderstanding this issue

Would you be open to the idea that panning should work with the scroll button instead of the left?

Unfortunately not, it was like this long ago and I changed it because that makes it useless for any device with a touch pad, and incompatible for stylus pens which adopted the left button convention.

Furthermore, I believe the self.mouse_event_taken isn't really needed inside the classes. We can simply check if there is an item where the event took place and set it directly in the view class.

Are you talking about FlowView.mouse_event_taken? Having an item at the position of an event doesn't mean the item consumed it. There's lots of items that don't. While Qt has an event accept system I think it didn't work as expected when working with proxies and stuff. I don't have all exact reasons for it at the top of my head but I remember this flag has good purpose.

from ryven.

Are you sure you posted this in the right place? I don't see how this is related, maybe I'm misunderstanding this issue

I continued the discussion having in mind this issue would only be about the Inspector and that we would move the discussion about the node-specific property tree in another issue. I may have been ahead of myself, sorry, been working long hours. The first 3 things I mentioned are important to the Inspector, the rest were just me playing around with Ryven, looking what works best.

Ideally, one should be able to undo changes made in the inspector. For better user experience, that needs an undo mechanism for the selections that the user does. That's all that comment was about.

Maybe there is a misconception, I didn't mean to fully standardize nodes, just properties that also an inspector would show...

I know. I was merely talking about the fact that the inspector and the node serialization ideally could ideally pass through the same system. Anyway, would you like to make this Inspector only and continue with it in another issue you'll open? I'm saying this because I have a working version of what I'm thinking and perhaps you'd like to see this first before we continue anything else.

from ryven.

Ideally, one should be able to undo changes made in the inspector. For better user experience, that needs an undo mechanism for the selections that the user does.

That is correct, but continuing that idea leads to integrating basically everything into undo-redo. While not impossible this is hard in an application where you cannot just store the state every time anything changes. Expanding undo-redo beyond the flow will require major changes (and probably higher complexity) so I think I would prefer shelving that for now.

Selection undo could still make sense (reminds me that "pure dark" theme needs a better highlighting of selected nodes)

I was merely talking about the fact that the inspector and the node serialization ideally could ideally pass through the same system.

Oh yes certainly.

Anyway, would you like to make this Inspector only and continue with it in another issue you'll open?

Let's keep this inspector (UI) only, I'll open a new issue. I would still be happy to take a look at your system soon.

from ryven.