Skip to content

[VFX] Mobile fixes #5149

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

Merged
merged 5 commits into from
Aug 31, 2021
Merged

[VFX] Mobile fixes #5149

merged 5 commits into from
Aug 31, 2021

Conversation

julienf-unity
Copy link
Contributor

@julienf-unity julienf-unity commented Jul 15, 2021

Purpose of this PR

Some mobile fixes for VFX Graph:

This is a first pass, and other fixes are required for mobile devices (in particular some C++ changes)

For 1348666, I've checked with @hkr, and decided to guard functions using cubemap arrays between SHADER_AVAILABLE_CUBEARRAY check. They may revert to old behavior possibly at some point.

For 1149057, The issue is caused by driver bug for certain integer instructions (in this case, it's with bitFieldExtract). I've worked around them by using float instruction instead.
I've tested all outputs: There's still a potential issue with cube output uv computation, but as it is an experimental feature that has other bugs, I don't have an immediate fix for that (and not sure if it's worth it anyway)


Testing status

Tested locally on Oculus Quest 2 with GLES3.1 (Vulkan does not exhibit the issue)


Comments to reviewers

Notes for the reviewers you have assigned.

@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

VFX
/.yamato%252Fall-vfx.yml%2523PR_VFX_2021.2

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@github-actions
Copy link

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page).
See the PR template for more information.
Thank you!

Copy link
Contributor

@gabrieldelacruz gabrieldelacruz 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!

@VitaVFX VitaVFX requested review from VitaVFX and removed request for a team July 19, 2021 07:41
@VitaVFX
Copy link

VitaVFX commented Aug 12, 2021

Hey @julienf-unity! Here's the WIP Test Doc. Main issue of extended particles on Android is fixed, it haven't occured on any of the devices during first QA pass. Whew!

Speaking further, some of the issues are already summarized (included minor and major issues) with provided reproduction and can be worked on. Some of them still need some attention thus dropped some questions in the doc. (Figured out answers (I think)).

Also, if there's anything I can do to make the investigation on your side easier, let me know.

@julienf-unity
Copy link
Contributor Author

Hey @julienf-unity! Here's the WIP Test Doc. Main issue of extended particles on Android is fixed, it haven't occured on any of the devices during first QA pass. Whew!

Speaking further, some of the issues are already summarized (included minor and major issues) with provided reproduction and can be worked on. Some of them still need some attention thus dropped some questions in the doc. (Figured out answers (I think)).

Also, if there's anything I can do to make the investigation on your side easier, let me know.

Yes this is what this PR is fixing. Of course there's still lots of other issues but it shouldnt block the PR. If the "extended particle" fix is verified, I would like to merge it before branching off.

Copy link

@VitaVFX VitaVFX left a comment

Choose a reason for hiding this comment

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

Approving as the main issue is fixed, other issues will be looked into in another fix pass.

@julienf-unity
Copy link
Contributor Author

#5582 must be backported at the same time as this one

julienf-unity added a commit that referenced this pull request Sep 9, 2021
* [VFX] Mobile fixes (#5149)

* Fix compil error when cubemap array is used in compute (even on platform supporting it) (#5582)
julienf-unity added a commit that referenced this pull request Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants