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

Max: Implementation of Camera Attributes Validator#6110

Merged
mkolar merged 13 commits intodevelopfrom
enhancement/OP-7075_Validate-Camera-Attributes
Feb 22, 2024
Merged

Max: Implementation of Camera Attributes Validator#6110
mkolar merged 13 commits intodevelopfrom
enhancement/OP-7075_Validate-Camera-Attributes

Conversation

@moonyuet
Copy link
Copy Markdown
Member

@moonyuet moonyuet commented Jan 8, 2024 •
edited
Loading

Changelog Description

Implement Validate Camera Attributes in camera family in Max host

Additional info

  • This validator is not eligible for the camera created from the external plugins such as Redshift and Arnold etc.
  • When the values of the camera attributes in the setting set to 0, the validator would skip checking the attribute.

Testing notes:

  1. Launch Max via launcher
  2. Tweak the settings ayon+settings://max/publish/ValidateCameraAttributes or project_settings/max/publish/ValidateCameraAttributes
    image
    image
  3. Create Camera Instance
  4. Publish
  5. If the camera attributes dont align with those attributes in the setting, the validator would error out.

@ynbot
Copy link
Copy Markdown
Contributor

ynbot commented Jan 8, 2024

Task linked: OP-7075 Validate Camera Attributes

@ynbot ynbot added size/S Denotes a PR changes 100-499 lines, ignoring general files host: 3dsmax Autodesk 3dsmax type: enhancement Enhancements to existing functionality labels Jan 8, 2024
@LiborBatek
Copy link
Copy Markdown
Member

LiborBatek commented Jan 9, 2024 •
edited
Loading

First finding is that the Camera Attributes option in the Publisher should be named a bit differently to be more clear imho...

image

something like> Validate Attributes or Validate Camera Attributes which a bit loong

also when the Validator kicks in the message could be more "human readible" imho... in this case FOV value being different to 45 also not sure whats the rest of the invalid attributes atm ...kinda cluttered report with a lot of "guts" exposed

image

@LiborBatek
Copy link
Copy Markdown
Member

LiborBatek commented Jan 9, 2024

When tested with Near and Far ranges for enviro and clipping I have been able to Publish camera successfully even with wrong values...maybe when not turned on those checkers for Show and Clip Manually ??

Screenshot 2024-01-09 103936

however next time trying publishing the Validator kicked in correctly but again the messaging could be strongly improved imho

Screenshot 2024-01-09 104329

@LiborBatek
Copy link
Copy Markdown
Member

LiborBatek commented Jan 9, 2024 •
edited
Loading

Not sure whats the origin of this PR but right now as we have it theres is no way to validate just a few of the values but all together.... theres no way to selectively choose what to validate or at least to disable particular value?

Maybe would be option to use 0 Value to disable each entry? Something like validating against zero means skip this value in validation ??

image

For example:

when Focal Length been set to 0 ....then the Validate might skip this entry
also meaning that each entry does need some legit value besides zero to validate something like

Near Range must have value at least like 0.001 or similar.

Does it make sense this proposal or not?? thx

@moonyuet
Copy link
Copy Markdown
Member Author

moonyuet commented Jan 9, 2024 •
edited
Loading

Not sure whats the origin of this PR but right now as we have it theres is no way to validate just a few of the values but all together.... theres no way to selectively choose what to validate or at least to disable particular value?

Maybe would be option to use 0 Value to disable each entry? Something like validating against zero means skip this value in validation ??

image

