Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Maya: Use resolution and frame range from task entity #39
Maya: Use resolution and frame range from task entity #39
Changes from 1 commit
a53e75f
5d92d22
c130fe5
5a9b4f2
b6ea398
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to query this data in a separate class method so the logic isn't duplicated between here and
repair
E.g.
We could potentially even improve the readability by having a FrameRange dataclass.
For example:
Then the method becomes:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore I still think this should check the frame range against the entity of the instance - not the current asset so that you still validate to the asset you're publishing to, not where you're publishing from.
So likely we'll also need to collect the task entity for the instance - and use that one here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented the get_instance_task_frame_range, and FrameRange object as suggested in the validate_frame_range.py and pushed the changes now.
I'm not sure what you mean by this below. The validation and repair action seem to be doing the right thing in my testing so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create two instances in your scene, change folder + task on one of the instances via the publisher UI.
Your one scene, now publishes into two different folder + task entities.
The frame range validation should now validate the frame range of the selected task entity - not from your current context. So make sure in the database that each folder (and its task?) entity has a different frame range set so you can confirm the validation adheres to that actual selected task entity's frame range.
The current logic relies on
instance.context
of which during publishing there's only one - and thus I'm quite confident this currently does not work the way I describe.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BigRoy is it just a matter of storing taskEntity in the instance data. so it gets stored per instance, rather than per context.
instance.data["taskEntity"]
however assetEntity is collected in collect_context_entities.py, so wouldn't it make sense to collect the taskEntity in the same place?
Once per context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BigRoy I updated the collection of taskEntity to happen within collect_instances.py rather than collect_context_entities.py,
https://github.com/ynput/ayon-core/pull/39/files#diff-13c18dd7cb684126254981e14bcf1590c1c5be889bc327ce05bbddc404d2133fR96
Is this the right approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BigRoy do you have any further information about approach above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, along those lines.
Basically the same behavior as is done for "assets" here basically inheriting the asset entity from context if it matches the context otherwise querying the differing asset entity instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BigRoy sorry I do not follow.
is it possible for you to propose a code change for this?
It's really difficult to follow in written form.
Or is my code already basically doing the right thing.