Skip to content

Conversation

PaulDemeulenaere
Copy link
Contributor

@PaulDemeulenaere PaulDemeulenaere commented Jun 1, 2021

Purpose of this PR

TLDR, it fixes two fogbugz :

More details :

  • Fix soft particles orthographic projection : it was initially fixed by @ludovic-theobald in this PR.
  • Fix missing Flip-Y : implicitly integrated in GetNormalizedScreenSpaceUV. The issue occurs when there isn't an HDR or Opaque texture as intermediate texture (⚠️This intermediate texture can be implicitly enabled by renderer feature like decal)
  • Fix wrong behavior on OGL : We were loading instead of sampling, when the depth texture isn't available, the default texture can be either black or white depending on reverse_z
  • Fix missing scale integration : See e73718c I'm not sure if the issue was already there, I didn't test it before, maybe relative to the change from Load to Sample

Testing status

1) Orthographic Depth
Before
_ortho_vs_perspective

After : Fix Orthographic Depth Sampling
_ortho_fix_after

Also an equivalent test from Ludovic :
eaf1df00-9b79-11eb-88a1-b0ee4bf399f0

2) Missing Y-Flip
Before
_before_fix_y_flip

⚠️ Decal has the same effect than enabling hdr/opaque texture
_flip_with_decal

After
_fix_soft_particle_depth

3) Fallback when there isn't a depth texture
Before DX11 (no issue actually)
_soft_particle_dx11

Before OGL (on the left Shuriken, on the right VFX)
_shuriken_vs_vfx

After
_fix_no_depth_ogl

4) Screen scale
Before
_screen_scale_fail

After
_screen_scale_fix


Comments to reviewers

This change will require a manual backport in 10.x.x & 7.x.x
See also this PR : https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/211

ludovic-theobald and others added 5 commits May 31, 2021 16:54
# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md
# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md
Correctly applying the flip-y when appropriate while sampling
It's simpler than reimplement the space transform and equivalent
…VFXCommon.hlsl


Fix comment

Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>
Remove VFXLinearEyeDepth & VFXLinearEyeDepthOrthographic from the SRP specific implementation
Synchronize implementation for VFXLinearEyeDepth in VFXCommonCompute.hlsl (not used anyway)
TODO : Move LinearDepthToEyeDepth in core for 21.2
Copy link
Contributor

@ludovic-theobald ludovic-theobald 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 to me!

@PaulDemeulenaere PaulDemeulenaere marked this pull request as ready for review June 4, 2021 17:16
@PaulDemeulenaere PaulDemeulenaere requested review from a team as code owners June 4, 2021 17:16
@VitaVFX
Copy link

VitaVFX commented Jun 8, 2021

Thank you for shiny tests and detailed description, @PaulDemeulenaere!

Tested:

  • All camera settings (ortho + perspective)
  • URP asset settings
  • Forward decal
  • Output settings/inspector
  • Standalone
  • Graphics APIs (DX11, Vulkan, Metal)

Everything you fixed works like a charm, but observed several issues:

  1. Soft particles do not work with camera stacking
    Screenshot_1
  2. Decal particles have 'sliding' problem when tweaking 'Render Scale' property in URP asset
    A1wqlfeilZ

Repro can be found here

Is this something that could be fixed in this PR?

@ludovic-theobald
Copy link
Contributor

ludovic-theobald commented Jun 8, 2021

@VitaSkruibyte For the second issue, it's possible that it is fixed in the VFX HDRP Decals PR (at the same time as the sliding artifact was fixed).
I'll test and come back to you.

Edit : It's not fixed in the other PR.

@PaulDemeulenaere
Copy link
Contributor Author

Edit : It's not fixed in the other PR.

I have an idea about this issue, forward decal use a simple sampling which require to consider the renderscale while sampling the depth. I will take a look, if it's a simple fix, we could append it to this PR ⏳

@PaulDemeulenaere
Copy link
Contributor Author

PaulDemeulenaere commented Jun 15, 2021

