Skip to content

Conversation

@lee-aandrew
Copy link
Contributor

@lee-aandrew lee-aandrew commented Jan 4, 2023

Purpose of this PR

Ticket/Jira #:
USDU-276

Testing

Adds testcase for Force Refreshing a USD prefab imported from a USDZ file, with Import Preview Surface option set for "Materials"
Is currently ignored as it will cause a failure due to USDU-276

Overall Product Risks

Complexity:
1

Halo Effect:
1

@lee-aandrew lee-aandrew requested a review from vickycl January 4, 2023 16:46
@lee-aandrew lee-aandrew force-pushed the add-test-UsdAssetTests branch from f33d997 to 95bc711 Compare January 6, 2023 14:54
@lee-aandrew lee-aandrew requested a review from IlliaKo January 9, 2023 19:49
Copy link
Contributor

@vickycl vickycl left a comment

Choose a reason for hiding this comment

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

I'm not entirely clear what the goal of this test is. I think this test (and the bug) are more about the ImportPreviewSurface than the reload behaviour, is that correct? If so, I think this test should be moved to a material specific file, and/or make that more clear in the test. In its current state it's not clear to me what additional value this test adds.

@lee-aandrew
Copy link
Contributor Author

I'm not entirely clear what the goal of this test is. I think this test (and the bug) are more about the ImportPreviewSurface than the reload behaviour, is that correct? If so, I think this test should be moved to a material specific file, and/or make that more clear in the test. In its current state it's not clear to me what additional value this test adds.

Hey Vicky, the goal of this test is to make sure that error handling is done correctly when Refresh errors out with the testing circumstances stated during the test - and as far as i know the steps taken in the test case is the only way a user may refresh a usdz imported prefab with Import Preview Surface without having to bring it into the Unity Scene

The same error does not happen when you directly import it as a prefab with the Scene Import Options with materials set to ImportPreviewSurface (have to temporarily change setting in ImportHelpers.ImportAsPrefab) so it seems like a problem with the Refresh feature more than specifically the ImportPreviewSurface setting

Though I do agree I havent been too descriptive in the naming of the test case, I changed it to
"UsdAsset_Force_Reload_Prefab_ImportPreviewSurface_ErrorShouldCleanScene"
But let me know if you still dont get the value of the test case in first glance - in which case i will need to do something more to it

@lee-aandrew lee-aandrew requested a review from vickycl January 17, 2023 07:19
Copy link
Contributor

@vickycl vickycl left a comment

Choose a reason for hiding this comment

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

DM'd on Slack, I think this test should probably be two based on reproducing the original bug :)

@lee-aandrew lee-aandrew force-pushed the add-test-UsdAssetTests branch from 4676690 to ee0ec86 Compare January 27, 2023 21:26
@lee-aandrew lee-aandrew requested a review from vickycl January 27, 2023 21:31
Copy link
Contributor

@vickycl vickycl left a comment

Choose a reason for hiding this comment

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

These new tests look much clearer :)

@lee-aandrew lee-aandrew merged commit 330e4e6 into dev Jan 30, 2023
@lee-aandrew
Copy link
Contributor Author

Thanks for the reviews @vickycl !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants