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

OP-2566: remove underscore from subset name #4059

Conversation

moonyuet
Copy link
Member

@moonyuet moonyuet commented Nov 2, 2022

Brief description

remove unnecessary underscore from subset name when rendering without AOVs

Description

When publishing the render with the empty AOV name, it will create subset name with underscore at the end. (See the screenshot below)
image

This PR will fix the issue of adding extra underscore at the end when publishing the render.
image

Additional info

This is only tested in maya Host.

Testing notes:

  1. Launch Maya in Openpype
  2. Create Render Main
  3. In Render Setting, add an AOV and empties the AOV name
  4. Save and Publish the scene.

@moonyuet moonyuet self-assigned this Nov 2, 2022
@github-actions github-actions bot added this to the next-patch milestone Nov 2, 2022
@moonyuet moonyuet changed the title bugfix/ remove underscore from subset name remove underscore from subset name Nov 2, 2022
@LiborBatek
Copy link
Member

LiborBatek commented Nov 3, 2022

I have made a test with Specular AOV added besides the default beauty (with removed AOV name for that Specular AOV) with this outcome...
Github_PR_pic_01

However I need you be more specific what conditions needed....should I just remove the name as I did for AOV? (its not very clear from your testing notes). See my picture. Also dont know if merge AOVs should be on or off. I have used defaults which is mergeAOVs to single exr file.

When I have these informations I will redo it again if need be. No it looks fine without any additional underscore in the name but not 100% sure if its okey right now.

Thx.

@moonyuet
Copy link
Member Author

moonyuet commented Nov 3, 2022

I have made a test with Specular AOV added besides the default beauty (with removed AOV name for that Specular AOV) with this outcome... Github_PR_pic_01

However I need you be more specific what conditions needed....should I just remove the name as I did for AOV? (its not very clear from your testing notes). See my picture. Also dont know if merge AOVs should be on or off. I have used defaults which is mergeAOVs to single exr file.

When I have these informations I will redo it again if need be. No it looks fine without any additional underscore in the name but not 100% sure if its okey right now.

Thx.

Hi Libor! I tested the code without turning on MergeAOVs, so that I can check to see if there's any extra underscore created after the AOV with the empty name. It's a weird ticket, you can check more information in ClickUp: https://app.clickup.com/t/6658547/OP-2566
https://app.clickup.com/t/6658547/OP-2622

@LiborBatek
Copy link
Member

LiborBatek commented Nov 4, 2022

tested in maya2022 and maya2023
Ok, so I have made another go of test with "mergeAOVs" turned off and it works ok...

I have Main layer (beauty) + N pass (without deleting AOV name) + Specular AOV (with deleted name)...
creates

renderLightingMain_beauty
renderLightingMain (the spec AOV with deleted name) doesnt add any underscore into the name so OK!
renderLightingMain_N

image

Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

Seems working in both maya2022 and maya2023

@mkolar mkolar marked this pull request as ready for review November 7, 2022 14:53
@mkolar mkolar merged commit 37eb905 into ynput:develop Nov 7, 2022
Comment on lines 459 to +468
if cam:
subset_name = '{}_{}_{}'.format(group_name, cam, aov)
if aov:
subset_name = '{}_{}_{}'.format(group_name, cam, aov)
else:
subset_name = '{}_{}'.format(group_name, cam)
else:
subset_name = '{}_{}'.format(group_name, aov)
if aov:
subset_name = '{}_{}'.format(group_name, aov)
else:
subset_name = '{}'.format(group_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this have been more succinct with something like this?

    subset_name = group_name
    if cam:
        subset_name += "_{}".format(cam)
    if aov:
        subset_name += "_{}".format(aov)

Or even:

    subset_name = group_name
    for suffix_element in [cam, aov]:
        if suffix_element:
            subset_name += "_{}".format(suffix_element)

Or even:

    parts = [part for part in [group_name, cam, aov] if part]
    subset_name = "_".join(parts)

Ok, that last one might be a bit too far for readability 😆

@BigRoy
Copy link
Collaborator

BigRoy commented Nov 7, 2022

Just be sure - this is a backwards incompatible change in that existing renders would now generate a new subset without the underscore, correct?

@antirotor antirotor changed the title remove underscore from subset name OP-2566: remove underscore from subset name Nov 24, 2022
@ynbot
Copy link
Contributor

ynbot commented Nov 24, 2022

Task linked: OP-2566 remove underscore from subset name

@BigRoy
Copy link
Collaborator

BigRoy commented Jun 27, 2023

Just be sure - this is a backwards incompatible change in that existing renders would now generate a new subset without the underscore, correct?

Just updated some things on our end and it seems this is definitely backwards incompatible.

Is there any way for legacy renders/scenes to still include the trailing _ to remain backwards compatible? Was there a setting for a project for that?

We're now getting publishes with a new name:

image (10)

BigRoy added a commit to BigRoy/OpenPype that referenced this pull request Jun 27, 2023
@BigRoy
Copy link
Collaborator

BigRoy commented Jun 27, 2023

For now I added legacy support for a project on our end: BigRoy@92d12f2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants