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

Unreal: Build shot structure from the database #25

Conversation

simonebarbieri
Copy link
Contributor

Changelog Description

Added a button to build the shot structure as sequences from the database.

PR linked to ynput/OpenPype#6168

@@ -590,21 +590,21 @@ def set_sequence_hierarchy(
hid_section.set_level_names(maps)


def generate_sequence(h, h_dir):
def generate_sequence(name, hierarchy_dir):
tools = unreal.AssetToolsHelpers().get_asset_tools()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get a docstring for this function?

Comment on lines 63 to 72
sequence_root_name = "shots"

hierarchy = get_folders_hierarchy(project_name=project)["hierarchy"]

# Find the sequence root element in the hierarchy
sequence_root = next((
element
for element in hierarchy
if element["name"] == sequence_root_name
), None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the requirement of there being a shots folder in the hierarchy an odd requirement maybe? Seems a bit hardcoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, yes, this is just a placeholder for now to get the functionality working, but there was discussion about creating a UI ad hoc to select which folder you want to create the shot structure for, so it's WIP :)

@simonebarbieri simonebarbieri marked this pull request as draft February 12, 2024 14:17
@simonebarbieri
Copy link
Contributor Author

@BigRoy sorry, I forgot to set the PR as draft! It needs some more work! I will address all of of your points when I get back to work on this :) thanks as always for the inputs on these :D

@simonebarbieri simonebarbieri marked this pull request as ready for review March 1, 2024 15:46
@simonebarbieri simonebarbieri requested a review from iLLiCiTiT March 1, 2024 15:46
@simonebarbieri simonebarbieri requested review from BigRoy and antirotor and removed request for BigRoy March 1, 2024 15:46
Copy link
Member

@antirotor antirotor left a comment

Choose a reason for hiding this comment

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

I need to go through it properly, but just quick first notes:

set_sequence_hierarchy() needs imho even better docstring. Or better names for arguments :) ... or both?

Like this usage:

set_sequence_hierarchy(
    parents_sequence[i], parents_sequence[i + 1],
    parents_frame_range[i][1],
    parents_frame_range[i + 1][0], parents_frame_range[i + 1][1],
    [level])
)

I must admit I really don't know what is going on there.

Another point - since paths in UE are paths, we can use pathlib.Path on them so no need of .split("/") and stuff.

Also, some part are rather static, like get_default_sequence_path(settings). And I realize we tend to do it a lot in Unreal - expecting that something is always on specific path. I am glad that some of those paths can be now set in the settings - but we might later on (not in this PR) consider using actual anatomy templates there.

PlaceholderLineEdit,
SquareButton,
)
from ayon_core.tools.ayon_utils.widgets import (
Copy link
Member

Choose a reason for hiding this comment

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

NOTE: This has changed to from ayon_core.tools.utils import SimpleFoldersWidget from this PR #195

@antirotor
Copy link
Member

when #614 is merged, we need to reopen it in https://github.com/ynput/ayon-unreal

@antirotor
Copy link
Member

PR moved to ynput/ayon-unreal#4

@antirotor antirotor closed this Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants