Skip to content
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

Inspector, Multi-Selection, Undo and more #176

Merged
merged 56 commits into from
Dec 5, 2023

Conversation

HeftyCoder
Copy link
Contributor

@HeftyCoder HeftyCoder commented Dec 2, 2023

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.

show_case.mp4

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 the parent parameter. An unload method is provided for cleanup.
  • NodeInspector: A basic implementation of BaseNodeInspector, where a NodeInspectorWidget is used. This inspector shows the title of the node, the python and ryven id and meta-data information. The attach_inspector method can be implemented to attach a configuration gui between the title and the metadata. The NodeInspector 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 the node_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 the Rand 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:

  • Every flow command must set its text. Since we need a generic way to acquire an object's name to display in undo commands, a PrettyName interface is defined in GUIBase.py. It can be used on any object to define how it should be displayed in text format. It has been used on NodeItem, ConnectionItem and DrawingObject.
  • NestedCommand. Not currently used, but provides a way to bundle commands together.
  • DelegateCommand. A command whose undo and redo are passed functions. The example RandNode uses it to implement undo functionality.
  • An undo history tab was added in the flow view.

Selection

The selection system has been refactored to work with the undo system. It utilizes a new _SelectionMode enum that has INSTANT, UNDO_CLICK and UNDO_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 on mouseRelease. 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 abstract QGraphicsItem over the path to create a visual flow. A QGraphicsItemWrapper class is provide in GUIBase.py for allowing the animation of any item, even if it doesn't implement QObject.
  • ConnectionAnimation that utilizes the ConnectionItemsAnimation 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.

python_BYedzFahHK

Flow View

  • Fixed any Flow Theme that had a bug caused by the node_color being a string instead of a color. If I haven't missed anything, every theme is bug free.
  • The flow view correctly updates the background when changing it. This utilizes the resetCachedContent() function.
  • Optimized the drawBackground of the flow view. It uses only the visible rect and not the whole scene. It's much faster now. Pretty and Fast modes aren't really that much different performance-wise now.
  • The view's transformation and scrolling is also saved along with the project 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)

- Implemented an inspector_gui decorator, like the node_gui one for registering an inspector.
- Defined an InspectorWidget for managing the inspection
- Defined an InspectorGUI (might be renamed) which any user can implement to have an inspector gui appear in the Inspector Section. This provides full control to the user.
- Added a selection mode that indicates how the selection works and how it handles the scenes' select event
- Implemented a selection undo mechanism using the selection mode. Whatever has been selected can be undone (should be useful for inspectors). When drag-selection, the selection is over when mouse is released.
- Added a on_move virtual function on GUIBase. This is called on move redoes, undoes. Currently it helps with recomputing the ConnectionItem when a move is undone.
- Made sure that pressing delete doesn't register unnecessary delete undoes.
- Removed self.remove_node_item and self.add_node_item forgotten functions.
- On drop event, made sure to setFocus again, otherwise keyPressed didn't work.
- Added a basic class for providing additional information
- Renamed InspectorGUI to BaseNodeInspector
Added a Delegate_Command for creating undo, redo functionality dynamically.
- Connections should be beneath nodes for pixel perfect pin handling
- Fixed the repaint background issue by calling self.resetCachedContent()
- Fixed the bugs in different flow themes
- Animating the scale when a node is updated
- Added a node item update when a connection is added or removed,(previously the state didn't refresh)
- Unloading the flow uis at exit to avoid an exception with selection undo / redo
- Added a PrettyName interface in GUIBase.py for easier name display
- NodeItem, ConnectionItem, DrawingObject implement the interface
- All flow commands are now given a name and displayed in the undo history

#TODO
Still need a way to clear memory if undo commands are deleted. But this is hard to do currently
- Massive improvement on performance for any flow theme. Used reset_cache content to ensure that the rect was updated. That way, we're drawing only on a small portion
- By resetting, any artifact problems with the proxy widgets in the scene seem to have been solved
+ Fix for Industrial Theme which had a bug
- Remove the starting of ConnectionAnimation on click
@leon-thomm
Copy link
Owner

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.

@HeftyCoder
Copy link
Contributor Author

HeftyCoder commented Dec 3, 2023

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.

Can't tell how a pyside2 reference creeped into here.
@leon-thomm
Copy link
Owner

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.

- Refactored the way packages are built. It's more efficient, more readable and allows for arbitrary nesting.
@HeftyCoder
Copy link
Contributor Author

These last commits have:

  • A more efficient way of creating the package visual tree. It now allows for any number of nesting. Packaging hasn't been changed, only the functionality for generating the visual tree.
  • Searching method for the packages, as well as including the nodes there for completion.

python_x3jsK9YsPN

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)

@leon-thomm
Copy link
Owner

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.

@HeftyCoder
Copy link
Contributor Author

HeftyCoder commented Dec 5, 2023

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.

@leon-thomm
Copy link
Owner

Isn't the docking stuff just another widget? Can't we put a dock widget in a splitter?

@HeftyCoder
Copy link
Contributor Author

HeftyCoder commented Dec 5, 2023

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.

@HeftyCoder
Copy link
Contributor Author

Is it that important? We could add menu functions that close them all or open them all.

@leon-thomm
Copy link
Owner

Hm yeah I can't come up with a better solution right now...

-From View/Windows where it acts on all docks, including every flow uis
-From each flow view's menu, where it closes only the flow vies
@HeftyCoder
Copy link
Contributor Author

HeftyCoder commented Dec 5, 2023

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.

@leon-thomm
Copy link
Owner

thanks! i'll take a look later, can merge afterwards

@HeftyCoder
Copy link
Contributor Author

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.

@leon-thomm
Copy link
Owner

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.

@HeftyCoder
Copy link
Contributor Author

Alright! We're good to go I think

@leon-thomm
Copy link
Owner

Alright! We're good to go I think

just check my review comment above

@HeftyCoder
Copy link
Contributor Author

The last review I see is about ItemIsSelectable

@leon-thomm
Copy link
Owner

I commented on b542a40 flow_ui.py

@HeftyCoder
Copy link
Contributor Author

Sorry, I'm searching but it shows 0 comments on my end.

@leon-thomm
Copy link
Owner

leon-thomm commented Dec 5, 2023

🤔
strange. let's just do this:

Screenshot_20231205_230047

@HeftyCoder
Copy link
Contributor Author

HeftyCoder commented Dec 5, 2023

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?

@leon-thomm
Copy link
Owner

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?

@HeftyCoder
Copy link
Contributor Author

do you think this is easy to do?

done

@leon-thomm
Copy link
Owner

looking good 👍

@HeftyCoder
Copy link
Contributor Author

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.

@leon-thomm
Copy link
Owner

I agree. I found this

@leon-thomm
Copy link
Owner

leon-thomm commented Dec 5, 2023

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

@HeftyCoder
Copy link
Contributor Author

HeftyCoder commented Dec 5, 2023

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.

@leon-thomm
Copy link
Owner

agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants