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

EMSUSD-972 refactor duplication code #3573

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

pierrebai-adsk
Copy link
Collaborator

The goal is to make the duplication more testable.

Some scenarios would require the Arnold plugin to be present and we want to avoid depending on it. So instead, the code refactor split the duplication into finer-grained functions that can be tested by using custom-made temporary layers to reproduce the behavior of Arnold.

The refactor also makes the code clearer and more correct by having smaller functions. Multiple subtle problems were discovered and fixed with this strategy, related to complex cases of renaming and inter-prim relationships.

  • Move code to copy temporary prims to a layer out of PrimUpdaterManager.
  • This makes the code testable with pre-fabricated temp layers to test specific scenarios.
  • This also lighten a bit the prim updater manager source code, which is very large.
  • (More code should be refactored out of it in the future to make it simpler and more testable.)
  • Fix copyLayerPrims to report all copied prims, even b=nested ones.
  • This is important for display layers and renaming.
  • Add Python wrapper for copyLayerPrims to allow testing.
  • Add explanation to the Python wrapper why stage and layer are not passed by pointers.
  • Write multiple unit tests of increasing complexities.
  • Add unit tests for optionally copying relationships targets.
  • Improve the renaming handling to avoid renaming chains.
  • Improve renaming handling to avoid modifying the prim hierarchy while we are traversing it, otherwise the traversal becomes unpredictable and elements could be processed twice.
  • Clearly explain in comments the pitfalls of rename chains.
  • Add relationship targets verification to the unit tests.

@pierrebai-adsk pierrebai-adsk force-pushed the bailp/EMSUSD-972/refactor-dup-to-usd branch from 499f9c1 to 044fbfd Compare January 24, 2024 18:06
@pierrebai-adsk pierrebai-adsk added adsk Related to Autodesk plugin unit test Related to unit tests (both python or c++) labels Jan 24, 2024
@pierrebai-adsk pierrebai-adsk self-assigned this Jan 24, 2024
The goal is to make the duplication more testable.

Some scenarios would require the Arnold plugin to be present and we want to avoid depending on it.
So instead, the code refactor split the duplication into finer-grained functions that can be tested
by using custom-made temporary layers to reproduce the behavior of Arnold.

The refactor also makes the code clearer and more correct by having smaller functions.
Multiple subtles problems were discovered and fixed with this strategy, related to complex
cases of renaming and inter-prim relationships.

- Move code to copy temporary prims to a layer out of PrimUpdaterManager.
- This makes the code testable with pre-fabricated temp layers to test specific scenarios.
- This also lighten a bit the prim updater manager source code, which is very large.
- (More code should be refactored out of it in the future to make it simpler and more testable.)
- Fix copyLayerPrims to report all copied prims, even b=nested ones.
- This is important for display layers and renaming.
- Add Python wrapper for copyLayerPrims to allow testing.
- Add explanation to the Python wrapper why stage and layer are not passed by pointers.
- Write multiple unit tests of increasing complexities.
- Add unit tests for optionally copying relationships targets.
- Improve the renaming handling to avoid renaming chains.
- Improve renaming handling to avoid modifying the prim hierarchy while we are traversing it,
  otherwise the traversal becomes unpredictable and elements could be processed twice.
- Clearly explain in comments the pitfalls of rename chains.
- Add relationship targets verifications to the unit tests.
@pierrebai-adsk pierrebai-adsk force-pushed the bailp/EMSUSD-972/refactor-dup-to-usd branch from 044fbfd to 97b3951 Compare January 24, 2024 19:52
@pierrebai-adsk
Copy link
Collaborator Author

I pre-emptively fixed many typos in comments and a wrong copyright year,

@pierrebai-adsk pierrebai-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jan 25, 2024
@seando-adsk seando-adsk added core Related to core library and removed adsk Related to Autodesk plugin labels Jan 25, 2024
@seando-adsk seando-adsk merged commit 469da23 into dev Jan 25, 2024
11 checks passed
@seando-adsk seando-adsk deleted the bailp/EMSUSD-972/refactor-dup-to-usd branch January 25, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to core library ready-for-merge Development process is finished, PR is ready for merge unit test Related to unit tests (both python or c++)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants