AmplifyCreations / AmplifyShaderEditor-Feedback

4 stars 0 forks source link

SRP Premultiply shader Bug #301

Closed AironT closed 6 months ago

AironT commented 6 months ago

Found a problem with Premultiply shaders after updating project to new version of Unity and new version of ASE. When I decided to recompile my old shaders I've notice pretty visible changes, their visuals changed.

I've check this on Unity/URP versions: 2022.3.5/14.0.8 and 2022.3.20/14.0.10 Also checked with versions of ASE: v1.9.1.5 and v1.9.3.2

So when I start digging I've found where things going wrong... In Forward pass there is define check with name "_ALPHAPREMULTIPLY_ON". If comment this, then shader works as should.

devenv_YXx1TbeT5V

After this I've cheked what logic was in legacy shaders and found this...

devenv_m7W5aWm3gS

I think it's better to change the way it works by default to the same way as it was in old legacy shaders. Or add some sort of control to this feature in the graph.


Additional info how I tested it.

I've created 3 shaders with the same logic inside: Test_shaders.zip

  1. ASE LegacyShader http://paste.amplify.pt/view/raw/61942a65
  2. ASE URP Unlit (http://paste.amplify.pt/view/raw/b205e7a3)
  3. ShaderGraph URP Unlit.

Inside you can see 2 parameters which called 'Emission' and 'Darkening'. Basically First myltiply on RGB channels and Second multiply on Alpha channel. image image

In materials just set Darkening=0 and you will find that ASE URP Unlit works in different way then other 2 shaders, it become full transparent.

xnviewmp_Bukdk2TuR0

In erlier version it works the same, now it's not.

AironT commented 6 months ago

This also works fine and seems like more correct way. image

diogovtx commented 6 months ago

You are correct. The multiplication is already being done via Blend Mode (One, OneMinusSrcAlpha), so it's effectively accumulating two multiplications by alpha.

This has now been corrected and will be included in the v1.9.3.4 release.

Thank you for taking the time to write this very detail report. Highly appreciated.

Dawie3565 commented 6 months ago

second user found issue also in default UI template bumped priority to highest critical