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

fix running builder on start if scene is the default empty scene #871

Merged
merged 23 commits into from
Oct 2, 2024

Conversation

MustafaJafar
Copy link
Contributor

@MustafaJafar MustafaJafar commented Sep 5, 2024

Changelog Description

This is a follow PR for ynput/ayon-houdini#74
to achieve test results mentioned in ynput/ayon-houdini#74 (comment)

Additional Info

I developed this and tested in Houdini addon..
and since this fix is in Core, then this also should work in other Addons since host.get_current_workfile always return None if scene is the default empty file.

Testing notes:

Basically, whenever an "empty scene" would initialize in Houdini - then use the template

  1. Open last workfile: True
    a. Launch with NO existing workfiles -> Workfile template build ✅
    b. Launch with existing workfiles -> NO workfile template build, because it opens the existing workfile. 🔴
  2. Open last workfile: False
    a. Launch with NO existing workfiles -> Workfile template build ✅
    b. Launch with existing workfiles -> Workfile template build ✅
    When doing "New scene" (e.g. CTRL+N), run the Workfile Template Build ✅

Create First Version: False
Never create and save workfile after template build if first version doesn't exist.

@fabiaserra
Copy link
Contributor

fabiaserra commented Sep 5, 2024

Also related to this I found a bug in our template use case if the placeholders don't explicitly define a representation name (which is what you'd want if you have varied representations named such as bgeo.sc, bgeo.gz...) on this function https://github.com/ynput/ayon-core/blob/develop/client/ayon_core/pipeline/workfile/workfile_template_builder.py#L1523

So I changed it to this:

        # Not return early if there's no representation name given as
        # that should be optional
        representation_names = None
        representation_name = placeholder.data["representation"]
        if representation_name:
            representation_names = [representation_name]

As you can see in our fork at https://github.com/fabiaserra/ayon-core/blob/release/alkemyx/client/ayon_core/pipeline/workfile/workfile_template_builder.py#L1523


NOTE: This is moved into separate PR #872

@BigRoy
Copy link
Collaborator

BigRoy commented Sep 5, 2024

Also related to this I found a bug in our template use case if the placeholders don't explicitly define a representation name (which is what you'd want if you have varied representations named such as bgeo.sc, bgeo.gz...) on this function https://github.com/ynput/ayon-core/blob/develop/client/ayon_core/pipeline/workfile/workfile_template_builder.py#L1523

@fabiaserra thanks - I've separated that into PR of its own since it's separate change/functionality. It's here: #872

Copy link
Contributor Author

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

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

Hey,
The PR still works as before.
I've added few comments to make it clearer.
Also, stopped using some unnecessary flag.

For reference, I tried the same cases mentioned in the PR description + the buttons in AYON menu
image

@BigRoy
Copy link
Collaborator

BigRoy commented Sep 6, 2024

@fabiaserra So - I've changed some things. Can you check whether you understand the arguments and the function now?

Copy link
Contributor Author

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

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

Test cases in PR description are fulfilled.
Also, explicit build and update workfile from template work.

@BigRoy
Copy link
Collaborator

BigRoy commented Sep 9, 2024

@fabiaserra and @MustafaJafar can you check whether you like the code style changes I just made?

@fabiaserra thanks for the notes - they were very helpful. I totally agree that the arguments are super weird on this function. I'd personally even argue that the new file toggle should be outside of the method that calls build_template. I'd expect a function to build_template to build regardless, and to have integrations either consider manually whether it's a new scene OR to have a dedicated build_template_for_new_file method that can have the initial create first version and save check. Anyway, that'd require refactoring into the other integrations and I've left that out for now.

@fabiaserra
Copy link
Contributor

@fabiaserra thanks for the notes - they were very helpful. I totally agree that the arguments are super weird on this function. I'd personally even argue that the new file toggle should be outside of the method that calls build_template. I'd expect a function to build_template to build regardless, and to have integrations either consider manually whether it's a new scene OR to have a dedicated build_template_for_new_file method that can have the initial create first version and save check. Anyway, that'd require refactoring into the other integrations and I've left that out for now.

100% agree, I was going to suggest this but I didn't want to be too critic and ask for too many changes

Copy link
Contributor

@fabiaserra fabiaserra left a comment

Choose a reason for hiding this comment

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

Looking much better already @BigRoy , thank you!

Copy link
Contributor

@fabiaserra fabiaserra left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor Author

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

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

ofc, I can't approve my own PR.

Anyways, it works on my side as expected.
Tested along with ynput/ayon-houdini#74

@iLLiCiTiT
Copy link
Member

Can someone shortly explain what should this PR solve (and put it into description)? Reading all the conversations did not give me much reliable information.

