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

Validate clashing sibling names when strip namespaces is enabled #112

Conversation

BigRoy
Copy link
Contributor

@BigRoy BigRoy commented Sep 16, 2024

Changelog Description

When exporting siblings that with stripped namespaces share the same name, validate it directly instead of erroring on extraction.

Additional info

image

Avoids this error on ExtractUSD:

Traceback (most recent call last):
  File "C:\Users\User\AppData\Local\Ynput\AYON\dependency_packages\ayon_2406251801_windows.zip\dependencies\pyblish\plugin.py", line 528, in __explicit_process
    runner(*args)
  File "E:\dev\ayon-maya\client\ayon_maya\plugins\publish\extract_maya_usd.py", line 357, in process
    cmds.mayaUSDExport(file=file_path,
  File "E:\dev\ayon-maya\client\ayon_maya\plugins\publish\extract_maya_usd.py", line 2, in mayaUSDExport
RuntimeError: Multiple dag nodes map to the same prim path after stripping namespaces: |group1|NewNamespace1:pCube1 - |group1|pCube1

Avoids this error on ExtractAlembic:

Traceback (most recent call last):
  File "C:\Users\User\AppData\Local\Ynput\AYON\dependency_packages\ayon_2406251801_windows.zip\dependencies\pyblish\plugin.py", line 528, in __explicit_process
    runner(*args)
  File "E:\dev\ayon-maya\client\ayon_maya\plugins\publish\extract_pointcache.py", line 227, in process
    extract_alembic(**kwargs)
  File "E:\dev\ayon-maya\client\ayon_maya\api\alembic.py", line 341, in extract_alembic
    cmds.AbcExport(
  File "E:\dev\ayon-maya\client\ayon_maya\plugins\publish\extract_pointcache.py", line 2, in AbcExport
RuntimeError: Alembic Exception encountered: OSchemaObject::OSchemaObject( OObject )
ERROR: EXCEPTION:
Already have an Object named: pCube1

Testing notes:

Create an hierarchy like:

image

  1. Export with stripped namespaces enabled.
  2. This should be invalidated because without namespaces, both the pCube1 would share same full path and clash on export.

@BigRoy BigRoy added the type: enhancement Improvement of existing functionality or minor addition label Sep 16, 2024
@BigRoy BigRoy self-assigned this Sep 16, 2024
f"{report_list}",
description=self.get_description())

def is_strip_namespaces_enabled(self, instance) -> bool:
Copy link
Contributor Author

@BigRoy BigRoy Sep 16, 2024

Choose a reason for hiding this comment

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

@iLLiCiTiT this really seems like madness - but I'm admittedly not entirely sure how to do this better. Many of these plugins can be enabled/disabled, they can be optional and hence disabled for the instance only, they can either have exposed publish attributes for stripNamespaces or have it locked down and defined by settings as defaults.

Only way to simplify that is maybe moving this to a singular 'toggle' to a dedicated Collector which puts it in instance.data? But it'd not be backwards compatible.

Copy link
Member

@iLLiCiTiT iLLiCiTiT Sep 17, 2024

Choose a reason for hiding this comment

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

That actually sounds more legit to me. Single plugin that is capable of showing all the input fields and based on the checked fields adds families (or different data) to instance. But that is a lot of work, so I don't know if should be handled in this PR.

OptionalPyblishPluginMixin was not really meant that it is dependent on other plugins. BTW shouldn't the other plugin add something to instance data (for now) instead of having this logic dependent on current state of instance (might not represent state of instance at the moment the other plugin was processed)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OptionalPyblishPluginMixin was not really meant that it is dependent on other plugins. BTW shouldn't the other plugin add something to instance data (for now) instead of having this logic dependent on current state of instance (might not represent state of instance at the moment the other plugin was processed)?

The other plugins are Extractors - they run after the validators and hence the data is not there yet, so I'd need to move those to a Collector instead - which means we get into the shitshow of backwards compatibilities, etc. :)

This logic here is the best I could think of to support it now without refactoring everything.

Copy link
Member

Choose a reason for hiding this comment

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

I do agree "correct implementation" would require massive refactor to allow it. I'll let @antirotor to decide.

@moonyuet
Copy link
Member

Have done some simple test with animation product type in my side.
image

But maybe there is something wrong about the detection of the plugin(it could be my misunderstanding)
DEBUG: Plugin ExtractAnimation is disabled for this instance.
I actually enable collect fbx animation and ExtractAnimation should be enabled and run after this.
image

@BigRoy
Copy link
Contributor Author

BigRoy commented Sep 17, 2024

Have done some simple test with animation product type in my side. image

But maybe there is something wrong about the detection of the plugin(it could be my misunderstanding) DEBUG: Plugin ExtractAnimation is disabled for this instance. I actually enable collect fbx animation and ExtractAnimation should be enabled and run after this. image

@moonyuet Hard to tell from your screenshots whether the ExtractAnimation (which is what generates the Alembic; not the FBX) is enabled. Also, the FBX exporter isn't problematic - because looking at the code it only strips a "shared namespace" and not a namespace per object so I'm not sure if that runs into an issue.

So please validate with the Alembic and USD exports - disregard the FBX ones.

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.

Works well, been tested on pointcache product with multiple scenarios (strip namespaces globally on/off, exposed strip namespaces as override etc)

Screenshot 2024-09-20 131941

note: there could possibly be super rare edge case with model outputted also into Alembic as now the validator doesnt kick in (even when having Strip namespaces ON) but there are other Validators which takes care about Namespaces within the model content and Suffix Naming Conventions forbidding names with ":" inside...so this would happen only when those 2 validators disabled...leading into Alembic with non unique naming when stripping on.

@moonyuet moonyuet merged commit de32270 into ynput:develop Sep 24, 2024
1 check passed
@BigRoy BigRoy deleted the enhancement/maya_validate_siblings_unique_strip_namespace branch September 24, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants