Skip to content
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

Add vertex light support #382

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

daleeidd
Copy link
Collaborator

@daleeidd daleeidd commented Dec 27, 2019

This will help address #357
There is also pixel light support with #383

Experimental support for vertex lights. Vertex lights are point lights which are set to Not Important or in some cases Automatic.

Night
Night Time

About

Attenuation

The attenuation formula was taken from here. The UNITY_LIGHT_ATTENUATION doesn't work for vertex lights. The Shade4PointLights, as discussed in the thread, didn't give good results.

Shader Parameters

I've added some shader parameters so others for flexibility for the time being. Not sure if they will be merged. They are mostly there to separate the parameters for the primary direction light.

Notes

ShadeVertexLightsFull cannot be used since it is for Legacy Vertex Lit.

Issues

Tiling Artefacts

Tiling

Tiling artefacts occur depending on the attenuation, lights overlapping and camera orientation. The frame debugger shows lighting values being filled. It could be the attenuation calculation or a limitation in vertex lighting. Further investigation required.

Underwater

Underwater

I have come across this issue when trying to integrate a weather asset with cloud shadows. It is much worse at night. Not including lights might be the best solution. Or at least having it behind a toggle.

Missing

  • Directional Light style reflections

@daleeidd daleeidd changed the title Add vertex lights support. Experimental Add vertex lights support Dec 27, 2019
@daleeidd daleeidd changed the title Add vertex lights support Add vertex light support Dec 27, 2019
@huwb
Copy link
Contributor

huwb commented Dec 27, 2019

Really solid work. Code looks all good to me.

I tried it out and seems to work well from initial experiments.

Could you push the tiling fail case? You could check in main.unity temporarily and we could revert it later, if that makes sense.

@daleeidd
Copy link
Collaborator Author

Thanks. Done

@huwb
Copy link
Contributor

huwb commented Dec 27, 2019

It may be because the light is set to Auto and unity is deciding to make it a pixel light. If i set the red light to be "Not Important" the issue goes away.. ?

@daleeidd
Copy link
Collaborator Author

I thought that too. I am in the same test case now and I have to set them (or at least the red one) to Important for the issue to go away.

ABROKE
ANOTBROKE

@daleeidd
Copy link
Collaborator Author

I just went through the frame debugger and VERTEXLIGHT_ON was active for those tiles.

Metal is a bit strange with lighting. I will have to test on Windows tomorrow. Hopefully, I can get the same as yours.

@daleeidd
Copy link
Collaborator Author

I get the same for Windows as for Mac. The issue is that the red colour is severely diminished:

Lights marked as Important
A Important

Lights marked as Not Important
A Not Important

Also, if I change one of the green lights to a blue colour, the red colour is passed through correctly.

This could be a bug in Unity.

@huwb
Copy link
Contributor

huwb commented Dec 28, 2019

Thanks for the above, I'm trying to wrap my head around things.

I think that Unity will move lights between vertex and pixel depending on number of lights, importance, maybe intensity(?). And there is a limit of 4 lights per vertex (and the directional light seems to impact the result for me - when i turn it off the result changes - so it's counted in the 4?).

If all that is true the story that my brain has constructed is that it is moving lights between vertex and pixel based on these factors, but this branch only has vertex, not pixel, so lights disappear. But I'm not 100% sure if that is consistent with what we're seeing / with the above?

@huwb
Copy link
Contributor

huwb commented Dec 28, 2019

Worth noting as well that since verts can be displaced quite far horizontally, the render bounds are quite a lot larger than the tile appears and it may be overlapping more lights than it would seem

image

@daleeidd
Copy link
Collaborator Author

daleeidd commented Dec 28, 2019

Thanks. Displacement tip is useful. After disabling attenuation:

Displacement Lighting

I have gone over the following again:
https://docs.unity3d.com/Manual/RenderTech-ForwardRendering.html

but this branch only has vertex, not pixel, so lights disappear

It is hard to say. The only thing I think I can do to confirm is to merge the pixel and vertex branch together, and max out the lights to see what happens. Not too difficult.

But I'm not 100% sure if that is consistent with what we're seeing / with the above?

From what I have been experiencing, it hasn't been consistent with the manual. I didn't think Important lights would even render without pixel light support in the shader. I thought that Unity was populating the shader data based on pixel light logic since it knows we don't support extra passes (Not sure if that make sense. My ignorance might be showing). I have also seen the issue with lower amounts of lights. Lastly, when comparing Important and Not Important in the Frame Debugger, the light positions, world light and spherical harmonics are the same; the only thing changed is the light colours. I would think if the light disappears, then the light positions shouldn't include the same set of lights (although, it could be the case where a light is both a vertex light and SH leading me astray):

Important
111 Good

Not Important
111 Bad

I might have to make some reproducible cases with planes and a trimmed down shader. I'll let this sit in my mind for a bit.

@daleeidd
Copy link
Collaborator Author

daleeidd commented Dec 29, 2019

After testing in isolation, I will make a forum post or file a bug since the issue is unrelated to Crest.

@daleeidd
Copy link
Collaborator Author

daleeidd commented Feb 5, 2020

Finally got around to it: https://forum.unity.com/threads/vertex-lights-behaving-strangely.822300/

@daleeidd
Copy link
Collaborator Author

Update from Unity (12/03/2020):

We successfully reproduced this issue and have sent it for resolution with our developers.

@daleeidd daleeidd marked this pull request as ready for review April 22, 2020 12:07
@daleeidd
Copy link
Collaborator Author

Removing draft status. It could probably use more work which I can look into another time.

Unity has given a conclusion:

This particular case has been investigated thoroughly and we have decided, to not address this fix for the time being. Our developers found out that using stock URP shader works as expected, so you could use that as a workaround.

We could implement it here and then recommend URP if anyone wants nice lighting.

Multiplying gave bad results for ambient light in scattering. Works
better with bubbles than addition.
  # Conflicts:
  #	crest/Assets/Crest/Crest/Shaders/Ocean.shader
  #	crest/Assets/Crest/Crest/Shaders/OceanEmission.hlsl
  #	crest/Assets/Crest/Crest/Shaders/OceanFoam.hlsl
  #	crest/Assets/Crest/Crest/Shaders/Underwater/UnderwaterCurtain.shader
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.

None yet

3 participants