Skip to content

Conversation

@lee-aandrew
Copy link
Contributor

@lee-aandrew lee-aandrew commented Dec 22, 2022

Purpose of this PR

Ticket/Jira #:
[USDU-230][USDU-232][USDU-275]

Testing

Functional Testing status:

Adds Testcases:

  • ImportAsPrefab_TextureDataImported: Tests if Import as Prefab functionality keeps texture data set in USDA file
  • ImportAsGameObjects_TextureDataImported: Tests if Import as GameObject functionality keeps texture data set in USDA file
  • ImportAsGameObject_TextureDataImported_FromUsdz: Tests if Import as GameObject from USDZ file keeps texture data set in original USDA file

Overall Product Risks

ImportHelper.cs:

  • Added another ImportAsPrefab that takes in a SceneImportOptions
  • GetSelectedAssetPath: private -> public (needed access from a test script)
  • GetDefaultRoot: private -> public (needed access from a test script)

Complexity:
1

Halo Effect:
1

Additional information

Note to reviewers:

Reminder:
Requires: #327 to be pushed in first

@lee-aandrew lee-aandrew changed the title Add testcases regarding texture mapping [USDU-230][USDU-232][USDU-275] Add testcases regarding texture mapping Dec 22, 2022
@lee-aandrew lee-aandrew requested review from judubu and vickycl January 3, 2023 15:53
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.

Can we reduce the sizes or remove some of the textures please? A few 100kb can add up quite quickly over the amount of times they're downloaded, and they could be really tiny and still function the same in these tests.

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.

Very nice! Just a few small requests:

  • Textures could still be smaller :P a single texture of a single colour shouldn't need to be more than 64x64pix, maybe even less.

  • I don't see a test in here for USDU-275 (which I don't think has been fixed, so would fail anyway), am I missing something?

@lee-aandrew lee-aandrew force-pushed the add-testcase-texture-mapping branch from 570ffe7 to 82509d9 Compare January 17, 2023 06:17
@lee-aandrew lee-aandrew requested a review from vickycl January 17, 2023 06:40
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.

Looks good! Thank you for the iterations :)

@lee-aandrew lee-aandrew merged commit 6bd5bd1 into dev Jan 18, 2023
@lee-aandrew
Copy link
Contributor Author

Thank you for the reviews @vickycl and @unity-chris !

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.

4 participants