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

Bugfix: houdini switching context doesnt update variables #5651

Conversation

MustafaJafar
Copy link
Contributor

@MustafaJafar MustafaJafar commented Sep 25, 2023

Changelog Description

Allows admins to have a list of vars (e.g. JOB) with (dynamic) values that will be updated on context changes, e.g. when switching to another asset or task.

Using template keys is supported but formatting keys capitalization variants is not, e.g. {Asset} and {ASSET} won't work

Note

If is Dir Path toggle is activated, Openpype will consider the given value as a path to a folder.

If the folder does not exist on the context change it will be created by this feature so that the path will always try to point to an existing folder.

Disabling Update Houdini vars on context change feature will leave all Houdini vars unmanaged and thus no context update changes will occur.

If $JOB is present in the Houdini var list and has an empty value, OpenPype will set its value to $HIP

image

image

Also, this PR adds a new button in menu to update vars on demand.
image

Testing notes:

  1. try different job path combinations
  2. open houdini files and switch contexts

@MustafaJafar MustafaJafar added type: bug Something isn't working type: enhancement Enhancements to existing functionality host: Houdini labels Sep 25, 2023
@ynbot
Copy link
Contributor

ynbot commented Sep 25, 2023

@ynbot ynbot added the size/S Denotes a PR changes 100-499 lines, ignoring general files label Sep 25, 2023
@MustafaJafar MustafaJafar linked an issue Sep 25, 2023 that may be closed by this pull request
openpype/hosts/houdini/api/lib.py Outdated Show resolved Hide resolved
openpype/hosts/houdini/api/lib.py Outdated Show resolved Hide resolved
openpype/hosts/houdini/api/lib.py Outdated Show resolved Hide resolved
openpype/hosts/houdini/api/lib.py Outdated Show resolved Hide resolved
@MustafaJafar MustafaJafar force-pushed the bugfix/OP-4326_Houdini-switching-context-doesnt-update-variables branch from 654c9fa to 32a88fd Compare September 26, 2023 07:52
@MustafaJafar MustafaJafar force-pushed the bugfix/OP-4326_Houdini-switching-context-doesnt-update-variables branch from 0ba8f02 to a349733 Compare September 26, 2023 17:39
@moonyuet
Copy link
Member

i have tested the code again.
If "$JOB path update on context change " is enabled and the JOB env does not exist, the dialog would be in red .

image

But if the toggle is disable, the $JOB env is the same with the $HIP env.

image

@BigRoy
Copy link
Collaborator

BigRoy commented Sep 27, 2023

Should we add admin docs for this feature?

@moonyuet
Copy link
Member

Should we add admin docs for this feature?

Yes, we should. At least let the user know $JOB would be not showed or errored out if they enabled the toggled button with the non-existent path.

@BigRoy
Copy link
Collaborator

BigRoy commented Oct 5, 2023

Can you share a screenshot of the current dialog you get when context changes?

@MustafaJafar
Copy link
Contributor Author

Can you share a screenshot of the current dialog you get when context changes?

image

@fabiaserra
Copy link
Contributor

@fabiaserra Any chance you could contribute that update vars pop-up logic to this branch? I feel like the feature would align one-to-one.

First we need to collect what vars will change and what their current values are. This will need to have a function available for that @MustafaJafar so we need to update the update_houdini_vars_context() function to use that too.

I assume it would be something like this?

pseudocode

def get_context_var_changes():
    """Return asset context variables that need to be updated"""
    vars_to_update = {}
    vars_to_update["RESX"] = (old, new)
    return vars_to_update

def update_houdini_vars_context():
    """Update asset context variables"""
    vars_to_update = get_context_var_changes()
    for var, (old, new) in get_context_var_changes().items():
        # perform the update
        # ...

def update_houdini_vars_context_dialog():
    """Show pop-up to update asset context variables"""
    update_vars = get_context_var_changes()
    if not update_vars:
        # Nothing to change
        return
    
    message = "\n".join(
        "${}: {} -> {}".format(var, old, new)
        for var, (old, new) in update_vars.items()
    )
    
    dialog = popup.PopupUpdateKeys(parent=parent)
    dialog.setModal(True)
    dialog.setWindowTitle("Houdini scene has outdated asset variables")
    dialog.setMessage(message)
    dialog.setButtonText("Fix")

    # on_show is the Fix button clicked callback
    dialog.on_clicked_state.connect(lambda: update_houdini_vars_context())

    dialog.show()

But maybe a slightly better dialog could also be nice to implement since the "Update keys []" in your example seems weird (due to re-using a different pop-up of course)

Hey yeah I'm sorry I've been hammered in my studio firefighting production needs and haven't had time to do what I said I would. Let me share here the quick and dirty implementation I did of this:

I added this on the on_open function of houdini.api.pipeline:

