Skip to content

Conversation

sophiaaar
Copy link
Contributor

Purpose of this PR

Backport of #32

Testing status

Manual Tests

Ran the project to ensure no errors occurred, but can't test fully on my Mac

Links

Yamato:
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics/tree/7.x.x%252Fbackport%252Ftesting%252Fgtf-removal

Comments to reviewers

Also disable tests 011, 017, 018, 019, 020, 021, 022 - fogbugz case will be made so these issues can be followed up on

@DennisDeRykeUnity DennisDeRykeUnity marked this pull request as ready for review May 15, 2020 07:46
@DennisDeRykeUnity
Copy link
Contributor

DennisDeRykeUnity commented May 15, 2020

The short answer is, "I approve!"

See below for my notes about each of the tests that @sophiaaar disabled. I added a couple commits to 7.x.x/backport/testing/gtf-removal to re-enable a few of the these. The images for 018 and 019 changed very little, and XR devs are investigating similar image discrepancies for PR22 (shim removal); I believe it is best to update these two reference images for now.

Test 020 needs work but that is my problem, not yours. I'm leaving it disabled for now. Tinkering with 020 in the Editor locally does not make me suspect any regressions in the code... I just need to fix my test.

Here is my Yamato job for my most recent commit on 7.x.x/backport/testing/gtf-removal (b2fc70e):
https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics/tree/7.x.x%252Fbackport%252Ftesting%252Fgtf-removal/.yamato%252Fupm-ci-universal_stereo.yml%2523All_Universal_Stereo_fast-2019.3/2201269/job/pipeline

This job has ran for three hours and I need to sleep now. If it finishes green during Friday business hours in Europe, then I encourage you to merge commit b2fc70e. If this job ends with any failed tests, then I'm fine with you merging sophiaar's last commit 8653cc1. I approve, in either case.


017_xr_Particles_Additive
Scene folder was missing from project.

018_xr_Particles_Multiply
Scene folder was missing from project.
Assets\CommonAssets\Scripts\Particles was also missing.
Assets\CommonAssets\Textures was also missing, specifically Exp_01.tif
Updated reference image. Particles are slightly scaled, or closer to camera.

019_xr_Particles_AlphaBlend
Assets\CommonAssets\Materials\Explosion.mat was mising.
Updated reference image. Particles are slightly scaled, or closer to camera.

020_xr_Lighting_ReflectionProbe
Material was missing from scene folder.
Sphere Smooth Probe > Mesh Renderer > Lighting > Contribute Global Illumination > Enabled
Rebaked lighting.
Still not working right. Disabling. I'll fix it later.

021_xr_Realtime_ReflectionProbe
Material was missing from scene folder.

022_xr_Static_Batching
Scene folder was missing from project.


Copy link
Contributor

@DennisDeRykeUnity DennisDeRykeUnity left a comment

Choose a reason for hiding this comment

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

Approving -- see my previous comment regarding the changesets I added and Yamato jobs in progress.

@sophiaaar sophiaaar marked this pull request as ready for review May 15, 2020 07:55
@DennisDeRykeUnity
Copy link
Contributor

@sophiaaar I fixed tests 018 and 019 in commit 8cfd398. When you merge this PR, would you be willing to merge commit 8cfd398 which re-enables most of the tests that you originally disabled, and is green on Yamato here:

https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics/tree/7.x.x%252Fbackport%252Ftesting%252Fgtf-removal/.yamato%252Fupm-ci-universal_stereo.yml%2523All_Universal_Stereo_fast-2019.3/2212769/job/pipeline

@sophiaaar
Copy link
Contributor Author

This PR strayed too far from the target branch and became un-mergeable :( new PR at #531

@sophiaaar sophiaaar closed this May 18, 2020
@sophiaaar sophiaaar deleted the 7.x.x/backport/testing/gtf-removal branch May 19, 2020 11:28
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.

2 participants