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

changed toon shader to step the accumulated light instead of each light separately #36271

Closed
wants to merge 1 commit into from

Conversation

QbieShay
Copy link
Contributor

@QbieShay QbieShay commented Feb 16, 2020

In this proposal the issue is highlighted. This is a different solution than the one proposed, but breaks compatibility with existing toon shaded games.

Here you can see on the left current Godot's toon shader, and on the right the look with this implementation
Screenshot_2020-02-16_16-10-59

@realkotob
Copy link
Contributor

Great fix! If this also applies to GLES2 then it would be nice to merge to master as well.

@QbieShay
Copy link
Contributor Author

@asheraryam unfortunately it's not possible to implement this fix for GLES2, which is why it won't be merged. There is an open source toon shader linked in the issue, I recommend to use that one, which looks really pretty even with the banding issue.

@akien-mga akien-mga force-pushed the 3.2 branch 2 times, most recently from 8eb193d to 69c1805 Compare March 6, 2020 23:01
@Calinou Calinou added this to the 3.2 milestone Jan 5, 2021
@QbieShay QbieShay requested a review from a team as a code owner March 12, 2021 12:26
Base automatically changed from 3.2 to 3.x March 16, 2021 11:11
@aaronfranke aaronfranke modified the milestones: 3.2, 3.3 Mar 16, 2021
@akien-mga akien-mga modified the milestones: 3.3, 3.4 Mar 26, 2021
Comment on lines +2144 to +2149
#ifdef DIFFUSE_TOON

diffuse_light *= step(0.1, max(diffuse_light.r, max(diffuse_light.g, diffuse_light.b)));


# endif // DIFFUSE_TOON
Copy link
Member

Choose a reason for hiding this comment

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

Style issues (indentation should be tabs, and two empty lines of separation is too much):

Suggested change
#ifdef DIFFUSE_TOON
diffuse_light *= step(0.1, max(diffuse_light.r, max(diffuse_light.g, diffuse_light.b)));
# endif // DIFFUSE_TOON
#ifdef DIFFUSE_TOON
diffuse_light *= step(0.1, max(diffuse_light.r, max(diffuse_light.g, diffuse_light.b)));
# endif // DIFFUSE_TOON

@lyuma
Copy link
Contributor

lyuma commented Jun 4, 2021

The godot-vrm project contains a shader which illustrates some techniques that can be used to recreate toon shaded output using existing Godot functionality.

https://github.com/V-Sekai/godot-vrm/blob/godot3/addons/Godot-MToon-Shader/mtoon.shader

Though the above shader simply adds each light and does not need post-light, one technique that can be used to emulate this proposal would be to accumulate the lighting into a "fragment-to-light varying" variable, and assign the post-lighting function directly to DIFFUSE_LIGHT each time light is called.

See here for an example of a "post_light" type function implemented using current 3.4 branch: https://gist.github.com/lyuma/a1a2093637f8216e5c53ec9e4081cd31
example using fragment-to-light varying
but nothing fundamentally changes the fact that this technique requires doing all lights in one render pass.

[Note that the shader I linked above depends on overriding NORMAL to get a single irradiance sample, but this disrupts metallic / PBR type materials (SPECULAR_LIGHT != 0) if they are to be mixed with toon shading, such as the MiHoYo style. These PBR toon shaders with specular light would require GLSL or engine modificaitons]

@QbieShay
Copy link
Contributor Author

There are some toon shaders here: https://godotshaders.com/?s=toon

Someone smarter than me figured out how to do it :D

For the time being, I'm closing this issue.
@lyuma I'll leave toon shading in your hands for 4.0, super hyped to see all your work 🎉

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.

6 participants