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

General: We should keep current subset version when we switch only the representation type #4629

Conversation

kaamaurice
Copy link
Contributor

Changelog Description

When we switch only the representation type of subsets, we should not get the representation from the last version of the subset.

Testing notes:

  1. Load a subset with several representation(any host)
  2. Set version that is not the last existing version.
  3. Switch the subset representation using the scene inventory.
  4. Version should not change to the latest.

Additional suggestion

We could implement a version combobox selection to the switch dialog.

@tokejepsen tokejepsen added type: enhancement Enhancements to existing functionality community contribution labels Mar 15, 2023
@tokejepsen
Copy link
Member

Tested in Nuke and it still updates to latest version.

@kaamaurice which hosts did you test this in?

@tokejepsen tokejepsen self-assigned this Mar 15, 2023
@kaamaurice kaamaurice closed this Mar 21, 2023
@kaamaurice kaamaurice deleted the enhancement/general_switch_dialog_keep_current_version branch March 21, 2023 10:39
@ynbot ynbot added this to the next-patch milestone Mar 21, 2023
@tokejepsen
Copy link
Member

@kaamaurice any reason for closing this?

@kaamaurice kaamaurice reopened this Mar 21, 2023
@kaamaurice
Copy link
Contributor Author

@tokejepsen Sorry, I have a very weird issue with branche names how start with enhancement/
VSCode add an upper case and I can't push commits after publishing the branch.

@kaamaurice
Copy link
Contributor Author

Tested with Blender

@kaamaurice kaamaurice requested a review from BigRoy March 21, 2023 10:54
Copy link
Member

@tokejepsen tokejepsen 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 Maya 2023 with model and it still updates to latest version.

@kaamaurice kaamaurice requested a review from BigRoy March 21, 2023 17:24
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.

I gave this a go and on my first try it didn't work and I wanted to respond like @tokejepsen mentioned:

Tested in Maya 2023 with model and it still updates to latest version.

I had the exact same problem. However this only occurs if you select a single container (or multiple containers that are exactly the same asset+subset+representation) in the scene inventory because then by default the switch asset box "preselects" the asset, subset and representation and thus logic fails.

If you remove the asset + subset selection then it works for that case too. But admittedly that's very confusing as you can see from both @tokejepsen and my first test run.
For cases where you select multiple containers those values are not preselected it works right out of the box.

I'd actually like to argue that maybe even for the 'select single container' case we shouldn't be preselecting and then this logic automatically works. It's extra confusing because it has a preselection and lists "No changes*" which technically is correct, but if you empty the field and re-enter exactly the value that it was then suddenly it lists as changes.

OR if we want to keep the automatic selection for switch asset on a single container then we should at least fix how we detect whether the option was explicitly defined by the artist - which apparently IS possible because *No changes is shown below it too - maybe we can just detect the "No changes" in a way.

@jakubjezek001 jakubjezek001 removed this from the next-patch milestone Mar 22, 2023
@tokejepsen tokejepsen assigned kaamaurice and unassigned tokejepsen Mar 22, 2023
@ynbot ynbot added the size/XS Denotes a PR changes 0-99 lines, ignoring general files label Mar 30, 2023
@BigRoy
Copy link
Collaborator

BigRoy commented Apr 20, 2023

Any chance we could see movement int his PR again @tokejepsen @kaamaurice ?

@kaamaurice
Copy link
Contributor Author

Hi ! I'm back ! So sorry for the delay, we had so much work with our blender pipe at Normaal Animation Studio.
I now able to test within Maya, I should be more consistent for the future. Thanks for the reviews.

@kaamaurice kaamaurice force-pushed the enhancement/general_switch_dialog_keep_current_version branch from 4290228 to 1ee6a3b Compare May 5, 2023 12:16
@kaamaurice kaamaurice requested a review from tokejepsen May 5, 2023 14:56
@kaamaurice kaamaurice requested a review from BigRoy May 5, 2023 14:56
@BigRoy BigRoy assigned tokejepsen and unassigned kaamaurice May 8, 2023
@iLLiCiTiT
Copy link
Member

Can someone test this with hero versions?

@kaamaurice
Copy link
Contributor Author

kaamaurice commented May 12, 2023

Can someone test this with hero versions?

Traceback (most recent call last):
  File "D:\Tools\OpenPype\openpype\tools\sceneinventory\switch_dialog.py", line 1181, in _on_accept
    self._trigger_switch()
  File "D:\Tools\OpenPype\openpype\tools\sceneinventory\switch_dialog.py", line 1257, in _trigger_switch
    container_version_name = container_version["name"]
KeyError: 'name'

Oups I have an issue, sorry, I fix this !

@kaamaurice
Copy link
Contributor Author

Works now as expected with hero version.

@mkolar
Copy link
Member

mkolar commented May 30, 2023

This is looking good, however whatever I do I can't checkout the branch. Actually i can't even find in when I add you remote @kaamaurice .

@kaamaurice
Copy link
Contributor Author

@mkolar Thanks for the review. I had some weird issue with VS code when I create and publish a new branch how start with enhancement/ ! I get a E uppercase in my remote branch but my local branch stile named without uppercase.

So checkout Enhancement/general_switch_dialog_keep_current_version should work

@Tilix4
Copy link
Collaborator

Tilix4 commented Jun 21, 2023

Hi @mkolar, did you have time to review since the last changes?

@iLLiCiTiT iLLiCiTiT requested a review from antirotor June 28, 2023 08:50
Copy link
Member

@iLLiCiTiT iLLiCiTiT left a comment

Choose a reason for hiding this comment

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

Changes make sense. LGTM

@iLLiCiTiT iLLiCiTiT merged commit 91a1fb1 into ynput:develop Jan 4, 2024
13 checks passed
@ynbot ynbot added this to the next-patch milestone Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community contribution size/XS Denotes a PR changes 0-99 lines, ignoring general files type: enhancement Enhancements to existing functionality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

9 participants