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

Maya: Validate Model Content improve validation message #267

Merged

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Mar 26, 2024

Changelog Description

Validator for model product type content should provide a better error message.

Additional info

Also fix get_invalid actually returning the instance node.

Testing notes:

  1. Publish model with invalid geometry, e.g. a proxy shape inside - it should be captured by this validator.

- Also fix `get_invalid` actually returning the instance node.
@ynbot ynbot added type: enhancement Improvement of existing functionality or minor addition size/XS host: Maya labels Mar 26, 2024
@MustafaJafar MustafaJafar added the community Issues and PRs coming from the community members label Mar 27, 2024
Copy link
Contributor

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

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

Tested as user.
It works on my side.

image

image

@BigRoy
Copy link
Collaborator Author

BigRoy commented Mar 27, 2024

Actually - let me actually improve the "description" for this validator. Because this still doesn't look very nice.

@BigRoy BigRoy marked this pull request as draft March 27, 2024 11:53
@BigRoy BigRoy self-assigned this Mar 27, 2024
@BigRoy BigRoy marked this pull request as ready for review April 4, 2024 15:10
@BigRoy BigRoy requested a review from MustafaJafar April 4, 2024 15:10
@BigRoy
Copy link
Collaborator Author

BigRoy commented Apr 4, 2024

Report should be improved now.

@BigRoy BigRoy assigned MustafaJafar and unassigned BigRoy Apr 4, 2024
Copy link
Contributor

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

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

This is helpful!

image

image

Copy link
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.

I have ran several different test occasions with following results (in terms of Validation message) for your consideration @BigRoy

Multiple Roots present:

Screenshot 2024-04-05 101343

Empty GRP with no geometries at all:

Screenshot 2024-04-05 101428

When invalid objects present in the model instance:

Screenshot 2024-04-05 101639

And if some Shape not being visible (actually I have turned on Intermediate Shape in the ShapeNode attribs) ...sort of works for this occasion too even tho its not 100% accurate info for user whats causing it and also when triggering Repair it simply removes the object which is a bit destructive imho...but works somewhat

image

Would be even cooler if it could detect the Intermediate shape and switch it off...and notify this more accurately....however its a bit of edge case aka synthetic circumstance anyway

@LiborBatek
Copy link
Member

@BigRoy maybe for the last occasion with Empty Transform we could enhance the detailed message on the right with something like:

Empty transforms found without Shapes or with "Intermediate" shape enabled. Warning: by hitting "Repair" action these will be removed!

or similar. What u think?

@LiborBatek LiborBatek self-requested a review April 5, 2024 08:33
Copy link
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.

Would be nice to adress the occasion when Shape not visible and informing user that Repair will actually delete those invalid entities...

@BigRoy
Copy link
Collaborator Author

BigRoy commented Apr 5, 2024

@BigRoy maybe for the last occasion with Empty Transform we could enhance the detailed message on the right with something like:

Empty transforms found without Shapes or with "Intermediate" shape enabled. Warning: by hitting "Repair" action these will be removed!

or similar. What u think?

Thanks for the thorough testing. I'd like to argue that the current behavior is actually correct.

The model family exports without construction history and thus should ignore the intermediate objects. As such, to what would be published that is just an "empty mesh". In day to day production I've also never seen a modeler count a Transform with an intermediate object shape under it (which Maya doesn't show in outliner by default, etc.) as a "mesh" - they just see the empty transform and often don't even know that it was containing an intermediate object - which usually happens due to modeling tools and construction history they make. As such, the input node that had the intermediate mesh usually also counts as "history" and thus should be removed.

I'd say, if there's more debate about it we can do so in another PR? @LiborBatek

@BigRoy BigRoy requested a review from LiborBatek April 5, 2024 09:59
@LiborBatek
Copy link
Member

@BigRoy yeah, I get your point...no need to take such occasion into account

in terms of "improving validation message" I can see some benefits to include that "removal warning" as its not repairing but deleting stuff from the workfile altering it.

And it does not affect the current correct behaviour in any way just improving the message for a user.

Lets merge it anyway...

@tokejepsen tokejepsen merged commit f08257b into ynput:develop Apr 8, 2024
2 of 3 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 host: Maya size/XS type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants