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

Fusion: Also update saver tool path on only task or asset change #231

Merged

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Mar 25, 2024

Changelog Description

Also update saver tool path on only task or asset change.

Without this the path would change to the other asset/task if you combined that with a change to subset or image format but not if you only changed asset or task. Which made it quite confusing and could potentially have been overwriting files on the wrong location (not particular to that asset or task it switched to). This changes makes it way more reliable, switching asset or task will result in the new path.

Additional info

n/a

Testing notes:

  1. Fusion publishing of render and image should work.
  2. Changing asset or task (or both) should update the saver's output path.

@ynbot ynbot added host: Fusion size/XS type: enhancement Improvement of existing functionality or minor addition labels Mar 25, 2024
@EmberLightVFX
Copy link
Contributor

Maybe I'm doing something wrong but there is no difference for me with or without this pr.
For me if I simply change the task type from comp to animation, save the change and validate, publish or close the publisher the path in my saver changes to the correct path.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Mar 25, 2024

Maybe I'm doing something wrong but there is no difference for me with or without this pr. For me if I simply change the task type from comp to animation, save the change and validate, publish or close the publisher the path in my saver changes to the correct path.

Ok, so the reason for that is because the subset includes the task name by default in the subset naming template. However, if you were to ONLY switch the folder to another with the same task name then without this PR the filepath would not update.

Also, if you were to change the templates for your project so that the subset does not include the task name then also on task change it would not change the save path to the other folder.

Anyway, in short, you got 'lucky' with the fact that the task name change influences the subset name and thus would be captured as a change already in the original design.

@EmberLightVFX let me know if you can confirm that behavior.

@MustafaJafar MustafaJafar added the community Issues and PRs coming from the community members label Mar 26, 2024
@mkolar mkolar requested a review from iLLiCiTiT March 27, 2024 08:56
@kalisp
Copy link
Member

kalisp commented Apr 2, 2024

It is failing for me when changing asset:
image

@kalisp kalisp self-requested a review April 2, 2024 16:00
@BigRoy
Copy link
Collaborator Author

BigRoy commented Apr 2, 2024

It is failing for me when changing asset: image

Should be fixed with 98bed9f

@kalisp
Copy link
Member

kalisp commented Apr 3, 2024

Should it change folder path when I change asset in the Publisher? It does not in Fusion, change in product name propagates, but character stays the same in the path.

image

@EmberLightVFX
Copy link
Contributor

EmberLightVFX commented Apr 3, 2024

Should it change folder path when I change asset in the Publisher? It does not in Fusion, change in product name propagates, but character stays the same in the path.

Did you save the change?

2024-04-03_11-21-25

@kalisp
Copy link
Member

kalisp commented Apr 3, 2024

YY, I did.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Apr 3, 2024

YY, I did.

Do you get any errors in the console? Are you able to debug up to where the code triggers? Does your instance somehow not have productName data due to it being a legacy older instance? (No idea why it might not trigger for you)

Does it work for a newly created instance?

@kalisp
Copy link
Member

kalisp commented Apr 3, 2024

I think that the issue is that workdir in formatting_data doesn't get refreshed, it gets pulled only from AYON_WORKDIR env var, which will not change if you change asset in Publisher.
image

@BigRoy
Copy link
Collaborator Author

BigRoy commented Apr 3, 2024

I think that the issue is that workdir in formatting_data doesn't get refreshed, it gets pulled only from AYON_WORKDIR env var, which will not change if you change asset in Publisher. image

Ok, so - I have implemented a change that should work for this. But admittedly, I'm not sure if I really like that having that code there. It feels cluttered and likely slow for many instances. I wonder if we have better options than this.

@EmberLightVFX @iLLiCiTiT thoughts?

Copy link
Member

@kalisp kalisp left a comment

Choose a reason for hiding this comment

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

This works.

Do we think that this would have such a massive occurrence (changing asset from current context) that it would have any noticeable impact?

@BigRoy
Copy link
Collaborator Author

BigRoy commented Apr 4, 2024

Do we think that this would have such a massive occurrence (changing asset from current context) that it would have any noticeable impact?

I don't suspect day-to-day usage should see performance issues due to this. I'd have just preferred this to be much simpler and cleaner than this.

@BigRoy BigRoy assigned jakubjezek001 and unassigned BigRoy Apr 4, 2024
@kalisp kalisp merged commit 70f2608 into ynput:develop Apr 5, 2024
5 of 6 checks passed
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 size/XS type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants