Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Maya: Handle excluding model family from frame range validator. #3370

Merged
merged 7 commits into from
Jul 5, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jun 20, 2022

Brief description

This resolves #2562 by excluding the model and some other families from having their frameStart and frameEnd attributes validated (which they wouldn't have because of their being geometry).

Description

The validate_frame_range.py currently validates the incorrect subset for published Review instances when there's a nested model subset in it. This fix bypasses the validator checks when it detects the families are of model instance, and some others.

Testing notes:

  1. Create model and then create OP model instance.
  2. Create camera and then create OP review instance.
  3. Append model instance subset into review subset.
  4. Publish.
  5. If frame range is set incorrectly, it should give an error and the validator should be able to repair that error by conforming the review instance frame range to the one set in the mongo document.

@ghost ghost added type: bug Something isn't working host: Maya labels Jun 20, 2022
@ghost ghost self-assigned this Jun 20, 2022
@ynbot
Copy link
Contributor

ynbot commented Jun 20, 2022

Task linked: OP-2449 Maya: Validate Frame Range

@ghost ghost changed the title Handle excluding model family from frame range validator. Maya: Handle excluding model family from frame range validator. Jun 20, 2022
@ghost ghost marked this pull request as draft June 20, 2022 10:49
@ghost ghost requested review from antirotor and m-u-r-p-h-y June 20, 2022 10:49
@BigRoy
Copy link
Collaborator

BigRoy commented Jun 20, 2022

This doesn't seem like a 'clean fix' and feels rather arbitrary? How come the model family is problematic? Wouldn't this e.g. also happen with mayaScene, layout, look, cameraRig, rig families? Or is it solely due to CollectModel which forces a single frame extraction?

Sounds like something that points to another potential deeper issue? Should the "review" family actually be on the model's 'instance'? Aren't we now mixing data of the Review instance and Model instance (or other reviewable instances)?

@mkolar
Copy link
Member

mkolar commented Jun 21, 2022

There are a few problems in play here. Most would be solved by implementing the new publisher in Maya, so we wouldn't have to hack the whole review system, however, that is just constantly slipping in priorities :(.

@BigRoy is right and this is not just an issue with model, but anything that get's nested under a review set inside maya. Right now, it really poses two questions.

  1. Should model instance inherit the review framerange, or keep it's own?
  2. Does it make sense to validate frame ranges whatsoever for publishes that are not shot based?

For 1. I'd say that the review framerange should be propagated to the model (or other nested instance) so that it can be properly validated, if required.

as for 2. we should probably just expose the families attribute to settings, so the studio can choose not to validate frameranges on a set of families for themselves.

@BigRoy
Copy link
Collaborator

BigRoy commented Jun 21, 2022

For 1. I'd say that the review framerange should be propagated to the model (or other nested instance) so that it can be properly validated, if required.

This sounds unwanted since it opens up for motion/multiple time samples to make it's way into the model .abc in the Extract Alembic.

Does it make sense to validate frame ranges whatsoever for publishes that are not shot based?

Good question. I personally think it's not necessarily only a Shot validation. Maybe at best the Project Manager could also have a state where start/end frame is NOT SET like a boolean stating "use frame range".

@mkolar
Copy link
Member

mkolar commented Jun 21, 2022

Good question. I personally think it's not necessarily only a Shot validation. Maybe at best the Project Manager could also have a state where start/end frame is NOT SET like a boolean stating "use frame range".

True, however, I'm not sure it's worth doing it in v3 data model now. In v4 there is differentiation between asset types and they can technically have empty values on attributes, which will make this a lot easier.

This sounds unwanted since it opens up for motion/multiple time samples to make it's way into the model .abc in the Extract Alembic.

Also a very good point. But in that case the only option we have now to make it at least functional is to skip validation on certain families which is exactly what this PR attempts. I just think those families should be configurable

@@ -27,6 +27,7 @@ class ValidateFrameRange(pyblish.api.InstancePlugin):
"yeticache"]
optional = True
actions = [openpype.api.RepairAction]
exclude_families = ["model"]
Copy link
Member

Choose a reason for hiding this comment

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

If we add this to settings, we'll solve the production situation right now and can focus on doing it properly as Roy suggests in v4

@BigRoy
Copy link
Collaborator

BigRoy commented Jun 21, 2022

Also a very good point. But in that case the only option we have now to make it at least functional is to skip validation on certain families which is exactly what this PR attempts. I just think those families should be configurable

Or could we reverse that logic? Only check review instance family frame range if NOT attached to another instance or if another of the families is set to validate with this validator (e.g. animation)?

@antirotor
Copy link
Member

Hmm... to add to it there seems to be probem with Repair Action for render instances (as it is running against non-existent instances in Maya - they are created on-the-fly by collector).

Maybe repair action should run only on those instances that are "physically" present in Maya and have frame range attributes.

@mkolar
Copy link
Member

mkolar commented Jun 24, 2022

Or could we reverse that logic? Only check review instance family frame range if NOT attached to another instance or if another of the families is set to validate with this validator (e.g. animation)?

This is a bit limited to pyblish capabilities right now it's checking in families, whereas, here it would be beneficial to only go by family, I still think that simply excluding model, rig and maybe a few others that are not time based, would be the best option right now.

Allan I. A and others added 3 commits June 28, 2022 14:42
Move default families.

Co-authored-by: Ondřej Samohel <33513211+antirotor@users.noreply.github.com>
@ghost ghost requested review from antirotor and mkolar June 29, 2022 07:07
Copy link
Member

@m-u-r-p-h-y m-u-r-p-h-y left a comment

Choose a reason for hiding this comment

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

The Frame Range Validator always passes no matter what numbers are entered in Review Instance

image

@mkolar
Copy link
Member

mkolar commented Jun 30, 2022

The Frame Range Validator always passes no matter what numbers are entered in Review Instance

that's the whole point of the PR. to not validate on models ;)

Copy link
Member

@m-u-r-p-h-y m-u-r-p-h-y left a comment

Choose a reason for hiding this comment

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

Ohh. I was reading the original Description and was expecting something else.

In this case it works as expected :)

@mkolar mkolar marked this pull request as ready for review July 4, 2022 13:10
@mkolar mkolar merged commit 8a52e70 into develop Jul 5, 2022
@mkolar mkolar deleted the OP-2449/Maya-Validate-Frame-Range branch July 5, 2022 06:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
host: Maya type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maya: Validate Frame Range
5 participants