Update for @VitaSkruibyte

  1. About Soft particles do not work with camera stacking
    image
    Can you confirm it doesn't work with shuriken as well ? If it's the case, it's probably a common issue within URP and should be reported in URP grab bag.

  2. About Decal particles have 'sliding', the issue is actually here
    It should be
    clipPos.xy = (i.pos.xy * rcp(GetScaledScreenParams().xy)) * 2.0f - 1.0f;
    Instead of
    clipPos.xy = (i.pos.xy / _ScreenParams.xy) * 2.0f - 1.0f; (cc @ludovic-theobald)
    Can you open a fogbugz (and assign it to me) for this one ? I will create an independent PR for this issue, it will probably require an new interface in VFX (GetScaledScreenParams is specific to URP).
    I'm also wondering if there is a specific issue on SubpixelAA & ScreenSpaceSize with RenderScale & URP.

@PaulDemeulenaere
Copy link
Contributor Author

I isolated the fix about forward decal here : 5a4cb98 (not included ⚠️)

_scale_screen_space_size.mp4

In the end, should I integrate this change in this PR @julienf-unity ?

@VitaVFX
Copy link

VitaVFX commented Jun 16, 2021

Heya @PaulDemeulenaere! Thank you for the update!

Here's the bug for URP team regarding soft particles
Here's the bug report regarding decals. Also, bumped into another issue where forward decal is not properly projected onto the surface with Overlay Camera, so regarding your comment could this be looked into in the independent PR you've mentioned? (of course, if you have bandwidth for this, otherwise I'll just open a new FB)

NSIixje5Dq

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.

All PR related issues were fixed, newly found bugs are not PR related and gonna be fixed in following PR, hence approving.

@PaulDemeulenaere
Copy link
Contributor Author

VFX_HDRP on Win_DX11_editmode_mono_Linear is expected to fail 🔴

@PaulDemeulenaere PaulDemeulenaere merged commit b84eb01 into master Jul 5, 2021
@PaulDemeulenaere PaulDemeulenaere deleted the vfx/fix/soft-particles-ortho-and-flip branch July 5, 2021 12:05
@PaulDemeulenaere
Copy link
Contributor Author

TODO : backport on 21.1, 10.x.x & 7.x.x

PaulDemeulenaere added a commit that referenced this pull request Aug 27, 2021
* Update changelog

# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md

* Different depth computation for orthographic camera

# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md

* Restore change from https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/211/commits/8169e083d9b68114bd7a248ebba1dfba38fca3f5

But using URP specific helper "LinearDepthToEyeDepth"

* FIx issue 1330697

Correctly applying the flip-y when appropriate while sampling

* Fix scaling using dedicated URP helper

It's simpler than reimplement the space transform and equivalent

* Update com.unity.render-pipelines.universal/Runtime/VFXGraph/Shaders/VFXCommon.hlsl

Fix comment

Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>

* Fix issue #4733 (comment)

Remove VFXLinearEyeDepth & VFXLinearEyeDepthOrthographic from the SRP specific implementation
Synchronize implementation for VFXLinearEyeDepth in VFXCommonCompute.hlsl (not used anyway)
TODO : Move LinearDepthToEyeDepth in core for 21.2

Co-authored-by: Ludovic Theobald <ludovic.theobald@unity3d.com>
Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>
# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/VFXGraph/Shaders/VFXCommon.hlsl
#	com.unity.visualeffectgraph/CHANGELOG.md
PaulDemeulenaere added a commit that referenced this pull request Aug 27, 2021
commit 73b6768
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Fri Aug 27 09:55:58 2021 +0200

    Fix changelog.md

commit 129edd3
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Fri Aug 27 09:50:59 2021 +0200

    Fix after merge

    Apply VFXSampleDepth for URP on the correct file

commit a29197d
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Jul 5 14:05:45 2021 +0200

    [VFX] Fix soft particles (#4733)

    * Update changelog

    # Conflicts:
    #	com.unity.visualeffectgraph/CHANGELOG.md

    * Different depth computation for orthographic camera

    # Conflicts:
    #	com.unity.visualeffectgraph/CHANGELOG.md

    * Restore change from https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/211/commits/8169e083d9b68114bd7a248ebba1dfba38fca3f5

    But using URP specific helper "LinearDepthToEyeDepth"

    * FIx issue 1330697

    Correctly applying the flip-y when appropriate while sampling

    * Fix scaling using dedicated URP helper

    It's simpler than reimplement the space transform and equivalent

    * Update com.unity.render-pipelines.universal/Runtime/VFXGraph/Shaders/VFXCommon.hlsl

    Fix comment

    Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>

    * Fix issue #4733 (comment)

    Remove VFXLinearEyeDepth & VFXLinearEyeDepthOrthographic from the SRP specific implementation
    Synchronize implementation for VFXLinearEyeDepth in VFXCommonCompute.hlsl (not used anyway)
    TODO : Move LinearDepthToEyeDepth in core for 21.2

    Co-authored-by: Ludovic Theobald <ludovic.theobald@unity3d.com>
    Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>
    # Conflicts:
    #	com.unity.render-pipelines.universal/CHANGELOG.md
    #	com.unity.render-pipelines.universal/Runtime/VFXGraph/Shaders/VFXCommon.hlsl
    #	com.unity.visualeffectgraph/CHANGELOG.md

(cherry picked from commit b38a9eb72b46d39f1f1cb052a85fffb1f7142380)

# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.visualeffectgraph/CHANGELOG.md
phi-lira added a commit that referenced this pull request Sep 6, 2021
* Squashed commit of the following:

commit 73b6768
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Fri Aug 27 09:55:58 2021 +0200

    Fix changelog.md

commit 129edd3
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Fri Aug 27 09:50:59 2021 +0200

    Fix after merge

    Apply VFXSampleDepth for URP on the correct file

commit a29197d
Author: Paul Demeulenaere <pauld@unity3d.com>
Date:   Mon Jul 5 14:05:45 2021 +0200

    [VFX] Fix soft particles (#4733)

    * Update changelog

    # Conflicts:
    #	com.unity.visualeffectgraph/CHANGELOG.md

    * Different depth computation for orthographic camera

    # Conflicts:
    #	com.unity.visualeffectgraph/CHANGELOG.md

    * Restore change from https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/211/commits/8169e083d9b68114bd7a248ebba1dfba38fca3f5

    But using URP specific helper "LinearDepthToEyeDepth"

    * FIx issue 1330697

    Correctly applying the flip-y when appropriate while sampling

    * Fix scaling using dedicated URP helper

    It's simpler than reimplement the space transform and equivalent

    * Update com.unity.render-pipelines.universal/Runtime/VFXGraph/Shaders/VFXCommon.hlsl

    Fix comment

    Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>

    * Fix issue #4733 (comment)

    Remove VFXLinearEyeDepth & VFXLinearEyeDepthOrthographic from the SRP specific implementation
    Synchronize implementation for VFXLinearEyeDepth in VFXCommonCompute.hlsl (not used anyway)
    TODO : Move LinearDepthToEyeDepth in core for 21.2

    Co-authored-by: Ludovic Theobald <ludovic.theobald@unity3d.com>
    Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>
    # Conflicts:
    #	com.unity.render-pipelines.universal/CHANGELOG.md
    #	com.unity.render-pipelines.universal/Runtime/VFXGraph/Shaders/VFXCommon.hlsl
    #	com.unity.visualeffectgraph/CHANGELOG.md

(cherry picked from commit b38a9eb72b46d39f1f1cb052a85fffb1f7142380)

# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.visualeffectgraph/CHANGELOG.md

* Fix changelog.md

Co-authored-by: Felipe Lira <felipedrl@gmail.com>
phi-lira pushed a commit that referenced this pull request Sep 6, 2021
* [VFX] Fix soft particles (#4733)

* Update changelog

# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md

* Different depth computation for orthographic camera

# Conflicts:
#	com.unity.visualeffectgraph/CHANGELOG.md

* Restore change from https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/211/commits/8169e083d9b68114bd7a248ebba1dfba38fca3f5

But using URP specific helper "LinearDepthToEyeDepth"

* FIx issue 1330697

Correctly applying the flip-y when appropriate while sampling

* Fix scaling using dedicated URP helper

It's simpler than reimplement the space transform and equivalent

* Update com.unity.render-pipelines.universal/Runtime/VFXGraph/Shaders/VFXCommon.hlsl

Fix comment

Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>

* Fix issue #4733 (comment)

Remove VFXLinearEyeDepth & VFXLinearEyeDepthOrthographic from the SRP specific implementation
Synchronize implementation for VFXLinearEyeDepth in VFXCommonCompute.hlsl (not used anyway)
TODO : Move LinearDepthToEyeDepth in core for 21.2

Co-authored-by: Ludovic Theobald <ludovic.theobald@unity3d.com>
Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>
# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/VFXGraph/Shaders/VFXCommon.hlsl
#	com.unity.visualeffectgraph/CHANGELOG.md

* Fix after merge

Apply VFXSampleDepth for URP on the correct file

* Fix changelog.md
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.

5 participants