the initial purpose of this PR is to build the Camera Attributes validator in Max which is similar to Maya. But the Maya one is more hard-coded and I am not sure it will be good idea for Max(That's why you see all the settings!). But I agree with you that we can just skip the check when the parameter set to zero

LiborBatek
LiborBatek suggested changes Jan 10, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

Im getting validate errors even when my values set on the camera matches the validator....

Screenshot 2024-01-10 115432

When I remove the Camera Instance and recreate it from scratch it suddenly works as expected and validate correctly.

Screenshot 2024-01-10 115627

@moonyuet moonyuet marked this pull request as draft January 10, 2024 11:31
@moonyuet
Copy link
Copy Markdown
Member Author

moonyuet commented Jan 11, 2024

@LiborBatek I have updated the commit and the changelog.
The validation should check if the camera attributes aligning with the settings.
You may find the video below helpful.

camera_attributes_validator.mp4

Do you think that it's also good idea to add repair action for the camera attributes?

@moonyuet moonyuet requested a review from LiborBatek January 11, 2024 05:54
@moonyuet moonyuet marked this pull request as ready for review January 11, 2024 05:54
@LiborBatek
Copy link
Copy Markdown
Member

LiborBatek commented Jan 11, 2024

Do you think that it's also good idea to add repair action for the camera attributes?

Yeah would be nice! ...Did u manage to incorporate those zero values for skipping the validation or not yet?

@moonyuet
Copy link
Copy Markdown
Member Author

moonyuet commented Jan 11, 2024

Yeah would be nice!

Okay I will add it into the next commit

.Did u manage to incorporate those zero values for skipping the validation or not yet?

Yes, it was already there in the updated commit.

LiborBatek
LiborBatek suggested changes Jan 11, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@LiborBatek LiborBatek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Screenshot 2024-01-11 132306

I have tested all sort of combinations of values used for validation and my findings so far being:

  • Repair Func works as expected
  • Validators set to 0 arent taken into account as expected
  • Values used for validation being in generic units which is ok but will give different value after Repair action if users scene being in different unit system - if switched later afterwards the values matches properly.
  • *Entering values for Near Range: 0.1 and Far Range: 999.5 wont pass Validate but 1 and 1000 yes.

*I mean in an occasion that the repair did fill in correct values and they are matching in theory...still wont pass in first example.

So there are 2 issues now, question being if we should take into account Scene Units or just note it in the OP Settings via docstring that those Values being in generic units and some conversion gonna happen if not using the same.

The Validation issue seems tied to the numbers format and invalid logic for decimal values?? Now just guessing tho.

@moonyuet
Copy link
Copy Markdown
Member Author

moonyuet commented Jan 11, 2024 •
edited
Loading

So there are 2 issues now, question being if we should take into account Scene Units or just note it in the OP Settings via docstring that those Values being in generic units and some conversion gonna happen if not using the same.

Yes I think we can mention that those default values are being in generic units in both settings and validators, so that the user can change the value as their own default in regard to what they set in scene units. (we can possibly add one more check on the scene units to give this message too)

The Validation issue seems tied to the numbers format and invalid logic for decimal values?? Now just guessing tho.

I need to double check, it is supposed that it tied to the float format. EDIT: yes, i will fix in the next commit.

@moonyuet moonyuet requested a review from LiborBatek January 11, 2024 14:46
LiborBatek
LiborBatek approved these changes Jan 12, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@LiborBatek LiborBatek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Now it all works fine! Even when having values for validating like

0.1 and 999.5

Repair function also works as expected and when using zero value in the OP Settings it correctly ignores / skip Validation.

image

@moonyuet moonyuet requested a review from Minkiu January 12, 2024 10:01
@moonyuet moonyuet added sponsored Client endorsed or requested target: OpenPype target: AYON labels Jan 15, 2024
@moonyuet moonyuet mentioned this pull request Feb 8, 2024
Closed
@mkolar
Copy link
Copy Markdown
Member

mkolar commented Feb 9, 2024

Looks good for merge. @moonyuet port to ayon pls

@moonyuet moonyuet mentioned this pull request Feb 13, 2024
Merged
@moonyuet
Copy link
Copy Markdown
Member Author

moonyuet commented Feb 13, 2024

Looks good for merge. @moonyuet port to ayon pls

Already ported to ayon-core!

@mkolar mkolar merged commit 10ec40b into develop Feb 22, 2024
@mkolar mkolar deleted the enhancement/OP-7075_Validate-Camera-Attributes branch February 22, 2024 17:12
@ynbot ynbot added this to the next-patch milestone Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@antirotor antirotor Awaiting requested review from antirotor

@Minkiu Minkiu Awaiting requested review from Minkiu

1 more reviewer

@LiborBatek LiborBatek LiborBatek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@antirotor antirotor

Labels

host: 3dsmax Autodesk 3dsmax port to AYON size/S Denotes a PR changes 100-499 lines, ignoring general files sponsored Client endorsed or requested target: AYON target: OpenPype type: enhancement Enhancements to existing functionality

Projects

PR reviewing
Archived in project

Milestone

3.18.8

Development

Successfully merging this pull request may close these issues.

5 participants

@moonyuet @ynbot @LiborBatek @mkolar @antirotor