godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.99k stars 21.16k forks source link

Vulkan: Per-vertex shading not working #43093

Closed AlzoPalzo closed 1 month ago

AlzoPalzo commented 4 years ago

Godot version: v4.0.dev.calinou.e16729a8c

OS/device including version: Windows 10, GTX 1070, Vulkan

Issue description: In Godot 3.2 I used Tree models with their normals edited in blender in tandem with per vertex shading to create smoothly shaded trees. In Godot 4.0, with vulkan backend switching the shading between per vertex and per pixel has no effect on the tree models

Examples of the expected result are shown below where the trees on the left are shaded per pixel and the trees on the right are shaded per vertex. treeShadingExample2

treeShadingExample

Steps to reproduce: Add a directional Light or other lights to the models and in the material turn on the per vertex shading flag(3.2) or switch shading to per vertex(4.0)

Minimal reproduction project:

LowPolyTrees.zip

r0-che commented 2 years ago

vertex shading still does not work in 4.0

3 godot 3.5, vertex shading on the left 4 godot 4.0 beta, vertex shading on the left aswell

AnalogFeelings commented 1 year ago

It seems it's also happening in the gl_compatibility and mobile renderers. I tried using the shaders from godot-psx on all 3 renderers and none of them have vertex lighting, only pixel lighting.

(Windows 11, RTX 2060, Godot 4.0 Beta 12)

zCubed3 commented 1 year ago

I've noticed 2 things regarding vertex lighting in 4.X, I hope it can be of help to someone more talented with graphics programming. (I'll note these are just my observations, I know nothing about how Godot's rendering pipeline works)

  1. Looking at 3.X, I can see that scene.glsl contains the USE_VERTEX_LIGHTING flag. While in the master branch, scene.glsl (and its equivalents) are completely missing the flag.
  2. It also appears that from the C++ side of things that USE_VERTEX_LIGHTING is never set anymore. The flag doesn't exist anymore in material.h either?
clayjohn commented 1 year ago

I've noticed 2 things regarding vertex lighting in 4.X, I hope it can be of help to someone more talented with graphics programming. (I'll note these are just my observations, I know nothing about how Godot's rendering pipeline works)

  1. Looking at 3.X, I can see that scene.glsl contains the USE_VERTEX_LIGHTING flag. While in the master branch, scene.glsl (and its equivalents) are completely missing the flag.
  2. It also appears that from the C++ side of things that USE_VERTEX_LIGHTING is never set anymore. The flag doesn't exist anymore in material.h either?

Vertex lighting hasn't been re implemented yet in 4.x

zCubed3 commented 1 year ago

👍 Thanks, good to know.

pozitiffcat commented 1 year ago

When it will be implemented? 20 fps on mobile devices with only one direct light is no good ((

Calinou commented 1 year ago

When it will be implemented? 20 fps on mobile devices with only one direct light is no good ((

This won't be done for 4.0 due to time constraints.

If performance is an issue, consider decreasing the 3D rendering resolution using the Scaling 3D > Scale advanced project setting (0.5 is a good value for mobile). This will often benefit performance more than switching to vertex shading, which comes with a lot of limitations.

TaraSophieDev commented 1 year ago

I hope it will be fixed soonish after the 4.0 release. It's to be honest a very important feature to have, for many of us.

GithubPrankster commented 1 year ago

Could this issue be looked into now that 4.0 is out and 4.1 is nearing? It reduces the feasibility of retro 3D work in the current iteration of the engine. I personally would be happier with a proper solution to #39513 but I'll take anything at this point.

Calinou commented 1 year ago

Could this issue be looked into now that 4.0 is out and 4.1 is nearing?

There's no ETA for implementing this feature, and nobody has looked into it yet to my knowledge. Feature freeze for 4.1 is approaching, so this won't be merged before 4.2 at the earliest.

MicrotonalMatt commented 1 year ago

Any update on ETA? I just tried with 4.2-dev4 and still not working. This and #4619 are deal breakers for moving my project from 3 to 4 unfortunately.

Calinou commented 1 year ago

Any update on ETA?

Same as before, as rendering contributors are busy with other tasks currently. https://github.com/godotengine/godot/pull/66030 is also stalled because the PR author no longer has time to work on it.

golddotasksquestions commented 1 year ago

I just want to state that I think this needs to be given much higher priority.

One of Godots most prominent usecases is 3D retro games. PS1 style games. 3D games that rely heavily on classic vertex lighting. The internet is filled with free and paid tutorials and complete courses which teach how to create Godot games in that style. Many of Godots most famous games are made in this style. People come to Godot knowing it's not a AAA engine, but they expect to be able to do 3D retro games with per-vertex lighting.

This is not a replacement for per vertex lighting, but if you have to use Godot 4.X over Godot 3.X, you could try to use the Standard Material Diffuse mode Lambert Wrap or Toon. It may be enough for some usecases. If you try this, I recommend to also play with the DirectionalLight3D Directional Shadow settings to adjust the shadow gradients.

AThousandShips commented 1 year ago

Anyone willing and able to fix this is welcome to, we can't order people to just fix one specific thing

golddotasksquestions commented 1 year ago

we can't order people to just fix one specific thing

a) Everyone knows people who contribute for free on their own time out of their own ambition can't be ordered or forced to work on anything specific. You can't tell me what to do with my free time, and neither can I. Insinuating anyone would assume otherwise is honestly pretty offending.

b) Also everyone knows the Godot core devs and maintainers are able promote specific issues, bring attention to them and ask for help or if anyone would like to take this on, and offer support. This has been done countless times before (for example on various social media channels of core devs and maintainers). Often very successfully.

c) Also everyone knows the Godot Foundation is able to hire people to work on on specific issues. This has also been done before countless times. Also everyone knows Godot Foundation has received a massive uptick in donations recently due to the Unity debacle.

AThousandShips commented 1 year ago

I'm sorry if pointing that out is offensive, I meant no offense, but I see no other way of "prioritising" it if no one is willing or able to work on it

I don't see this as not being promoted or prioritised, seeing the attention it has and is getting

Have a nice day

golddotasksquestions commented 1 year ago

I don't see this as not being promoted or prioritised, seeing the attention it has and is getting

I don't see any of the core devs with a large following promoting this issue on their socials, or the foundation taking some of the money in their hands and offer to hire someone to fix it.

Such a promotion could be shared on the official Godot twitter with ~100K followers, and by Remi and Juan who has a large following with technically skilled devs. There could also be a separate tab on https://godot.foundation/ next to "Donations" saying "For hire", where prioritized Github issues are linked. They could also post and promote on the Godot subreddit with 142K subscribers and the Godot Discord server. In such posts links to the contributor chat rendering channel could be included and members of the rendering team mentioned to be addressed in the contributor chat for support/review of the PR.

So I think there is still a lot of room to officially prioritize & promote this issue.

ywmaa commented 1 year ago

@golddotasksquestions well I need this feature too, so I am going to try to do it, though I am new to rendering.

ywmaa commented 1 year ago

I looked at the material.cpp and material.h, I saw vertex shading parameters, so I guess I need to only do the rendering backend, right ?

AnalogFeelings commented 1 year ago

I looked at the material.cpp and material.h, I saw vertex shading parameters, so I guess I need to only do the rendering backend, right ?

I am pretty sure the entire framework for it is already done, but the shaders lack the code required to handle vertex lighting.

ywmaa commented 1 year ago

I looked at the material.cpp and material.h, I saw vertex shading parameters, so I guess I need to only do the rendering backend, right ?

I am pretty sure the entire framework for it is already done, but the shaders lack the code required to handle vertex lighting.

Yeah that's true, atleast for GLES3

I looked at the scene.glsl and it looks like it only needs to add the Vertex Shading pass and we are good to go (Working on it).

Does vulkan use this shader too ?

Calinou commented 1 year ago

Does vulkan use this shader too ?

Forward+ uses scene_forward_clustered.glsl, Mobile uses scene_forward_mobile.glsl. Both include scene_forward_lights_inc.glsl.

ywmaa commented 1 year ago

My draft PR https://github.com/godotengine/godot/pull/83360

Calinou commented 5 months ago

I made MRPs to test shading mode performance in 3.x and 4.x:

This is a situation where per-vertex shading gives you nearly identical visuals, but can greatly improve performance in situations where you have lit particles close to the camera. In games, there are two common situations where this occurs:

Many people use vertex shading for stylistic reasons, but personally I want it for performance reasons first and foremost. This is doubly true on mobile and integrated graphics – I only tested a top-end dedicated GPU here and the difference is already pretty noticeable. You can imagine it only gets that much more important on low-end GPUs :slightly_smiling_face:

Results with the MRPs on a RTX 4090 in 4K for reference:

3.5.2 GLES3

Imgsli

Per-pixel Per-vertex Unshaded
1449 FPS (0.69 mspf) 1842 FPS (0.54 mspf) 1936 FPS (0.52 mspf)

4.3.dev5 Forward+

Imgsli

Per-pixel Per-vertex Unshaded
856 FPS (1.17 mspf) N/A 1875 FPS (0.53 mspf)

4.3.dev5 Compatibility

Imgsli

Per-pixel Per-vertex Unshaded
2105 FPS (0.48 mspf) N/A 3259 FPS (0.31 mspf)

If done well, a 4.x implementation of per-vertex shading should offer performance between per-pixel shading and unshaded (hopefully closer to unshaded than per-pixel), like in 3.x.

That said, if we choose to sample shadow maps on a per-pixel basis on per-vertex materials, this will incur a performance cost so it won't be as fast as in 3.x. Doing so will greatly improve vertex shading usability, as in 3.x, you can't see shadow maps on vertex-shaded materials. There is a separate "do not receive shadows" flag that already exists in BaseMaterial3D, so you could enable that if you really need to skip shadow map sampling.

akien-mga commented 1 month ago

Fixed by #83360.