def on_open():

    if not hou.isUIAvailable():
        log.debug("Batch mode detected, ignoring `on_open` callbacks..")
        return

    log.info("Running callback on open..")

    outdated_asset_variables = lib.get_outdated_asset_variables()

    if outdated_asset_variables:
        parent = lib.get_main_window()
        if parent is None:
            # When opening Houdini with last workfile on launch the UI hasn't
            # initialized yet completely when the `on_open` callback triggers.
            # We defer the dialog popup to wait for the UI to become available.
            # We assume it will open because `hou.isUIAvailable()` returns True
            import hdefereval
            hdefereval.executeDeferred(
                lib.show_outdated_asset_variables_popup,
                outdated_asset_variables
            )
        else:
            lib.show_outdated_asset_variables_popup(outdated_asset_variables)

        log.warning("Scene has outdated context variables.")

And these are the functions that Ia dded on the houdini.lib:

def set_scene_resolution(width, height, pix_aspect):
    if width:
        hou.hscript(f"set -g RESX={width}")
        hou.hscript("varchange -V RESX")
    if height:
        hou.hscript(f"set -g RESY={height}")
        hou.hscript("varchange -V RESY")
    if pix_aspect:
        hou.hscript(f"set -g PIX_AR={pix_aspect}")
        hou.hscript("varchange -V PIX_AR")


def get_outdated_asset_variables():
    outdated_variables = {}

    asset_doc = get_current_project_asset()

    # Validate FPS
    fps = get_asset_fps()
    current_fps = hou.fps()  # returns float

    if current_fps != fps:
        outdated_variables["FPS"] = (current_fps, fps)

    # Validate resolution
    width, height, pix_aspect = get_resolution_from_doc(asset_doc)
    cur_width = hou.getenv("RESX")
    cur_height = hou.getenv("RESY")
    cur_pix_aspect = hou.getenv("PIX_AR")

    if width != cur_width:
        outdated_variables["RESX"] = (cur_width, width)

    if height != cur_height:
        outdated_variables["RESY"] = (cur_height, height)

    if pix_aspect != cur_pix_aspect:
        outdated_variables["PIX_AR"] = (cur_pix_aspect, pix_aspect)

    return outdated_variables


def show_outdated_asset_variables_popup(outdated_variables):
    # Get main window
    parent = get_main_window()
    if parent is None:
        log.info("Skipping outdated content pop-up "
                 "because Houdini window can't be found.")
    else:
        from openpype.widgets import popup
        dialog = popup.PopupUpdateKeys(parent=parent)
        dialog.setModal(True)
        dialog.setWindowTitle("Houdini scene has outdated asset variables")
        outaded_vars_list = []
        for variable_name, outdated_value in outdated_variables.items():
            cur_value, new_value = outdated_value
            outaded_vars_list.append(
                f"${variable_name}: {cur_value} -> {new_value}"
            )

        dialog.setMessage("\n".join(outaded_vars_list))
        dialog.setButtonText("Fix")

        new_width = None
        outdated_width = outdated_variables.get("RESX")
        if outdated_width:
            new_width = outdated_width[1]

        new_height = None
        outdated_height = outdated_variables.get("RESY")
        if outdated_height:
            new_height = outdated_height[1]

        new_pix_aspect = None
        outdated_pix_aspect = outdated_variables.get("PIX_AR")
        if outdated_pix_aspect:
            new_pix_aspect = outdated_pix_aspect[1]

        new_fps = None
        outdated_fps = outdated_variables.get("FPS")
        if outdated_fps:
            new_fps = outdated_fps[1]

        # on_show is the Fix button clicked callback
        dialog.on_clicked_state.connect(
            lambda: (
                set_scene_resolution(new_width, new_height, new_pix_aspect),
                set_scene_fps(new_fps)
            )
        )

        dialog.show()

Notice that I changed the existing get_resolution_from_doc to also return pixel aspect:

def get_resolution_from_doc(doc):
    """Get resolution from the given asset document. """

    if not doc or "data" not in doc:
        print("Entered document is not valid. \"{}\"".format(str(doc)))
        return None

    resolution_width = doc["data"].get("resolutionWidth")
    resolution_height = doc["data"].get("resolutionHeight")
    pixel_aspect = doc["data"].get("pixelAspect")

    # Make sure both width and height are set
    if resolution_width is None or resolution_height is None \
            or pixel_aspect is None:
        print("No resolution information found for \"{}\"".format(doc["name"]))
        return None

    return int(resolution_width), int(resolution_height), float(pixel_aspect)

This is very dirty and we should make a proper Qt dialog that's more user friendly and ideally something we can reuse in all other OP DCCs for startup dialog

@fabiaserra
Copy link
Contributor

This startup dialog should show up automatically every time an artist opens (not saves) a scene and has outdated variables. Doing it as an on-demand button it's helpful for the cases you want to validate or trigger an update because you know some have been updated but the most common case should be when an artist starts to work on a scene, they want to know if there's been an update on any of those and choose if he wants to pull them or not

@MustafaJafar
Copy link
Contributor Author

MustafaJafar commented Oct 5, 2023

@fabiaserra Never mind man!

you could add FPS to houdini vars, but that's redundant because lib.validate_fps() do that and it's registered in on_open and on_save callbacks.

I made a quick test with this PR

  1. I added these vars in settings
    image

  2. I made these changes in project manager
    image

  3. here's what I found when I opened my file
    image

  4. also, it's possible to update on demand from this menu button
    image

@fabiaserra
Copy link
Contributor

fabiaserra commented Oct 5, 2023

@fabiaserra Never mind man!

you could add FPS to houdini vars, but that's redundant because lib.validate_fps() do that and it's registered in on_open and on_save callbacks.

I made a quick test with this PR

  1. I added these vars in settings
    image
  2. I made these changes in project manager
    image
  3. here's what I found when I opened my file
    image
  4. also, it's possible to update on demand from this menu button
    image

Nice! that's looking good. Could we spend some cycles on this PR to make a better Qt dialog for this use case? We should be able to update each of the variables independently (although resolution I would probably expect to be a single entry)

@MustafaJafar
Copy link
Contributor Author

Nice! that's looking good. Could we spend some cycles on this PR to make a better Qt dialog for this use case? We should be able to update each of the variables independently (although resolution I would probably expect to be a single entry)

It's a great idea, however I prefer to add it as a # TODO in code because the main concern of this PR to update the Houdini vars when context changes.

@BigRoy
Copy link
Collaborator

BigRoy commented Oct 6, 2023

It's a great idea, however I prefer to add it as a # TODO in code because the main concern of this PR to update the Houdini vars when context changes.

Can you create a Github issue to track it once this PR is merged?

The UX on path changes (since they are long) felt way off in this screenshot.

@moonyuet
Copy link
Member

moonyuet commented Oct 6, 2023

Nice! that's looking good. Could we spend some cycles on this PR to make a better Qt dialog for this use case? We should be able to update each of the variables independently (although resolution I would probably expect to be a single entry)

@fabiaserra there there is popup dialog which can be imported by using from openpype.widgets import popup.
That one is used for checking outdated container and user can click fix button for updating container. Not sure it is a good choice and this Qtdialog looks so similar to that popup dialog(I used it for setting ocio config in 3dsmax in regards to the DCC's launching ocio config in 2024 while it doesn't have any feature before that)

@BigRoy
Copy link
Collaborator

BigRoy commented Oct 6, 2023

@fabiaserra there there is popup dialog which can be imported by using from openpype.widgets import popup.
That one is used for checking outdated container and user can click fix button for updating container. Not sure it is a good choice and this Qtdialog looks so similar to that popup dialog(I used it for setting ocio config in 3dsmax in regards to the DCC's launching ocio config in 2024 while it doesn't have any feature before that)

That's the one currently being used, also see this comment.

@moonyuet
Copy link
Member

moonyuet commented Oct 6, 2023

@fabiaserra there there is popup dialog which can be imported by using from openpype.widgets import popup.
That one is used for checking outdated container and user can click fix button for updating container. Not sure it is a good choice and this Qtdialog looks so similar to that popup dialog(I used it for setting ocio config in 3dsmax in regards to the DCC's launching ocio config in 2024 while it doesn't have any feature before that)

That's the one currently being used, also see this comment.

Yes I noticed that just now too :). So ignore my previous comment.

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.

Code looks good for a first prototype. Solid work.

@mkolar mkolar merged commit d41ac2a into develop Oct 6, 2023
1 check passed
@mkolar mkolar deleted the bugfix/OP-4326_Houdini-switching-context-doesnt-update-variables branch October 6, 2023 16:41
@ynbot ynbot added this to the next-patch milestone Oct 6, 2023
@fabiaserra
Copy link
Contributor

Nice! that's looking good. Could we spend some cycles on this PR to make a better Qt dialog for this use case? We should be able to update each of the variables independently (although resolution I would probably expect to be a single entry)

@fabiaserra there there is popup dialog which can be imported by using from openpype.widgets import popup. That one is used for checking outdated container and user can click fix button for updating container. Not sure it is a good choice and this Qtdialog looks so similar to that popup dialog(I used it for setting ocio config in 3dsmax in regards to the DCC's launching ocio config in 2024 while it doesn't have any feature before that)

Yeah that's the same one I used for my dirty implementation of this... but it's just so Windows 98 haha I wish I could show you a screenshot of the startup dialog we had in my prior company, it was so pretty and clear for the artists. I think a simple vertical layout with horizontal layouts of three QLabels (variable, current and new value) and an "Update" button next to it, inheriting the stylesheet of the OP widgets with a title of "Outdated scene variables" and the OP logo would have already gone a long way and shouldn't take more than an hour to write

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Houdini size/S Denotes a PR changes 100-499 lines, ignoring general files type: bug Something isn't working type: enhancement Enhancements to existing functionality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Houdini: Switching context doesn't update variables
8 participants