-
-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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 stencil support to spatial materials #80710
base: master
Are you sure you want to change the base?
Conversation
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.h
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/forward_mobile/scene_shader_forward_mobile.h
Outdated
Show resolved
Hide resolved
Woahh awesome @apples ! Thank you so much! I am busy busy until next week but I'll review it then! |
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
ff5c884), it mostly works as expected. MSAA, TAA and FXAA correctly smooth out stencil rendering as expected too.
In the demo project, the Wind Waker Light appears much darker when using the Mobile rendering method though. I assume this is due to the use of a RGB10A2 color buffer instead of RGBA16, but maybe this is due to the 2× color multiplier that has to be applied? It doesn't affect the x-ray outline on the second screenshot further below.
Forward+ | Mobile |
---|---|
Forward+ | Mobile |
---|---|
I also noticed the stencil modes don't feature syntax highlighting and autocompletion in the shader editor, unlike the render modes:
Shader syntax validation also doesn't work if the stencil type has anything written after write
, even if it makes no sense:
Here, I don't get a shader compilation error but I logically should.
I also suggest changing the default outline color in the material preset to be opaque black (or opaque white), so that you can see it immediately after setting it up. Right now, the default color is fully transparent.
PS: Using a Material Overlay property, it's possible to have an object that has both an outline and x-ray at the same time 🙂
Also, if you're curious about how a stencil outline compares with an inverted hull outline. Inverted hull on the left, stencil outline on the right:
Is it possible to create stencil shadows that don't appear through walls with this PR? Logically, it seems feasible, but I've tried to modify the Wind Waker light shader to no avail. |
@Calinou My intuition says that stencil shadows would require the write operations Right now, since the write operator isn't exposed, it's just set to
I noticed this too and haven't had a chance to debug it. It's something to do with how the raw color values are being mixed (multiply), it seems to be clamping the values to 1.0 at some point. The xray/outline effects in my demo just use standard alpha blending with regular colors, so the problem doesn't affect them. EDIT: If we want to expose those write operations for stencil shadows, we'll likely need to expose the compare/write masks as well. This can't really be done in the material properties for bloat reasons, but we could expose them in the shader |
0a1e4e9
to
be2f6cf
Compare
Tested with my project, I didn't attempt windwaker lights because I have completely forgot how the setup for that is supposed to work. I tested my "particles passing through hole in the ground" scene and also the uotline and xray mode. My own particles worked great! I am glad that this syntax seems indeed to be an update from the full stencil one: Outlines&Xray: works great! The default configuration however seems to use stencil reference 0 and not work, it started working as soon as i changed it to something that isn't 0 (maybe that has to do with the mask like you were saying @apples ?) |
RE: stencil shadows I have not seen a huge demand amongst users for this. Since it seems to add some complexity to the API, i'd leave it for a future PR in case there's need for it. I'd specifically wait to see if anyone needs those features also for other reasons. The internet seems to think stencil shadows are a bit outdated ad not "worth it" with current programmable pipelines? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on my test project for stencil, looks good!
I've noticed this PR also adds depth test operations, which I'm assuming they're needed for the stencil write if depth fail.
While exposing the whole thing just for this may be overkill, I see no harm in adding it for the users, I think they'll find ways to use it (plus i think the requested windwaker thing isn't possible without it).
Thank you Apples! My comments on default configurations can be tweaked in a later PR (and doesn't need to be you since changing defaults is something good for a first contributor as well ^^ )
This is true, but sometimes art direction prevails above all else 🙂 |
Oh. Didn't know they looked different! I really know nothing of them |
Wasn't the original proposal #3373 made explicitly because of stencil shadows? I can attest to that at the very least one of the 40 thumbs ups was for those. If they won't be supported the proposal should probably be reopened. 👀
I wouldn't put much weight to a 7 year old comment with 5 likes. As Calinou alluded to, stencil shadows have a distinct look and properties that are better fit for stylized games. I've commonly encountered them in platforming games, where the environment uses shadow maps and characters have down pointing stencil shadows. I could've swore some of the mario games worked like this but I can't recall which. |
@Lielay9 could you test this PR, and see if it supports them? I don't understand how they work, so I can't quite test them myself. You can go on the "checks" tab and at the right there is an "artifacts" menu drop down - you'll find a build for this PR there.
The proposal was indeed for stencil shadows specifically, but they cannot be done without stencil at all. It should be reassessed with this API. |
Nothing to do with masks, it's just that the buffer is initialized to all zeros every frame, so choosing 0 as a default reference value is kind of useless. It could easily be changed to 1 as the default.
Some stencil effects (Wind Waker lights in particular) will need the depth test operator exposed. Since #73527 seems like it's on track to be merged, I included it in this PR for testing purposes. I'm hoping that PR gets merged first so I can just rebase this one on top of it. |
Regarding stencil shadows:
Modern shadows are typically based on shadow maps, which can be a bit fuzzy/pixelated since they are stored as textures. Stencil shadows, in comparison, use geometry-based stencil volumes instead of textures, so they have infinitely crisp edges. Some users will desire this for artistic reasons, typically because they're trying to follow the style of retro games.
I don't think that'll be necessary. This PR is a good foundation, and will be very easy to expand upon in future PRs. Additionally, stencil shadows might require some other changes not directly related to stencils, such as exposing depth clamping, which is needed for infinite shadow volumes. Also, finding ways to generate the infinite shadow volumes in the first place and avoid culling them is outside the scope of this PR. Rest assured: I am one of those people who wants stencil shadows. It is not something forgotten with this PR. It just might take multiple PRs to fully support them. |
2023-08-24.01-26-39.mp4Well, I guess this is best you can do now. Works if volumes don't overlap; a simple torus, for example, will break it.
These are indeed necessary for shadows. Besides that, the only thing missing would be culling the shadows correctly which is outside the scope of this PR. Thereafter, stencil shadows are viable, if not exactly painless to use. Extra note: Some primitive shapes seem to have small holes/gaps and therefore didn't play too nicely with the code generating the shadow meshes. |
Could you post your code somewhere? I think the current primitive meshes are fine as they are, although some of them have hard seams as you'd typically expect (e.g. boxes aren't smooth-shaded). |
This comment was marked as off-topic.
This comment was marked as off-topic.
Hey @apples , sorry for the long wait for feedback on this PR. Upon further conversation it turned out that the sorting issue is non negligible at the moment because we will need to make changes in the renderer that will most likely invalidate the usage of render priority for opaque materials that use stencil, which means that merging this now will mean necessarily breaking compatibility later. That said, this PR is not wasted, we will integrate it later down the line once we figure out exactly how the opaque pass needs to work with godotengine/godot-proposals#7870 and godotengine/godot-proposals#7916 I owe you an apology on this because I misled you to think this work would be merged on 4.2. While it was unintentional and I was honestly convinced of the timeline, I made an evaluation mistake and told you things that turned out to not be feasible. My apologies for it. I'll be more mindful of this in the future both with you and other contributors. What we need to make happen is the compositor work so that we can ensure consistent sorting of stencil operations. I will write under this PR once that happens, and if you won't be available at that point in time I'll do my best to ensure that this work is ported by me or someone else. Thank you again for taking the time to implement this, we're one big step closer to stencil support in Godot and your work has been a crucial part of it. |
Just to add to Qbie's comment. The issue is not with this PR. The issue is that users can't count on What we need is fine-grained control over the order of rendering. We don't want to merge this until we have that, because we want to make sure that once this feature is in user's hands its fully functional and represents the best that we can do. Unfortunately that means delaying merging this work until we have the necessary other features to make this behave as expected. As always, we would rather delay a feature than merge it in a state that we are not 100% happy with. And again, I want to stress that this delay is not your fault, and it is totally regrettable. |
Question, would Stencil be usable to define a Volumes to have 2 different environments? Example use case: Water Volumes, where you've got an aggressive fog and a color correction gradient in adjustments, that doesn't apply outside of water |
This may not belong here, but wondering how the work here with stencils would change doing an xray effect like I have in the shader below. Is the change performance related or is the output entirely different?
|
@mrjustaguy Stencil effects can help when making underwater effects, but it's not always necessary. A simple underwater volume can be done by just using physics areas to detect when the camera goes underwater and adjusting the fog accordingly. More complicated underwater effects can be done using screen-reading shaders or other post-processing effects. @jbromberg For a simple x-ray effect like that, the output would probably be the same. Stencil effects have some advantages, such as being able to work with transparent objects, but those advantages might not matter for your situation. And yes, stencil effects would be more efficient. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style looks good otherwise
@@ -2492,6 +2594,171 @@ BaseMaterial3D::EmissionOperator BaseMaterial3D::get_emission_operator() const { | |||
return emission_op; | |||
} | |||
|
|||
void BaseMaterial3D::_prepare_stencil_effect() { | |||
const Ref<Material> current_next_pass = get_next_pass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const Ref<Material> current_next_pass = get_next_pass(); | |
Ref<Material> current_next_pass = get_next_pass(); |
While const
indicates intention here it doesn't really do anything except prevent reassignment of the variable itself, any operation is still allowed (and it is treated as a non-const pointer)
} | ||
|
||
Ref<BaseMaterial3D> BaseMaterial3D::_get_stencil_next_pass() const { | ||
const Ref<Material> current_next_pass = get_next_pass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const Ref<Material> current_next_pass = get_next_pass(); | |
Ref<Material> current_next_pass = get_next_pass(); |
depth_stencil_state.enable_depth_write = depth_draw != DEPTH_DRAW_DISABLED ? true : false; | ||
|
||
RD::CompareOperator depth_function_rd_table[DEPTH_FUNCTION_MAX] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RD::CompareOperator depth_function_rd_table[DEPTH_FUNCTION_MAX] = { | |
static const RD::CompareOperator depth_function_rd_table[DEPTH_FUNCTION_MAX] = { |
Here as well (even though it's not done in the other cases it's cleaner IMO)
|
||
depth_stencil_state.enable_stencil = stencil_enabled; | ||
if (stencil_enabled) { | ||
RD::CompareOperator stencil_compare_rd_table[STENCIL_COMPARE_MAX] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RD::CompareOperator stencil_compare_rd_table[STENCIL_COMPARE_MAX] = { | |
static const RD::CompareOperator stencil_compare_rd_table[STENCIL_COMPARE_MAX] = { |
depth_stencil_state.enable_depth_write = depth_draw != DEPTH_DRAW_DISABLED ? true : false; | ||
|
||
RD::CompareOperator depth_function_rd_table[DEPTH_FUNCTION_MAX] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RD::CompareOperator depth_function_rd_table[DEPTH_FUNCTION_MAX] = { | |
static const RD::CompareOperator depth_function_rd_table[DEPTH_FUNCTION_MAX] = { |
|
||
depth_stencil_state.enable_stencil = stencil_enabled; | ||
if (stencil_enabled) { | ||
RD::CompareOperator stencil_compare_rd_table[STENCIL_COMPARE_MAX] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RD::CompareOperator stencil_compare_rd_table[STENCIL_COMPARE_MAX] = { | |
static const RD::CompareOperator stencil_compare_rd_table[STENCIL_COMPARE_MAX] = { |
Really hoping this will be added in 4.3 Nevertheless stencil support truly is an awesome and much needed feature |
I share @Alia5 sentiment on this one, as-is it does allow for the most simple case to be delivered. It certainly lowers the bar for engagement people can stencil their third person character behind opaque and transparent geometry and draw them through it with various effects. Here is a custom dither (with a bit of alpha blending when not discarded for a softer look): I noticed that the xray colour drawn in the editor can go stale (as shown in the picture) changing the stencil mode flushes things and gets further colour changes applied immediately this might be some Godot UI quirk instead of an issue with the feature but just in case it is not I thought I would flag it. |
Would it be possible to at least rebase these changes on to master for people (me included lol) to try these changes out with the most recent master changes? I'd do it myself but I was struggling to resolve some of the conflicts properly. |
We usually don't ask contributors to rebase constantly unless we're close to merging their contribution, in en effort to respect people's time. However it's been a while so maybe there's merit in rebasing. |
Apologies, I don't mean to be disrespectful and do not mean to apply pressure on the authors. I wasn't sure who or where to contact after I was having trouble getting any help with doing this rebase in the Discord or the Rocketchat and it had been a while since there was an update so I figured there was no harm in asking here. |
No worries no harm done :) You can try joining the https://chat.godotengine.org/channel/rendering channel in the dev chat if you struggle with rebase, folks there can help |
Any updates? This has been not updated for so long, this branch is literally nearly 8000 commits behind master. |
f8040f5
to
6d98df0
Compare
I found some free time, so I decided to do the rebase since it seems like some people need it. It was a pretty clean rebase. I just had to flip the depth functions due to the recent reverse Z changes. I'm not sure how well this interacts with the new motion buffer, I haven't had a chance to test it yet. I also updated the demo project (https://github.com/apples/godot-stencil-demo). |
Updated stencil features to match new design Fixed stencil opaque pass Added STENCIL_RW_WRITE_ALWAYS Added docs for BaseMaterial3D stencil operations Added mobile renderer support Renamed stencil rw modes Implemented property hiding for stencil fields Updated docs Apply suggestions from code review Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com> Update scene/resources/material.cpp Co-authored-by: RedMser <5117197+RedMser@users.noreply.github.com> Update servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com> Added missing cases Fixed stencil_mode autocompletion Fixed syntax highlighting of stencil_mode Added stencil support to VisualShader Added stencil support for GLES3 Changed stencil property default values in BaseMaterial3D Revert "Added stencil support for GLES3" This reverts commit c6f581b. Removed mask_uses_ref project setting Changed depth functions to better match PR godotengine#73527 Apply suggestions from code review Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com> Changed set_stencil_effect_color to pass by const ref Fixed errors
6d98df0
to
1c1b7a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nit-picks and improvements for when you have the time
|
||
Ref<BaseMaterial3D> stencil_next_pass; | ||
|
||
if (!current_next_pass.is_valid() || !current_next_pass->has_meta("_stencil_owned")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!current_next_pass.is_valid() || !current_next_pass->has_meta("_stencil_owned")) { | |
if (current_next_pass.is_null() || !current_next_pass->has_meta("_stencil_owned")) { |
for (int j = 0; j < render_modes.size(); j++) { | ||
const ShaderLanguage::ModeInfo &mode_info = render_modes[j]; | ||
|
||
if (!mode_info.options.is_empty()) { | ||
for (int k = 0; k < mode_info.options.size(); k++) { | ||
built_ins.push_back(String(mode_info.name) + "_" + String(mode_info.options[k])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (int j = 0; j < render_modes.size(); j++) { | |
const ShaderLanguage::ModeInfo &mode_info = render_modes[j]; | |
if (!mode_info.options.is_empty()) { | |
for (int k = 0; k < mode_info.options.size(); k++) { | |
built_ins.push_back(String(mode_info.name) + "_" + String(mode_info.options[k])); | |
for (const ShaderLanguage::ModeInfo &mode_info : render_modes) { | |
if (!mode_info.options.is_empty()) { | |
for (const StringName &option : mode_info.options) { | |
built_ins.push_back(String(mode_info.name) + "_" + String(option)); |
Use for-each syntax when applicable
if (!mode_info.options.is_empty()) { | ||
for (int k = 0; k < mode_info.options.size(); k++) { | ||
built_ins.push_back(String(mode_info.name) + "_" + String(mode_info.options[k])); | ||
for (int j = 0; j < stencil_modes.size(); j++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
{ | ||
const Vector<ShaderLanguage::ModeInfo> &shader_modes = ShaderTypes::get_singleton()->get_modes(RenderingServer::ShaderMode(shader->get_mode())); | ||
|
||
for (int i = 0; i < shader_modes.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
|
||
for (int i = 0; i < modes.size(); i++) { | ||
const ShaderLanguage::ModeInfo &mode_info = modes[i]; | ||
for (int i = 0; i < stencil_modes.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
} | ||
|
||
// Add flags afterward. | ||
for (int i = 0; i < flag_names.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
for (int i = 0; i < RenderingServer::SHADER_MAX; i++) { | ||
const Vector<ModeInfo> modes = p_is_stencil ? ShaderTypes::get_singleton()->get_stencil_modes(RenderingServer::ShaderMode(i)) : ShaderTypes::get_singleton()->get_modes(RenderingServer::ShaderMode(i)); | ||
|
||
for (int j = 0; j < modes.size(); j++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
} | ||
} | ||
} else { | ||
for (int i = 0; i < p_modes.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
for (int i = 0; i < RenderingServer::SHADER_MAX; i++) { | ||
const Vector<ModeInfo> modes = ShaderTypes::get_singleton()->get_stencil_modes(RenderingServer::ShaderMode(i)); | ||
|
||
for (int j = 0; j < modes.size(); j++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
} | ||
} | ||
} else { | ||
for (int i = 0; i < p_info.stencil_modes.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Adds stencil buffer support to spatial materials.
Linked Issues
Supersedes #78542
Implements the proposed design from godotengine/godot-proposals#7174
Currently integrates #73527
Testing with the sample project https://github.com/apples/godot-stencil-demo
Differences from the design
ALWAYS
. TheALWAYS
operator is needed for some effects.next_pass
materials are handled is a bit strange right now, but I tried to make it as non-destructive as possible.Currently implemented
stencil_mode
to shaders, which works very similarly torender_mode
.read
- enables stencil comparisons.write
- enables stencil writes on depth pass.write_depth_fail
- enables stencil writes on depth fail.compare_(never|less|equal|less_or_equal|greater|not_equal|greater_or_equal|always)
- sets comparison operator.BaseMaterial3D
stencil properties.stencil_mode
- choose between disabled, custom, or presets.stencil_flags
- set read/write/write_depth_fail flags.stencil_compare
- set stencil comparison operator.stencil_reference
- set stencil reference value.stencil_effect_color
- used by outline and xray presets.stencil_outline_thickness
- used by outline preset.BaseMaterial3D
stencil presets.STENCIL_MODE_OUTLINE
- adds a next pass which uses thegrow
property to create an outline.STENCIL_MODE_XRAY
- adds a next pass which usesdepth_test_disabled
to draw a silhouette of the object behind other geometry.Supported Renderers
Depth Prepass
Stencil effects work well when rendered with a second opaque pass immediately following the current opaque pass. However, with depth prepass enabled, the depth information from any stencil materials is not available for screen-space effects. For instance, with SSAO enabled, opaque stencil materials have strong visual artifacts. Due to this, there might not be a good way to support opaque stencil effects when depth prepass is enabled.
GLES3
More work is needed for GLES3 support. The depth buffer attachment for the scene FBO must be changed to a depth-stencil attachment. Making this change without breaking existing projects will be difficult.
Sorting Problems
Currently the biggest annoyance with sorting, is that
next_pass
materials aren't necessarily rendered immediately after their parent. Users have to rely entirely onrender_priority
to get correct sorting.Individual stencil effects which have material passes in both the opaque and the alpha passes will have more difficulty being correctly sorted. This might make effects such as portals and impossible geometry more challenging to implement.