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

Fixes #34 Dev mode fix to load plugins from dev folder instead of OPENRV_ROOT_DIR & frame range from version #35

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion client/ayon_openrv/addon.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from .version import __version__

OPENRV_ROOT_DIR = os.path.dirname(os.path.abspath(__file__))

DEV_MODE = bool(os.getenv("AYON_USE_DEV"))

georgeantonopoulos-bcnvisuals marked this conversation as resolved.
Show resolved Hide resolved
class OpenRVAddon(AYONAddon, IHostAddon, IPluginPaths):
name = "openrv"
Expand Down Expand Up @@ -48,6 +48,11 @@ def add_implementation_envs(self, env, app):
def get_launch_hook_paths(self, app):
if app.host_name != self.host_name:
return []
if DEV_MODE:
georgeantonopoulos-bcnvisuals marked this conversation as resolved.
Show resolved Hide resolved
# In development mode, use the current directory's hooks
return [
os.path.join(os.getcwd(), "client/ayon_openrv/hooks")
]
return [
os.path.join(OPENRV_ROOT_DIR, "hooks")
]
Expand Down
43 changes: 30 additions & 13 deletions client/ayon_openrv/plugins/load/openrv/load_frames.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from ayon_core.pipeline import load
from ayon_core.pipeline.load import get_representation_path_from_context
from ayon_core.lib.transcoding import IMAGE_EXTENSIONS
from ayon_core.lib import Logger

from ayon_openrv.api.pipeline import imprint_container
from ayon_openrv.api.ocio import (
Expand All @@ -15,6 +16,9 @@
import rv


log = Logger.get_logger(__name__)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to define a logger here. Loader plugins come with their own logger. You can do self.log.debug for example on methods on the class instances (also works in classmethods if you need it)

Choose a reason for hiding this comment

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

Good to know thanks!

Choose a reason for hiding this comment

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

Noticed that the self.log.debug does not output to the terminal when --use-dev --debug args are used, might be doing something wrong :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose logging levels for Loaders log handlers do not default to debug level (not even in dev mode) @iLLiCiTiT ?
We might want to do that in ayon-core to ease debugging? Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

If that is the case then that deserves an issue in ayon-core indeed.



class FramesLoader(load.LoaderPlugin):
"""Load frames into OpenRV"""

Expand All @@ -28,7 +32,6 @@ class FramesLoader(load.LoaderPlugin):
color = "orange"

def load(self, context, name=None, namespace=None, data=None):

filepath = self._format_path(context)
# Command fails on unicode so we must force it to be strings
filepath = str(filepath)
Expand All @@ -37,6 +40,7 @@ def load(self, context, name=None, namespace=None, data=None):
namespace = namespace if namespace else context["folder"]["name"]

loaded_node = rv.commands.addSourceVerbose([filepath])
log.debug(f"Loaded node: {loaded_node}")

# update colorspace
self.set_representation_colorspace(loaded_node,
Expand Down Expand Up @@ -97,22 +101,32 @@ def _get_sequence_range(self, context):

"""
repre_entity = context["representation"]
version_entity = context["version"]

# Only images may be sequences, not videos
ext = repre_entity["context"].get("ext") or repre_entity["name"]
if f".{ext}" not in IMAGE_EXTENSIONS:
log.debug(f"Extension {ext} not in IMAGE_EXTENSIONS")
return

repre_attribs = repre_entity["attrib"]
# Frame range can be set on version or representation.
# When set on representation it overrides version data.

repre_frame_start = repre_attribs.get("frameStart")
repre_frame_end = repre_attribs.get("frameEnd")
if repre_frame_start is not None and repre_frame_end is not None:
if repre_frame_start != repre_frame_end:
return repre_frame_start, repre_frame_end
# Single frame
# Check version attributes first as they should have the correct frame range
frame_start = None
frame_end = None
georgeantonopoulos-bcnvisuals marked this conversation as resolved.
Show resolved Hide resolved

version_attribs = version_entity["attrib"]
frame_start = version_attribs.get("frameStart")
frame_end = version_attribs.get("frameEnd")

# If not in version attributes, check representation attributes
if frame_start is None or frame_end is None:
repre_attribs = repre_entity["attrib"]
frame_start = repre_attribs.get("frameStart")
frame_end = repre_attribs.get("frameEnd")
Comment on lines +110 to +118
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should reverse this - we should prefer repre values over version values.

Choose a reason for hiding this comment

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

@BigRoy in our case (publishes from Houdini and Nuke) the frameStart and frameEnd were pointing to the same frame (1001) whereas the version had the correct frameEnd, maybe the issue is elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sounds like some bug is going on for those publishes - that should not be the case or you're loading the wrong representation. Please provide a reproducable and we should investigate. But if a representation defines a frame range it should be a stronger opinion than the parent entity stating another frame range.

Copy link
Author

Choose a reason for hiding this comment

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

This log might help to debug, i'll look further into it to see where the representation attributes are being set,
How would you like a reproducible provided?

To me it looks like each frame has a different representation id, and all frames have frameStart and frameEnd of 1001. That goes for the representation_entity as well. Not sure if this is the default behaviour

Version attributes:
Version ID: c9cd8c78b71511ef88940615323c842b
Version attributes: {'comment': '', 'clipIn': 1, 'frameEnd': 1455, 'handleEnd': 0, 'pixelAspect': 1.0, 'source': '{root[work]}/civilizationvii/work/Film/NYD400/Lighting/civilizationvii_NYD400_Lighting_v056_alex.nindl.hip', 'frameStart': 1215, 'description': None, 'families': ['default', 'render', 'review'], 'handleStart': 0, 'fps': 60.0, 'site': None, 'ftrackId': None, 'intent': None, 'resolutionWidth': 1920, 'clipOut': 1, 'colorSpace': None, 'resolutionHeight': 1080, 'machine': 'ip-10-0-12-8.internal.bcnvisuals.com'}
Representation attributes:
Representation ID: ca003c38b71511efb1bc0615323c842b
Representation attributes: {'handleStart': 0, 'template': '{root[work]}/{project[code]}/publish/{hierarchy}/{folder[name]}/{product[type]}/{task[name]}/v{version:0>3}/{project[code]}{folder[name]}{product[name]}v{version:0>3}<{output}><.{frame:0>4}><_{udim}>.{ext}', 'fps': 25.0, 'clipIn': 1, 'path': '/mnt/production/project/civilizationvii/publish/Film/NYD400/render/Lighting/v004/civilizationvii_NYD400_renderLightingL_Helm_v004.1215.exr', 'frameEnd': 1001, 'handleEnd': 0, 'pixelAspect': 1.0, 'frameStart': 1001, 'resolutionWidth': 1920, 'clipOut': 1, 'resolutionHeight': 1080, 'extension': None, 'description': None}
Sequence frames: 1215-1455 (241 frames)

Copy link
Contributor

@BigRoy BigRoy Jan 9, 2025

Choose a reason for hiding this comment

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

If each frame has a different representation then there is definitely! something wrong. Please provide a reproducable.


if frame_start is not None and frame_end is not None:
if frame_start != frame_end:
log.debug(f"Using frame range: {frame_start}-{frame_end}")
return frame_start, frame_end
return

# Fallback for image sequence that does not have frame start and frame
Expand All @@ -137,7 +151,6 @@ def _format_path(self, context):
/path/to/sequence.1001-1010#.exr

"""

sequence_range = self._get_sequence_range(context)
if not sequence_range:
return get_representation_path_from_context(context)
Expand All @@ -146,6 +159,7 @@ def _format_path(self, context):
representation = context["representation"]
if not representation["attrib"].get("template"):
# No template to find token locations for
log.debug(f"No template found, using direct path")
return get_representation_path_from_context(context)

def _placeholder(key):
Expand All @@ -161,6 +175,7 @@ def _placeholder(key):
tokens = {
"frame": f"{start}-{end}#",
}
log.debug(f"Using tokens: {tokens}")
has_tokens = False
repre_context = representation["context"]
for key, _token in tokens.items():
Expand All @@ -170,12 +185,14 @@ def _placeholder(key):

# Replace with our custom template that has the tokens set
path = get_representation_path_from_context(context)
log.debug(f"Initial path with placeholders: {path}")

if has_tokens:
for key, token in tokens.items():
if key in repre_context:
path = path.replace(_placeholder(key), token)


log.debug(f"Final formatted path: {path}")
return path

def set_representation_colorspace(self, node, representation):
Expand Down