Skip to content
This repository was archived by the owner on Sep 20, 2024. It is now read-only.

OpenRV integration#4839

Closed
ddesmond wants to merge 116 commits intoynput:developfrom
ddesmond:openrv-integration
Closed

OpenRV integration#4839
ddesmond wants to merge 116 commits intoynput:developfrom
ddesmond:openrv-integration

Conversation

@ddesmond
Copy link

@ddesmond ddesmond commented Apr 13, 2023

Changelog Description

Draft OpenRV integration including Ftrack support
Features:

  • View FTrack action, Review Ftrack action
  • OpenPype Menus package
  • OpenPype Comments package
  • Loading and Publising annotations

Additional info

You need to compile OpenRV first, head to OpenRV github and do it!

TODO

  • to have a review session you need a task in ftrack where RV can hook to -> in my case ive setup/hardcoded
task_name = "prepDaily"
asset_name = "DaliesPrep"

so RV can have its session files stored with a proper task. this should be somehow parametrized ?

Roy: Untested - still todo.

TODO

  • it needs a proper rv home env var setup otherwise it wont work. Roy: Bit unclear where this needs to be used and why it's required, but something to pay attention to.
  • Have OP menus working stabily by serving a customSUPPORT path and config path or to install the packages automatically. Roy: This is currently hardcoded into the OP codebase (in launch prehook) and should be resolved. Relevant todo is in the code. (there should be better ways to automate this)
  • OpenRV needs a clean pythonpath and we have to rebuild everything afterwards. Roy: This has currently been worked around with a hack using sys.path in OpenPype menus package.
  • Fix remainder of Hound comments
  • Test whether everything works with OpenPype on PYTHONPATH - according to @ddesmond it breaks with OP on path but I didn't have that issue. He mentioned it's something with OTIO / OCIO or alike, so I'll have to investigate those features.
  • Fix loading frame ranges for sequences
  • Fix removing loaded containers
  • Add support for applying correct OCIO color space per loaded footage using the representation's colorspace.
  • Fix schema refactor for imageio settings
  • Test the annotations/commenting workflow
  • Styling of the OpenPype Qt widgets is slighty off (non-critical), see image below
    • text in header is small
    • spacing in column/row highlighting
    • scrollbars look different?

afbeelding

Testing notes

  1. Run OpenRV through OpenPype and go crazy
  2. Test the ftrack actions to see whether they do what they should

@ynbot ynbot added module: Ftrack size/XL Denotes a PR changes 1500-2499 lines, ignoring general files labels Apr 13, 2023
@tokejepsen tokejepsen self-assigned this Apr 13, 2023
@ddesmond ddesmond marked this pull request as draft April 13, 2023 08:12
Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff. There are quite a few cosmetic things the hound trips over. Would you like me to clean that up for you? I can push directly into your branch (if you'd like that) or create a PR to your repo if you prefer that?

@ddesmond
Copy link
Author

Great stuff. There are quite a few cosmetic things the hound trips over. Would you like me to clean that up for you? I can push directly into your branch (if you'd like that) or create a PR to your repo if you prefer that?

push directly to my branch

@BigRoy BigRoy requested a review from tokejepsen April 13, 2023 09:34
@BigRoy BigRoy marked this pull request as ready for review April 18, 2023 15:02
@BigRoy
Copy link
Collaborator

BigRoy commented Apr 18, 2023

This PR isn't finished but I'm marking it for review anyway - there are definitely things that need cleaning up before it can get merged but there's also already a lot which should be up for discussion and testing.

What is there now?

@ddesmond code has been refactored to:

  • remove unused code
  • add docstring + comment code here and there
  • refactored to use new publisher
  • some additional features fixed like OCIO support + open last workfile, etc.
  • there is a bit of 'auto-install' stuff and a script editor exposed in OpenRV now to ease debugging/usage
  • OpenPype menu with the tools showing structured like it is in other hosts
  • loading containers works for videos + sequences and supports the colorspace data

What to do next?

Required before merge

  • see bullet point todo list in original post
  • refactor auto-install on startup that doesn't "nest itself" inside of OpenPype deployment
  • finish support annotation publishing (or remove it and keep for later PR)
    • I will create a forum discussion related to this to discuss how we'd want to publish "annotations" related to a publish in OpenPype.
  • finish support for ftrack actions to open a review (or remove it and keep for later PR)
  • finish support for the "review" package that is in this PR for OpenRV. However it's currently a bit wonky in functionality and far from polished finished. I'd say it's likely best to push that out of the base OpenRV PR and separate that into a PR of its own.

Nice to have*

  • fix Qt styling

Who do we have available to test this PR?

@BigRoy
Copy link
Collaborator

BigRoy commented Apr 18, 2023

Here's a screen capture of the current state of this PR with some notes as a ramble on about it and misclick half of the UI buttons the whole time.

openpype_openrv_state_2023-18-04_PR.mp4

Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments to highlight todos.

Comment on lines +3 to +5
This tries to set the relevant OCIO settings on the group's look and render
pipeline similar to what the OpenColorIO Basic Color Management package does in
OpenRV through its `ocio_source_setup` python file.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we could also autoload this package - but I know too little of RV to say what the best approach is to do so.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've asked more information about this on the Shotgrid Software community about Autoload OpenColorIO Basic Color Management RV package.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions are basically untested at this stage as far as I know.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much haven't worked on this extractor at all - it's clearly WIP still.

@mkolar mkolar added the sponsored Client endorsed or requested label Apr 19, 2023
@rontown
Copy link

rontown commented Nov 4, 2023

Interesting to see you successfully compiled a version of OpenRV.
Ive been trying on and off to compile it on windows 10 and rocky 9.2 for months to no avail. Even more exciting to see the developing integration with openpype/ayon!

@antirotor
Copy link
Member

Interesting to see you successfully compiled a version of OpenRV.
Ive been trying on and off to compile it on windows 10 and rocky 9.2 for months to no avail. Even more exciting to see the developing integration with openpype/ayon!

Building on Windows does work, just follow closely this guide - AcademySoftwareFoundation/OpenRV#91

Comment on lines +16 to +21

executable = self.application.find_executable()
if not executable:
self.log.error("Unable to find executable for RV.")
return

Copy link
Member

@iLLiCiTiT iLLiCiTiT Dec 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would not get to prelaunch hook if executable would not be found.

Suggested change
executable = self.application.find_executable()
if not executable:
self.log.error("Unable to find executable for RV.")
return
executable = self.executable.executable_path

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The executable path is still needed for the logic below - so I guess it's just the if statement that you want removed, not the find_executable? Or is there a better way in the prelaunch hook to get the executable path?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified suggestion

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth and for your information:

This PR was originally opened by @ddesmond

I contributed into the PR at the time because I could because I had write permissions to the repo - I currently don't anymore so I am personally unable to contribute to this PR.

I believe @ddesmond is not working on this feature currently meaning that without other contributions from someone with write access this will end up staying as a stale PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need permissions i can give some to you @BigRoy

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is also ddesmond#8
You have been added as a colab to my repo @BigRoy , let me know guys if you need more help

@kalisp
Copy link
Member

kalisp commented Feb 1, 2024

Closing this one in favor of #6185 - which cleans up a bit this one, translates it into Ayon addon.

Any comments welcomed in that PR or in Issues in https://github.com/ynput/ayon-openrv

@kalisp kalisp closed this Feb 1, 2024
@ynbot ynbot added this to the next-patch milestone Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community contribution module: Ftrack size/XL Denotes a PR changes 1500-2499 lines, ignoring general files sponsored Client endorsed or requested target: OpenPype

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

10 participants