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

feat: Add job node graph plugin #888

Conversation

IdrisMiles
Copy link
Contributor

Summarize your change.

  • Add plugin for visualising job layers in a node graph.
  • Vendorise NodeGraphQt module for node graph.
  • Apply layer filter on frame tree on single click of layer in
    layer tree or job graph (used to be double click).
  • Add global select_layer signal to CueGuiApplication

As discussed here this adds NodeGraphQt to the requirements_gui.txt as a git source, however from my fork, this is so that I could remove the python 3 pin in the setup.py and PySide2 version requirement in its own requirements.txt. Have tested this with both python 2 and 3 successfully.

job_node_graph

@larsbijl
Copy link
Contributor

Are there any considerations we need to have regarding the vendor's icons? @bcipriano

@IdrisMiles IdrisMiles force-pushed the feat-implement-job-node-graph-git-submodule branch from 4ffaa64 to 2e402ff Compare January 28, 2021 18:13
@larsbijl
Copy link
Contributor

Looks like NodeGraphQt requires PySide to already be installed before it can be installed itself.

@IdrisMiles
Copy link
Contributor Author

Yeah seems like it, I just tried a fresh virtual env and getting the same result as the CI.
I have to a pip install PySide2==5.11.2 and then pip install -r requirements_gui.txt works.

I think I can see the issue in NodeGraphQt's setup.py, it imports NodeGraphQt toget pkg info, but that import triggers the import of Qt.py which does a check for any available Python Qt bindings. I'll have a play around and see if I can tweak it to make it play ball

@IdrisMiles IdrisMiles changed the title feat: Add job node graph plugin WIP: feat: Add job node graph plugin Jan 30, 2021
@IdrisMiles IdrisMiles changed the title WIP: feat: Add job node graph plugin feat: Add job node graph plugin Jan 30, 2021
@IdrisMiles IdrisMiles force-pushed the feat-implement-job-node-graph-git-submodule branch 2 times, most recently from e40fc5e to d1fcc77 Compare February 17, 2021 21:40
@aoblet
Copy link
Contributor

aoblet commented Nov 24, 2021

Great feature !
Is merging still on the radar ? :)

@DiegoTavares
Copy link
Collaborator

Sorry, this feature fell between the cracks. We're going to test it locally an review the changes soon.

@roulaoregan-spi
Copy link
Contributor

As @DiegoTavares mentioned, after testing it locally, here are my comments:

Overall a really great plugin!! GJ! I think users (especially artists who use node graphs like Nuke/Houdini) will enjoy using this feature when monitoring their jobs.

When looking at the submodule in GitHub I did notice a disclaimer for using NodeGraphQt that (Note: this project is a work in progress and not production ready.) with stability labeled as work in progress. I am wondering if there are any risks for incorporating WIP projects, if so, should this be documented in Opencue as a disclaimer.

When possible rpc objects should call the api's function calls instead of natively calling .data for example rpcObject.name() instead of using rpcObject.data.name (ex JobMonitorGraph.py line 29, AbstractGraphWidget.py line 58 etc).

Is there a particular reason this cuegui/CueNodeGraphQt uses it's own constants.py folder? (I only see one variable in there) If not it should use the Constants.py to keep consistency and only have one point of access for constants.

I did notice that the label text in the node is still clickable and highlightable, although the user can't edit the text, it might be best to disable any clicking on the widget to avoid any confusion.

Just a few housekeeping comments:

  • Keeping to the existing naming style, the CueNodeGraphQT file names and variable names are using snake casing which deviates from the rest of the opencue/cuegui camel case convention since it is already the prevailing style (PEP 8). For example node_property.py, self.update_node_color(layerRpcObject) etc. mixing case types is discouraged.
  • I think the folder name "CueNodeGraphQT" should keep with the rest of the projects' subfolders naming conventions too which tend to use one word all lower case that encapsulates the category for extendability (ex. "images"), maybe try abstracting the name something like graph or to that effect?
  • As well the new files should also keep to the convention of including a one line summary description, example AbstractTreeWidget.py to the effect of """Base class for CueGUI .... and JobMonitorGraph.py with """Tree widget to display a node graph .... as well the files included in the CueNodeGraphQt folder. I'm not sure if the Boilerplate copyright to the top of the files need to be added during the PR or not?
  • Try to remove uncomment code or add a comment why it should be kept, for example lines 52-55 in JobMonitorGraph.py

Looking forward to this feature!
Cheers

@IdrisMiles
Copy link
Contributor Author

Thanks for all the feedback!
As I wrote this quite a while back I'm a bit hazy on some of the details and decisions made, I think I was attempting to follow any conventions I saw within NodeGraphQt, but in general everything you said makes sense.
Will aim to address your notes soon :)

I've also noticed that a newer version of NodeGraphQt has a change for implementing locking/unlocking node ports. I think I remember having to do some hacks to prevent users being able to disconnect nodes, so this update may help cleanup the code a bit more too!

- Add plugin for visualising job layers in a node graph.
- Vendorise NodeGraphQt module for node graph.
- Apply layer filter on frame tree on single click of layer in
  layer tree or job graph.
- Add global select_layer signal to CueGuiApplication
- Rename CueNodeGraphQt submodule to nodegraph
- Address style notes from PR
- Replace usage of `.data` attr on rpcObjects
- Bump NodeGraphQt version to v0.1.7 as provides
  port lock feature
- Implement  new `services` method on `Layer` wrapper
@IdrisMiles IdrisMiles force-pushed the feat-implement-job-node-graph-git-submodule branch from d1fcc77 to 7751457 Compare January 16, 2022 22:23
@IdrisMiles IdrisMiles marked this pull request as draft January 16, 2022 22:32
I believe there may be a bug in NodeGraphQt, as subclasses of
`NodeBaseWidget` shouldn't need to implement the `value` properties,
but this is currently required for the progress bar widget to update on
the node.
@IdrisMiles IdrisMiles marked this pull request as ready for review January 21, 2022 12:09
@IdrisMiles
Copy link
Contributor Author

Some things to note with this PR..

I've updated the version of NodeGraphQt used as it provides some features which simplify the code here (for example the new built in auto layout function, and port locking ) and have reverted the requirements_gui.txt to pull from the original project rather than my own fork.

However the main drawback to now pulling from the original project is the projects setup.py pins python_requires='>=3.6' which causes the CY2019 tests to fail:

ERROR: Package 'NodeGraphQt' requires a different Python: 2.7.15 not in '>=3.6'

Is supporting python 2.7 backwards compatibility still a requirement for new features?
If so, I can update my fork of the project to pull the latest updates and tweak the setup.py, based on the authors response to my issue a while back there's nothing critical that would break from 2.7.

Also the Lint CI job seems to be flagging false negatives, both timeout and itemSelectionChanged are Qt signals.

Running lint for cuegui/...
************* Module cuegui.AbstractGraphWidget
cuegui/AbstractGraphWidget.py:35:8: E1101: Method 'timeout' has no 'connect' member (no-member)
************* Module cuegui.LayerMonitorTree
cuegui/LayerMonitorTree.py:147:8: E1101: Method 'itemSelectionChanged' has no 'connect' member (no-member)

@DiegoTavares
Copy link
Collaborator

This PR has been stale for a while, but it is a neat feature. I'm giving it another chance to make its way to the master branch.

@szuhow
Copy link

szuhow commented May 6, 2024

This PR has been stale for a while, but it is a neat feature. I'm giving it another chance to make its way to the master branch.

@DiegoTavares I am waiting for news on that topic. I've implemented your feature on my own way, but it still would be nice to have it officialy.

@IdrisMiles
Copy link
Contributor Author

Hey there, I'll try refresh myself on this PR and update it.
It's been a while since I've worked on OpenCue (changed job and using another render scheduler now), but something I recall that may be an issue is that NodeGraphQt technically is supported for python >=3.6. Does CueGui need to be Python 2 compatible still?

@DiegoTavares
Copy link
Collaborator

On SPI we have migrated cuegui to python 3, we're discussing on the next TSC meeting if there's still need for python 2.7

@DiegoTavares
Copy link
Collaborator

Feature is being re-implemented at #1400

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.

6 participants