Closed HeftyCoder closed 9 months ago
Awesome, this has a lot of great stuff! The undo history tab has been on my TODO for like two years and I never went and actually did it.
Is it possible for you to pull out all the inspector stuff from this PR (via git rebase, revert, cherry-pick and whatnot)? I would like to do that separately. Everything else is fixing stuff and improving Flow UI as far as I can see.
I have a few thoughts on the rest. Since there's a lot, I can offer to have a zoom/teams call instead of discussing everything here, which might save a lot of time. But if you prefer to keep it here, that's fine for me.
Is it possible for you to pull out all the inspector stuff from this PR
The base inspector stuff is only a single file addition which I think is already quite intuitive and ready to use. The only other thing it affects is flow_ui.py
where it is added. I would like to keep this here if possible. I can remove the example which uses traits if you want. Ryven doesn't depend on that. So, if you don't find any massive faults with it, I'd like to keep it as is. The main reason is that I'm going to create a new application for signal (etc) processing utilizing this improved version of Ryven, based on the concept of a start-stop button that we talked about in our e-mail correspondence. I would like to finish any changes that are Ryven compatible as fast as possible before creating a non-compatible repository.
I have a few thoughts on the rest.
We could arrange a call. I'm assuming you want to talk more about the RandNode traits example and less about the basic inspector implementation which is completely agnostic. Unless I got that completely wrong and you want to talk about everything else except the inspector.
The base inspector stuff is only a single file addition which I think is already quite intuitive and ready to use.
I find some things confusing and the API is not yet well integrated with the rest, so I would request changes which might delay things. But if you're waiting for all features here to be merged anyway we can keep it here. Conceptually, I think your approach is fine.
I can remove the example which uses traits if you want. Ryven doesn't depend on that.
that would be good
We could arrange a call. I'm assuming you want to talk more about the RandNode traits example and less about the basic inspector implementation which is completely agnostic. Unless I got that completely wrong and you want to talk about everything else except the inspector.
It's kind of the opposite ;) some notes on the rest and discussing the inspector stuff. But can share some thoughts on node properties as well.
These last commits have:
The nodes widget below the package widget should also be refactored to use a QListView, which allows for far more efficient searching internally through regex (not for this PR)
As discussed, PrettyName
was replaced with the __str__
method. QGraphicsItemWrapper
was renamed to QGraphicsItemAnimated
.
As for the rest:
pkg.sub1.sub2.sub3....subN.<ClassName>
it should work out of the box.NodeGUI
. Feel free to do the change here. After you do, notify me and we can close the PR.QStandardItem
and QStandardItemModel
as well as a QTreeView
. Searching is done via regex. Searching is far more efficient because it's on the C++ side, but I at least have no idea how to implement fuzzy searching via regex. (also check QTreeWidget
and QTreeItemWidget
, perhaps things are easier there)If I've forgotten anything, please add for completion.
Somehow I'm unable to push to this PR. I pushed to branch pr176
instead, you can just merge that into your master
.
some notes:
sorry, I forgot to include serialization of inspector widgets, commited it now
what is Delegate_Command.__events? I commented it out
Oops, I forgot to remove that. Good catch.
do you know how to change the tab view with inspector,
No idea, I will do a short search and report back.
I also implemented the unconstrained package nesting I proposed
The pkg_test
which had a combination of std
and linalg
is broken. Should I fix it or remove it?
I see. So the user has to define the sub-package name and can also achieve N levels of nesting, e.g sub_1.sub_2...subN
, correct? I can see someone making a mistake when defining a nested path, e.g sub_1.sub_2
and sub_1.sub_21.sub_3
. I'm alright with that, and people should use python to avoid this. Also, shouldn't we set the data's identifiers to use the package path as well?
I reworked the inspector mechanism
Nice, I can see what you wanted to do. Now that I'm thinking about it, serialization might be overkill, since I only see an inspector's view being dependent on the configuration of the nodes and not some kind of internal state. But an API should provide everything I suppose.
All I need is your opinion on my small concerns regarding the packages and we could close this.
do you know how to change the tab view with inspector...
I found a solution that you can see in 1e468ea (b452f05 is for a mistake I made). I defaulted it to 65px. I don't know if all tab bars of the flow ui are affected by this, but at least the starting ones are.
Also, two other things:
FlowView
from inside the NodeInspectorWidget
. Without this, we don't have a way to push delegate undo commands.Node
or base for NodeInspectorWidget
, node creation fails silently. No node visual is shown in the editor and no message, even in verbose mode.It's easy to reproduce..
class MyNode(Node):
title = 'My Node'
def __init__(self, params):
pass
If you could fix these two things, it'd be awesome!
Sidenote:
If you want to implement fuzzy searching for the package tree, you can do this by inheriting from QSortFilterModel
, overriding the filterAcceptsRow
method and using that class instead of QSortFilterModel
in NodeListWidget
. Searching would probably become really slow when a lot of big packages are imported though, since we're trading the fast C++ internal implementation with a python method. QSortFilterModel
provides regex and wildcards out of the box. You can test the custom fuzzy searching, but for this to really work, ideally you'd want a C++ plugin.
The
pkg_test
which had a combination ofstd
andlinalg
is broken. Should I fix it or remove it?
I think you can remove it.
shouldn't we set the data's identifiers to use the package path as well?
I don't think so, the identifier should only be the class identifier extended by the (sub-)ryven-package name.
Now that I'm thinking about it, serialization might be overkill, since I only see an inspector's view being dependent on the configuration of the nodes and not some kind of internal state.
I agree, the general suggestion is to avoid state whenever possible, and otherwise keep it tiny
I'll take another look at the code today, maybe clean some things and let you know when you can add remaining changes from pr176
so I can merge this.
I defaulted it to 65px. I don't know if all tab bars of the flow ui are affected by this, but at least the starting ones are.
I don't think that's the way to go
If there's no better solution, at the very least we should set the width of only this widget in the stylesheet file to something pixel density agnostic.
- We need a way to access the
FlowView
from inside theNodeInspectorWidget
. Without this, we don't have a way to push delegate undo commands.
The package should not access the FlowView directly, but we can make a method, I'll take a look.
- If an error is caused on node creation, e.g we forgot to call base for
Node
or base forNodeInspectorWidget
, node creation fails silently. No node visual is shown in the editor and no message, even in verbose mode.
thanks, will take a look
If you want to implement fuzzy searching for the package tree, you can do this by inheriting from
QSortFilterModel
, overriding thefilterAcceptsRow
method and using that class instead ofQSortFilterModel
inNodeListWidget
. Searching would probably become really slow when a lot of big packages are imported though, since we're trading the fast C++ internal implementation with a python method.QSortFilterModel
provides regex and wildcards out of the box. You can test the custom fuzzy searching, but for this to really work, ideally you'd want a C++ plugin.
I think the fuzzy search for nodes is basically instant, even with >100 nodes, so it should be doable. If it really becomes too slow it might be worth considering actual extension of QSortFilterModel
.
We need a way to access the FlowView from inside the NodeInspectorWidget. Without this, we don't have a way to push delegate undo commands.
see 22b12a0dc2acc4a0ae525710b2baa6240b9412cc
If an error is caused on node creation, e.g we forgot to call base for Node or base for NodeInspectorWidget, node creation fails silently. No node visual is shown in the editor and no message, even in verbose mode.
I can't quite reproduce this, I get an internal error (AttributeError: 'MyNode' object has no attribute 'input_added'
) on pr176
branch.
edit:
If there's no better solution, at the very least we should set the width of only this widget in the stylesheet file to something pixel density agnostic.
I noticed most of the styling rules are still pixel based. I moved the min width to the stylesheet file, but it's still bothering me that it's not specific on the tab widget, and I'm not sure how to make it since the tab widget is not explicitly created by us, but by the docking system
I just fixed something in ryvencore, but I'm still running into some bugs with the undo-redo.
I don't think so, the identifier should only be the class identifier extended by the (sub-)ryven-package name.
What I meant is that currently the identifier for any Data type is only the class name, since we only pass the extended package name identifier only to the node classes. Shouldn't we pass it to the Data classes as well? Otherwise, two data classes with the same name from different packages will collide with each other.
If it really becomes too slow it might be worth considering actual extension of QSortFilterModel.
I was talking about the fuzzy search for the package search bar. Currently it's only regex, to implement a fuzzy search one would need to override QSortFilterModel
. I pointed it out so you'd remember it later.
I noticed most of the styling rules are still pixel based. I'm not sure how to make it since the tab widget is not explicitly created by us, but by the docking system
We can change it to em
if you want to and decide on a basic value that would only hide really large words, like "Undo History".
No clue on how to access that specific tab bar only as well. But changing to em
and finding a reasonable value should solve most of the problem.
I can't quite reproduce this, I get an internal error (AttributeError: 'MyNode' object has no attribute 'input_added') on pr176 branch.
If I change the inspector example MyNode
to:
class MyNode(Node):
title = 'My Node'
def __init__(self, params):
pass
I get nothing when I drop the node in the view. Neither a visual nor an error. An exception occurs, but nothing is printed anywhere. You tried this and you get an error?
but I'm still running into some bugs with the undo-redo.
Can you please elaborate? Do the bugs concern the selection undo-redo, the delegate or something else?
EDIT: I noticed Clearing the selection sometimes makes an inspector widget pop up randomly. I didn't have this behavior before, I guess it must be one of the bugs you're talking about.
Also, I noticed you hide the console in verbose mode. It could still be used for entering commands, but if you prefer it that way, I'm okay with it.
Can you please elaborate? Do the bugs concern the selection undo-redo, the delegate or something else?
Some of your selection implementation changes were a bit confusing (or otherwise under-documented) I think. I just pushed some simplifying changes which fix the selection bugs I encountered, but I didn't thoroughly test yet.
EDIT: I noticed Clearing the selection sometimes makes an inspector widget pop up randomly. I didn't have this behavior before, I guess it must be one of the bugs you're talking about.
Yes, this is really strange, I'm unable to narrow this down to a specific source so far. A minimal reproduction I found:
Delete node
to <empty>
) then the node's inspector pops up as a windowAlso, I noticed you hide the console in verbose mode. It could still be used for entering commands, but if you prefer it that way, I'm okay with it.
Ideally, the terminal should also serve as input in verbose mode, it doesn't make sense to input at one place but see the output somewhere else. But I didn't do that yet.
I just pushed some simplifying changes which fix the selection bugs I encountered, but I didn't thoroughly test yet.
if hover_item is not None
should be if not hover_item
. The original is correct. If there isn't an item underneath, then set it to _SelectionMode.UNDO_DRAG
which is for multi-selection. The way it is now it's a bug. Should I pull the changes, fix it and rename the enums to better names?
EDIT: Are you sure you encountered selection bugs? I'm testing before the changes and it seems to be working ok.
Yes, this is really strange, I'm unable to narrow this down to a specific source so far. A minimal reproduction I found:
It's due to InspectorView.clear()
. It's an old method that probably shouldn't even be there anymore. Calling setParent(None)
on a widget apparently makes it a standalone window (sometimes?). Tell me when I can pull to check up - fix things.
InspectorView.clear()
should be removed and set_node
should be:
def set_node(self, node: Node):
"""Sets a node for inspection, if it exists. Otherwise clears the inspector"""
if self.node == node:
return
if self.inspector_widget:
self.inspector_widget.unload()
self.inspector_widget.setVisible(False)
self.inspector_widget.setParent(None)
self.node = None
self.inspector_widget = None
if node is not None:
self.node = node
self.inspector_widget = self.node.gui.inspector_widget
self.layout().addWidget(self.inspector_widget)
self.inspector_widget.setVisible(True)
self.inspector_widget.load()
The original is correct.
oops good catch, should be if hover_item is None
then, fixed it
and rename the enums to better names?
doing that right now :)
EDIT: Are you sure you encountered selection bugs? I'm testing before the changes and it seems to be working ok.
I encountered recursive undo errors but didn't look into where they originate from tbh. Can't reproduce them with my changes to FlowView
I pushed the fix, seems to work now, I don't see any bugs at the moment. Could you elaborate how the connection animations worked? And what's the difference between ConnectionAnimation
and ConnectionItemsAnimation
. After connection animations I think we can proceed.
ConnectionItemsAnimation
is responsible for computing the segments based on the length of the path to place QGraphicsItemAnimated
objects on by calling recompute
. It can then animate these items over the path in an infinite loop. It can be reloaded at any time calling recompute
if the path changes. The duration
parameter is obsolete, the speed
parameter determines the constant speed with which the items move. (remove the self.duration stuff or leave it to me)
ConnectionAnimation
uses ConnectionItemsAnimation
and also animates the scale of each QGraphicsItemAnimated
individually. It serves as a way to toggle the "dots animation" on and off at any point.
Both of these are as optimal as I could make them. ConnectionAnimation
guarantees that the path animation won't be running if it is toggled off. Rename them any way you like.
I'm creating both of them in the constructor of ConnectionItem
, though ideally it should be passed from outside. I just left it there for completion. If you want to see it live and also recalculate, you can comment out the mouseReleaseEvent
in lines 184-185 and simply click on the connection to toggle it off and on and move the nodes around.
Also, should we set the min width to something like 8em
instead of 65px
for the tabs or leave it as is?
So what should the duration
argument for QTimeLine
in the constructor of ConnectionItemsAnimation
be?
If you want to see it live and also recalculate, you can comment out the
mouseReleaseEvent
in lines 184-185 and simply click on the connection to toggle it off and on and move the nodes around.
ah so that doesn't actually animate update at the moment?
Also, should we set the min width to something like
8em
instead of65px
for the tabs or leave it as is?
since everything else is pixel based right now, I'll leave it as 65px to prevent confusion, but the stylesheet might need an overhaul
The argument shouldn't exist. Nor the properties. Only the speed argument is needed. Duration is recalculated on recompute. You can just pass a random argument in QTimeline's
constructor. I intentionally left the animation out of update to avoid animation overload. But the API it comes with should be pretty simple if anyone wants to use it.
Alright, I did some refactoring and added a few clarifications. Does it look good to you?
I get nothing when I drop the node in the view. Neither a visual nor an error. An exception occurs, but nothing is printed anywhere. You tried this and you get an error?
Indeed, I get the error in verbose mode.
Hmmm1. I noticed one thing. When creating a node, previously that node was selected but not registered as an undo. It still isn't registered as an undo, but instead of selecting only that node, it adds it to the current selection. I'll try to fix it...
Also, when selecting something and undo-ing it, at least the visual selection isn't undone. Let me check a bit because this wasn't here before.
Indeed, I get the error in verbose mode.
Interesting...
For the connection animations:
I didn't implement this on update largely because I didn't know how much time it should be switched on for. Also, if the time hasn't passed, it should be reset to stay on. It shouldn't be that hard to do though. I'll probably do it at some point for my own project and will try to make a contribution.
select_items
clears the current selection before applying the selectionPlease check that any undo done results in the previous selected item (if you undo a delete, the deleted items are selected... or if you undo a place, the previous items are selected etc or if you place, that item is selected...)
If that is fixed, we should be good to go I think!
Btw. do you know why we cannot hide widgets in sliders anymore since the docking additions? I can put this on the TODO but it's bothering me quite a bit so I'd rather fix this sooner than later. I want to be able to drag a splitter handle all the way to one end so at the end it fully hides the content in that direction.
I think it has to do with the fact that it's not a splitter anymore. Docks can be closed by right clicking on the title of a dock or through their X button or in the background of the main window in which the docks reside. For main window docks, you can also close them from View/Windows and for flow docks from the Menu button in the Graphs View. I don't think you can hide the whole dock area by dragging the splitter.
The only docks that I made un-closeable are the flows and the nodes.
Isn't the docking stuff just another widget? Can't we put a dock widget in a splitter?
The docking stuff attach to a main window's predefined dock areas (left, top, right, bottom). They have various settings to make them float or be tabbed and etc... But I'm pretty sure you can't put a dock inside a splitter. I tried searching for this when I was making it but didn't find anything.
Is it that important? We could add menu functions that close them all or open them all.
Hm yeah I can't come up with a better solution right now...
Done, check it out.
The code is the same for both main_window.py
and flow_ui.py
. Leaving it up to you if you want to make it into a utility somewhere.
thanks! i'll take a look later, can merge afterwards
What I meant is that currently the identifier for any Data type is only the class name, since we only pass the extended package name identifier only to the node classes. Shouldn't we pass it to the Data classes as well? Otherwise, two data classes with the same name from different packages will collide with each other.
Oh, also, I got no answer on this, unless I missed it.
Oh, also, I got no answer on this, unless I missed it.
Sorry, forgot about it and yes I misread your comment, of course we should do the same to Data
. I just pushed it.
Alright! We're good to go I think
Alright! We're good to go I think
just check my review comment above
The last review I see is about ItemIsSelectable
I commented on b542a4016cbffab6a8596d6d3ca32e28e04582ac flow_ui.py
Sorry, I'm searching but it shows 0 comments on my end.
:thinking: strange. let's just do this:
Weird that I can't see it. Anyway, I put that there so we could close only the docks of the flow through its menu in the graph view if needed and not all the docks everywhere.
Also, now that I'm thinking about it, should we only close any tabified docks and not any detached, floating ones?
Weird that I can't see it. Anyway, I put that there so we could close only the docks of the flow through its menu if needed and not all the docks everywhere.
ohh now I see where it is, yes that makes sense
Also, now that I'm thinking about it, should we only close any tabified docks and not any detached, floating ones?
good point, do you think this is easy to do?
do you think this is easy to do?
done
looking good :+1:
Found another potentially good thing. We can make it so that the menu isn't closed when clicking window actions, i.e not closing when pressing Menu/Windows/Inspector in the graph view menu. That way, the user can toggle many things at once.
I agree. I found this
Okay, I have this working:
# add all docks to menu
for w in all_dock_widgets:
def toggle(checked, w=w):
# second parameter is to avoid the closure problem
# https://stackoverflow.com/questions/3431676/creating-functions-in-a-loop
w.setVisible(checked)
label = w.windowTitle()
cb = QCheckBox(label, self)
cb.setChecked(True)
cb.toggled.connect(toggle)
action = QWidgetAction(self)
action.setDefaultWidget(cb)
windows_menu.addAction(action)
I'll push to pr176
edit: unfortunately, w.isVisible()
seems to be False
at this point, which is why I hard-coded setChecked(True)
. Let me know if you have a better idea
It also only works when pressing the check box or the name. If you press slightly to the right it closes the menu. And currently it isn't highlighted when hovered.
Due to the fact that you may open a saved project, the tab would be closed but the action would be highlighted. I think this is too hacky. Perhaps add it to the TODO list? I'll search for a bit but I think it's too minor a thing to keep this PR open, since there is no apparent easy and correct way to do this.
This PR is a result of #173. Apart from the inspector, it includes some fixes, optimization and additions. Most of the updates are showcased in the video below.
https://github.com/leon-thomm/Ryven/assets/135903670/37def18b-8545-48ad-b74c-634506871be8
Inspector
For the inspector, there's a new
InspectorGUI.py
file. It contains:InspectorWidget
: A simple wrapper for holding the actual [BaseNodeInspector
] inspector.BaseNodeInspector
: The base inspector;create_inspector
must be implemented, where the user can insert their GUI using theparent
parameter. Anunload
method is provided for cleanup.NodeInspector
: A basic implementation ofBaseNodeInspector
, where aNodeInspectorWidget
is used. This inspector shows the title of the node, the python and ryven id and meta-data information. Theattach_inspector
method can be implemented to attach a configuration gui between the title and the metadata. TheNodeInspector
is the default for any node that hasn't explicitly defined an inspector.For connecting an inspector to a node, a new decorator
inspector_gui
is defined which works like thenode_gui
decorator.A new example package named
traits_inspector_test
is given. It uses traits and traits ui to rapidly implement a configuration window for theRand
node. Configuration and value generation are undoable and there's a generate button for getting a new random value. State saving-loading is a simple hard-coded implementation, nothing generic here. The random state is not saved.This inspector could serve as an example on how one can use traits to create a base inspector for their packages.
Undo
New additions:
PrettyName
interface is defined inGUIBase.py
. It can be used on any object to define how it should be displayed in text format. It has been used onNodeItem
,ConnectionItem
andDrawingObject
.NestedCommand
. Not currently used, but provides a way to bundle commands together.DelegateCommand
. A command whose undo and redo are passed functions. The exampleRandNode
uses it to implement undo functionality.Selection
The
selection system
has been refactored to work with theundo system
. It utilizes a new_SelectionMode
enum that hasINSTANT
,UNDO_CLICK
andUNDO_DRAG
values.INSTANT
: Selection isn't added in undo stack. Whenever anything is created or removed, this mode is used.UNDO_CLICK
: A single selection that occurs when the user presses on an object and is added to the undo stack.UNDO_DRAG
: If the user presses on the canvas / view, then the selection event terminates onmouseRelease
. Anything that has been selected is a single multi-select event passed to the undo-stack.Node and Connection items
The
NodeItemAnimator
has been expanded to include a scaling animation. Whenever a node is updated, it plays that animation along with the others. You can see it in the showcase video.When a connection item is added or deleted, forced the corresponding node items to update their representation; This fixes a bug where ports weren't showing correct visuals.
ConnectionItem
has two additional useful classes:ConnectionItemsAnimation
that animates any abstractQGraphicsItem
over the path to create a visual flow. AQGraphicsItemWrapper
class is provide inGUIBase.py
for allowing the animation of any item, even if it doesn't implementQObject
.ConnectionAnimation
that utilizes theConnectionItemsAnimation
to toggle the data flow. You can see it at ~0:57 in the video. The toggling happens on click and it's for demonstration purposes. In this PR nothing happens when clicking. However, this item might be useful for anyone that wants to expand Ryven or use it as a template for something else.I've optimized the way these animations are created, played, paused and resumed. Here's a snapshot; a gif showing there's no visible performance problem.
Flow View
node_color
being a string instead of a color. If I haven't missed anything, every theme is bug free.resetCachedContent()
function.drawBackground
of the flow view. It uses only the visible rect and not the whole scene. It's much faster now.Pretty
andFast
modes aren't really that much different performance-wise now.Unfortunately, I could not fix the horrendous performance when there are a lot of items and we're zooming in / out (essentially scaling). I read that it may have something to do with setting the correct cache mode (1, 2), but I couldn't find anything that works.
This is all! Should provide a substantial update for Ryven.
(I didn't include panning and zooming in the undo. Perhaps it can be added at a later time.) (Ideally I'd also like to implement port validation with specific data types and a multi port where you can add N connections automatically, but these two may need ryvencore changes or be package specific, so I opted out of this feature for now)