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

Implement vertex shading #83360

Merged
merged 1 commit into from
Sep 28, 2024
Merged

Implement vertex shading #83360

merged 1 commit into from
Sep 28, 2024

Conversation

ywmaa
Copy link
Contributor

@ywmaa ywmaa commented Oct 15, 2023

Implements Vertex Shading
resolves #43093

  • OpenGL
  • Vulkan Mobile
  • Vulkan Forward+

Bugs :

  • Vertex Shading receives shadows from one light source in GLES3 No Shadows will be casted in Vertex Lighting
  • When using Forward+, vertex lighting doesn't render unless you get very close to the light node.
  • When using lightmaps, DirectionalLights with the Static bake mode still affect lightmapped surfaces.
  • This PR completely removes shadows in the compatibility renderer regardless of whether vertex shading is enabled
  • The "force vertex shading" project setting should prompt users to restart the engine
  • The "force vertex shading" project setting appears to only work for StandardMaterials (it should work regardless of the material type and should apply to all materials in the scene)
  • Use simplified light model for vertex shading like 3.x
  • Directional Light does nothing if shadows is enabled both in Vertex Lighting or Pixel Lighting (GLES3 only Issue)
  • Shadows for vertex shading in Vulkan/Forward Renderers.
  • In Forward+, the vertex lighting will pop in on the edges of the viewport. (Disable Light clustering with vertex lighting)
  • When you enable shadows in Compatibility, the light is drawn twice: one with per-pixel shading, one with per-vertex shading
  • when adding a varying in gdshader, it errors out

Note: This PR implements Vertex-Lighting without shadows.
Note: This PR implements Vertex-Lighting with Pixel-Shadows.
Production edit: Pixel shadows are only available for the first DirectionalLight3D in the scene. Omni and spot lights cannot cast shadows on vertex-shaded materials (like in Godot 3.x), except in the Compatibility rendering method where lights with shadows are rendered with a multipass approach.

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 15, 2023

For some reason, only the material preview shows the vertex shading, in the viewport it looks like it doesn't use vertex shading.
Any Idea why ?

Per-Pixel
Screenshot from 2023-10-15 03-49-19
Per-Vertex
Screenshot from 2023-10-15 03-49-24

@AnalogFeelings
Copy link

AnalogFeelings commented Oct 15, 2023

For some reason, only the material preview shows the vertex shading, in the viewport it looks like it doesn't use vertex shading.
Any Idea why ?

Per-Pixel
Screenshot from 2023-10-15 03-49-19
Per-Vertex
Screenshot from 2023-10-15 03-49-24

That does not look like vertex shading at the bottom material preview

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 15, 2023

are you sure this is not vertex-lighting ?
image

this is normal pixel lighting :
image

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 15, 2023

mmm I guess I did vertex shadow not vertex lighting, fixing it.

Edit : lol, it is actually the opposite, shadow calculation should be added in vertex.

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 15, 2023

I guess I need an advice from the Rendering Team, currently I am almost duplicating functions for the vertex-shading, should I just use Preprocessor to move every thing to vertex shading, or should I keep duplicating the stuff, because we may remove features from the vertex shading ?

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 15, 2023

Progress Vertex lighting (Lighting + Shadows) in GLES3/Compatibilty renderer:
image

currently known bug : Vertex Shading receives shadows from one light source in GLES3, and if you enable shadow casting on the second light, it won't cast light at all to the mesh.

Bug images :

Second light no shadows :
Screenshot from 2023-10-15 19-49-41

Second light with shadows :
Screenshot from 2023-10-15 19-49-36

Edit :

Well I just tried Godot 3.5, and vertex_lighting doesn't cast shadows at all
image

was it intended ?

@Calinou
Copy link
Member

Calinou commented Oct 15, 2023

Regarding why the second light with shadows disappear, remember that Godot 4's GLES3 performs multipass lighting if you use multiple lights with shadows. This is different from Godot 3 GLES3 (and more comparable to Godot 3 GLES2, although it used multipass for non-shadowing lights too).

Well I just tried Godot 3.5, and vertex_lighting doesn't cast shadows at all

Yes, real-time shadows can't be received by vertex-lit surfaces. Only baked lightmap shadows can be seen on them.

In GLES3 (unlike GLES2), it should be possible to sample the shadowmap texture in vertex() if you want real-time shadows to be visible on vertex-lit surfaces, but it's not trivial. The shadow detail will also be limited by the receiving object's mesh complexity. (If this behavior needs to be disabled, the Do Not Receive Shadows material flag can be used.)

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 15, 2023

In GLES3 (unlike GLES2), it should be possible to sample the shadowmap texture in vertex() if you want real-time shadows to be visible on vertex-lit surfaces, but it's not trivial. The shadow detail will also be limited by the receiving object's mesh complexity.

so do you think I should remove shadows from vertex-lighting ?

@Calinou
Copy link
Member

Calinou commented Oct 15, 2023

so do you think I should remove shadows from vertex-lighting ?

I think you can remove them for now. Support for receiving shadows in vertex-lit materials can be implemented in a future PR (if it's feasible at all) 🙂

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 19, 2023

Now Compatibilty/GLES3/OpenGL Rendering should be ready for Vertex-Lighting testing and regression testing.

I will continue work on Vulkan Mobile + Vulkan Forward+

@ywmaa ywmaa marked this pull request as ready for review October 19, 2023 16:10
@ywmaa ywmaa requested review from a team as code owners October 19, 2023 16:10
@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 19, 2023

SOOOOO Vertex Shading now works in Vulkan

Unfortunately I had to do Mobile + Forward in one commit, as both depend on scene_forward_lights_inc.glsl

Now TIME for review,
but remember :
image

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 19, 2023

My comment on shadows for vertex lighting :

I think it is possible to cast shadows using vertex_shading.

But I think vertex shading is mainly useful for performance reasons, and probably realtime shadows are costly.

So it doesn't make a lot of sense to have realtime shadows in vertex lighting.

I may implement shadows in another PR if there is a demand for it.

Personally I don't need shadows in my game for vertex lighting.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it mostly works as expected. This is a great start 🙂

Testing projects:

There are some rendering issues:

  • When using Forward+, vertex lighting doesn't render unless you get very close to the light node (the objects far away use per-pixel shading for comparison):
simplescreenrecorder-2023-10-19_20.03.01_forward_plus.mp4

This may be related to the light clustering system.

  • When using Mobile, vertex lighting occasionally flickers in and out on specific objects when moving the camera (this may be hard to reproduce):
simplescreenrecorder-2023-10-19_20.04.02_mobile.mp4

In the testing project, the plane is subdivided so it can receive vertex lighting correctly.

Another issue is that when using lightmaps, DirectionalLights with the Static bake mode still affect lightmapped surfaces:

Current appearance Expected appearance (light is hidden)
Screenshot_20231019_203146 Screenshot_20231019_203142

This isn't an issue with omni or spot lights. You can hide the directional light after baking as a workaround, but then it won't affect dynamic objects.

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 20, 2023

@Calinou
these two commits should fix your issues (except flicker in Vulkan Mobile, or maybe it is fixed too ?)
I tested locally, but I need a confirm.

@AnalogFeelings
Copy link

AnalogFeelings commented Oct 20, 2023

Recording.2023-10-20.162439.mp4

It seems some polygons become unlit if they become partially out of view?

EDIT: I am on Forward+ by the way.

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 20, 2023

@AnalogFeelings Yes this happens to me too, I think this is light clustering in Forward+ and not a bug.

Maybe calinou knows.

@AnalogFeelings
Copy link

I also noticed setting the viewport to lighting only causes HEAVY artifacts.

SPOILER_20231020-1433-32.5621820.mp4

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 20, 2023

I also noticed setting the viewport to lighting only causes HEAVY artifacts.

Interesting, I'll check it out.

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 23, 2023

@AnalogFeelings

I am unable to reproduce your issue :
image

But I noticed that in lighting mode it shows vertex lighting like pixel lighting, so that will probably need a fix.

@Calinou
Copy link
Member

Calinou commented Oct 23, 2023

But I noticed that in lighting mode it shows vertex lighting like pixel lighting, so that will probably need a fix.

Lighting mode replaces all materials in the scene with a basic ShaderMaterial that uses per-pixel lighting (it might just be the fallback material). It can't know whether the original material used vertex shading or not. The Lighting debug draw mode's appearance will probably have to be toggled with a project setting.

@ywmaa
Copy link
Contributor Author

ywmaa commented Oct 23, 2023

Just fixed some bugs that I noticed from my work.

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Sep 20, 2024
@clayjohn
Copy link
Member

This PR has been rebased, squashed, and fixed up! It should be ready to merge anytime now.

I have tested it with the following:

  1. Single lights (each light type)
  2. Multiple lights (same light type)
  3. Multiple lights (all light types)
  4. Multiple of each light type
  5. The above with various combinations of shadows on/off
  6. High res geometry
  7. Low res geometry
  8. Force_vertex_shading on/off
  9. vertex shading flag on/off (BaseMaterial)
  10. vertex shading render_mode on/off (shader)

I believe that covers all the permutations in the shaders. At any rate, I think it is broad enough coverage that I feel confident this is ready to merge

Performance

Using Calinou's test project and an intel i7-1165G7 with integrated graphics.

To be clear, the goal of this PR is not necessarily performance. Ideally performance would improve, but these days the benefit of vertex shader is mostly for people who want to recreate styles from older games

Forward+

Per-pixel Per-vertex Unshaded
175 FPS (5.71 mspf) 266 FPS (3.76 mspf) 379 FPS (2.64 mspf)

Mobile

Per-pixel Per-vertex Unshaded
391 FPS (2.56 mspf) 591 FPS (1.69 mspf) 780 FPS (1.28 mspf)

Compatibility

Per-pixel Per-vertex Unshaded
481 FPS (2.08 mspf) 958 FPS (1.04 mspf) 1139 FPS (0.88 mspf)

@ywmaa
Copy link
Contributor Author

ywmaa commented Sep 21, 2024

I have tested the new changes, everything works great except that now no shadows for vertex lighting in Forward+/Mobile ?

@clayjohn
Copy link
Member

I have tested the new changes, everything works great except that now no shadows for vertex lighting in Forward+/Mobile ?

For positional lights ya. This is the same limitation we had in Godot 3 GLES3. There isn't a way to accurately calculate shadows for all light types when doing single pass rendering. We discussed this on the rendering chat and agreed with sticking to the same limitation as Godot 3, which is that the first directional light in the scene tree can cast shadows. All other lights won't have shadows.

For the Compatibility renderer all lights can cast shadows since it does multipass lighting.

@TaraSophieDev
Copy link

Just tested it out, it works great. Thanks a lot for the hard work.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected in all rendering methods.

I've also tested vertex shading combined with SDFGI, VoxelGI and ReflectionProbe and it looks correct. Note that the appearance of GI techniques is not affected by vertex shading, but real-time lights are still affected as expected.

@Calinou
Copy link
Member

Calinou commented Sep 23, 2024

I noticed an oddity when Force Vertex Shading is enabled on surfaces that receive VoxelGI or SDFGI though:

Per-vertex shading

Screenshot_20240923_161453

Per-vertex shading on a per-pixel material with Force Vertex Shading enabled

Notice how per-pixel shading still appears to be partially used. The vertex-lit specular lobe is also missing. I restarted the editor after enabling the setting.

Screenshot_20240923_161500

Per-pixel shading

Screenshot_20240923_161514

No visual difference is seen on surfaces that don't receive VoxelGI or SDFGI.

For context, I'm working on a follow-up PR that modifies the BaseMaterial3D.shading_mode property hint to indicate that vertex shading is forced in the project settings (and also that Lambert is forced over Burley if relevant).

@clayjohn
Copy link
Member

@Calinou Can you share that test project? I'm not sure what would cause that other than, perhaps VoxelGI is overwriting the color somehow

@Calinou
Copy link
Member

Calinou commented Sep 23, 2024

@Calinou Can you share that test project?

The test project is test_pr_83360.zip (same as the one I used previously) with SDFGI enabled, or VoxelGI added and baked at its default position and extents. I reduced the material's roughness value to a value around 0.5 and increased metallic to 1.0.

@akien-mga
Copy link
Member

For release maintainers: This is fine to merge as is (once conflicts are resolved), the oddity noticed by Calinou is not critical and can be looked at in a follow-up issue/PR.

@ywmaa
Copy link
Contributor Author

ywmaa commented Sep 27, 2024

So rebased to reslove the conflicts, @clayjohn the issue was with this section https://github.com/godotengine/godot/pull/83360/files#diff-94158bc3d510819bb8d7ebd62a8890eeebad49608d9e934d148b73b6b65aeff9L1422

which you removed anyway previously so I just accepted the incoming result "the vertex shading commit changes"

also for some reason your name does not show again in the commit even though I didn't change anything in the commit message ?! can this be solved ?

@clayjohn
Copy link
Member

So rebased to reslove the conflicts, @clayjohn the issue was with this section https://github.com/godotengine/godot/pull/83360/files#diff-94158bc3d510819bb8d7ebd62a8890eeebad49608d9e934d148b73b6b65aeff9L1422

which you removed anyway previously so I just accepted the incoming result "the vertex shading commit changes"

That makes sense. It looks fine now

also for some reason your name does not show again in the commit even though I didn't change anything in the commit message ?! can this be solved ?

I'm not sure why that is. I wouldn't worry about it, the attribution is in the commit message anyway

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Ready to merge :)

This adds support in all backends, but the Compatibility renderer works the best.
Mobile and Forward+ can only support one directional light shader (the first in the tree)
While the Compatibility renderer supports any number of shadows.

Co-authored-by: Clay John <[email protected]>
@akien-mga
Copy link
Member

also for some reason your name does not show again in the commit even though I didn't change anything in the commit message ?! can this be solved ?

I'm not sure why that is. I wouldn't worry about it, the attribution is in the commit message anyway

That was because the commit description was missing a separation line between the first paragraph and the "Co-authored-by:" line. GitHub is very picky about this, that line needs to be properly separated from the rest of the commit message.

I amended the description to fix it.

@akien-mga akien-mga merged commit 285ebed into godotengine:master Sep 28, 2024
18 checks passed
@akien-mga
Copy link
Member

akien-mga commented Sep 28, 2024

Thanks @ywmaa and @clayjohn for the outstanding work! and @Calinou of course for amazing testing and review throughout the process! This should make a number of users very happy 🎉

ahxgamma referenced this pull request in ahxgamma/redot-engine Oct 1, 2024
This reverts commit 285ebed, reversing
changes made to e7a9104.
@Axis4s
Copy link

Axis4s commented Oct 4, 2024

Would be nice if more flexibility were given to this over time,, for example Toon Shading with vertex-shading.
Yeah that's possible, the game the most people like to copy toon shading from (Wind Waker) actually uses vertex shading under the hood. Mostly because that the Gamecube/Wii used vertex-shading as their main method of lighting objects.
But some games faked phong/pixel-shading with some tricks, so some GC/Wii games had a per-pixel shading-like look.

Funfact Wii is essentially a repackaged Gamecube, the big differences are.. more RAM and better GPU for the Wii but anyway what i meant is the vertex shading mode should be able to handle more than just a basic diffuse shader

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Vulkan: Per-vertex shading not working