Does this try to solve case when you run template builder, but used "skip last workfile" in launcher?

@MustafaJafar
Copy link
Contributor Author

Does this try to solve case when you run template builder, but used "skip last workfile" in launcher

short answer is yes.


This PR adds support to run the workfile template builder on startup and allow to override template path resolution per DCC which is helpful for supporting native DCC keys/variables.

This PR was made as a follow up for ynput/ayon-houdini#74 the Houdini PR only allows calling the template builder on startup but we still needs to update the builder in the core to make it work with empty new scenes.

Maybe @BigRoy can explain it better than me.

@BigRoy
Copy link
Collaborator

BigRoy commented Sep 12, 2024

This PR adds support to run the workfile template builder on startup and allow to override template path resolution per DCC which is helpful for supporting native DCC keys/variables.

This PR was made as a follow up for ynput/ayon-houdini#74 the Houdini PR only allows calling the template builder on startup but we still needs to update the builder in the core to make it work with empty new scenes.

As far as I know the current PR is only cosmetic. Multiple people: me, Mustafa and Fabia were working on code that touched this but understood nothing of what build_template() call was trying to do.

Code logic seemed odd with method names doing more than they described, return types were off and just the whole logic of what workfile_creation_enabled is was completely misleading. This doesn't refactor the problem that the function itself is doing too much. Building the template is for whatever reason also responsible for detecting whether the current file is an unsaved new file or not.

What this does should be about the same but commenting better what each step does and why + improve the variable names and function names a bit whilst keeping backwards compatibility.

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Sep 12, 2024

Ok, please put that into PR description. Because current description has no value (maybe as additional information).

Copy link
Contributor Author

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

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

Well, I can't approve my own PR.
but, it works as expected on my side.
Also, I like the informative log.
Also, I think it's redundant to log __ last_workfile_path . Also, it'll print file path with version 1 if there is not workfile yet.

Here are some test runs:

  1. Launching Houdini with no existing workfiles.
>>> [  Found template at: '\\storage\studio\templates\my_houdini_template.hip'  ] 
>>> [  Building the workfile template: \\storage\studio\templates\my_houdini_template.hip  ] 
>>> [  __ last_workfile_path: \\storage\work\ayon_projects\Robo\Assets\Character\robococo\work\cfx\robo_robococo_cfx_v001.hip  ] 
>>> [  Saving first workfile: \\storage\work\ayon_projects\Robo\Assets\Character\robococo\work\cfx\robo_robococo_cfx_v001.hip  ] 
  1. Launching Houdini with existing workfiles.

>>> [  Found template at: '\\storage\studio\templates\my_houdini_template.hip'  ] 
>>> [  Building the workfile template: \\storage\studio\templates\my_houdini_template.hip  ] 
>>> [  __ last_workfile_path: \\storage\work\ayon_projects\Robo\Assets\Character\robococo\work\cfx\robo_robococo_cfx_v001.hip  ] 
>>> [  A workfile already exists. Skipping save of workfile as initial version.  ] 
  1. Launch Houdini while having empty workfile template in template builder settings.
//storage/work/ayon_projects/Robo/Assets/Character/robococo/work/cfx
Traceback (most recent call last):
  File "C:\PROGRA~1/SIDEEF~1/HOUDIN~1.805/houdini/python3.9libs\hdefereval.py", line 155, in _processDeferred
    result = code(*args, **kwargs)
  File "E:\Ynput\ayon-houdini\client\ayon_houdini\api\lib.py", line 1404, in start_workfile_template_builder
    build_workfile_template(workfile_creation_enabled=True)
  File "E:\Ynput\ayon-houdini\client\ayon_houdini\api\workfile_template_builder.py", line 135, in build_workfile_template
    builder.build_template(*args, **kwargs)
  File "E:\Ynput\ayon-core\client\ayon_core\pipeline\workfile\workfile_template_builder.py", line 531, in build_template
    preset = self.get_template_preset()
  File "E:\Ynput\ayon-core\client\ayon_core\pipeline\workfile\workfile_template_builder.py", line 847, in get_template_preset
    raise TemplateLoadFailed((
ayon_core.pipeline.workfile.workfile_template_builder.TemplateLoadFailed: Template path is not set.
Path need to be set in Houdini\Template Workfile Build Settings\Profiles

Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

Tested in Nuke and works as expected

@iLLiCiTiT iLLiCiTiT merged commit 616f8ee into develop Oct 2, 2024
5 checks passed
@iLLiCiTiT iLLiCiTiT deleted the bugfix/workfile_builder_on_start branch October 2, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Issues and PRs coming from the community members type: bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants