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 demo showing the use of RenderingEffects to do post processing #942

Closed

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Aug 23, 2023

Adding a demo to show the use of RenderingEffects to create post processing.
Implements godotengine/godot#80214

This is still a pretty simple example, will work on making this more elaborate at some point.

image

@BastiaanOlij
Copy link
Contributor Author

Next to the very simple grayscale example I've added a work in progress, and not working correctly yet, Stocastic SSR implementation.

Decided to already push this in as it's a much bigger example on how to create different type of passes, how to create additional buffers, how to obtain state around the rendering, and how to do a raster pass.

image

@Ansraer
Copy link

Ansraer commented Sep 15, 2023

While trying the hooks PR yesterday, I noticed that you use float gray = max(max(color.r, color.b), color.g); in your grayscale shader. However, given that the human eye is far more sensitive when it comes to detecting green colors, the correct way to get luminance should be something along the lines of color.r * 0.2125 + color.g * 0.7154, color.b * 0.0721;

Which reminds me, I need to go through the built-in godot shaders. We use this strange max approximation way too often.

@BastiaanOlij
Copy link
Contributor Author

@Ansraer , changed the grayscale logic :)

Copy link

@RedMser RedMser left a comment

Choose a reason for hiding this comment

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

I've used this demo as a reference point and found parts of it using an outdated API. Here's my findings.

Note that my comments are not exhaustive, so "apply changes" via the GitHub GUI won't find all occurrences.

Comment on lines 2 to 3
extends RenderingEffect
class_name RenderingEffectGrayScale
Copy link

Choose a reason for hiding this comment

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

The API renamed to CompositorEffect, should be changed in multiple files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I need to revisit this

Comment on lines 53 to 54
# Barrier
rd.barrier(RenderingDevice.BARRIER_MASK_ALL_BARRIERS, RenderingDevice.BARRIER_MASK_COMPUTE)
Copy link

Choose a reason for hiding this comment

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

Manual barriers are deprecated and should be removed, should be changed in multiple files.

tonemap_mode = 2
ssr_depth_tolerance = 3.41
glow_enabled = true
rendering_effects = Array[RenderingEffect]([SubResource("RenderingEffect_rprpc"), SubResource("RenderingEffect_sm2nk"), SubResource("RenderingEffect_jcxua")])
Copy link

Choose a reason for hiding this comment

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

Must use the new Compositor API.

Comment on lines +29 to +36
if nearest_sampler.is_valid():
rd.free_rid(nearest_sampler)
if linear_sampler.is_valid():
rd.free_rid(linear_sampler)
if reflection_shader.is_valid():
rd.free_rid(reflection_shader)
if overlay_shader.is_valid():
rd.free_rid(overlay_shader)
Copy link

Choose a reason for hiding this comment

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

Some RIDs are not being cleaned up here, such as the *_pipeline variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As before, dependent RIDs are automatically freed.

Comment on lines +23 to +24
if shader.is_valid():
rd.free_rid(shader)
Copy link

Choose a reason for hiding this comment

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

The pipeline RID is not being freed here.

Comment on lines +16 to +17
if shader.is_valid():
rd.free_rid(shader)
Copy link

Choose a reason for hiding this comment

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

The pipeline RID is not being freed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you free a shader, all dependencies of the shader will be freed, including all pipelines related to that shader.

Comment on lines +188 to +191
rd.compute_list_bind_uniform_set(compute_list, depth_set, 0)
rd.compute_list_bind_uniform_set(compute_list, normal_set, 1)
rd.compute_list_bind_uniform_set(compute_list, color_set, 2)
rd.compute_list_bind_uniform_set(compute_list, reflect_set, 3)
Copy link

Choose a reason for hiding this comment

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

I'm not nearly as experienced with compute shaders as you are, but I'll still risk asking a potentially stupid question here:

Why create multiple uniform sets, with one binding each? Why not create a single uniform set with multiple bindings? The docs also only ever do it the way that's done here without further elaborating. But in my testing, both seem to work fine.

Copy link

Choose a reason for hiding this comment

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

I've noticed that I can use the same set with different bindings as well, so I've just started looking at the following document:

https://developer.nvidia.com/vulkan-shader-resource-binding

One thing the article notes:

We recommend making use of the different set numbers, to avoid redundant bindings. Putting many bindings that have very different frequencies in the same DescriptorSet can be bad for overall performance. Imagine a DescriptorSet with several textures and uniform buffer binding of which only one changes, that’s potentially a lot of data being sent to the GPU that effectively doesn’t do anything.

☝️ I'm not entirely sure I understand comprehensively yet, but it seems to me that separating different uniforms into sets can help with optimizing GPU usage. That said, it's hard for me to understand what would be ideal for a typical game project in Godot.

Copy link

Choose a reason for hiding this comment

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

Thanks for the article, it's hard to find resources on this topic! On that note, most post processing shaders will be run every single frame and always update all of their bound uniforms, so I'm not sure if there's any benefit or drawback to doing it one way over the other here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no one size fits all answer here. As a general rule it is best to combine uniforms into a single set but there can be optimisations in splitting this out if certain uniforms remain unchanged over multiple frames while others change every frame.

Other than that its really personal preference, in this case I'm splitting them out to keep the code straight forward and easy to read.

effect_callback_type = RenderingEffect.EFFECT_CALLBACK_TYPE_POST_TRANSPARENT
RenderingServer.call_on_render_thread(_initialize_compute)

func _notification(what):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func _notification(what):
func _notification(what):

var shader : RID
var pipeline : RID

func _initialize_compute():
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func _initialize_compute():
func _initialize_compute():

shader = rd.shader_create_from_spirv(shader_spirv)
pipeline = rd.compute_pipeline_create(shader)

func _render_callback(p_effect_callback_type, p_render_data):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func _render_callback(p_effect_callback_type, p_render_data):
func _render_callback(effect_callback_type, render_data):

var apply_sssr_effect : RenderingEffectApplySSSR
var gray_scale_effect : RenderingEffectGrayScale

# Called when the node enters the scene tree for the first time.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Called when the node enters the scene tree for the first time.
# Called when the node enters the scene tree for the first time.

var cam_x = 0.0
var cam_y = 0.0

func _input(event):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func _input(event):
func _input(event):

uniform.add_id(input_image)
var uniform_set = UniformSetCacheRD.get_cache(shader, 0, [ uniform ])

# Run our compute shader
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Run our compute shader
# Run our compute shader.

if size.x == 0 and size.y == 0:
return

# We can use a compute shader here
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# We can use a compute shader here
# We can use a compute shader here .

var x_groups = (size.x - 1) / 8 + 1
var y_groups = (size.y - 1) / 8 + 1

# Barrier
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Barrier
# Barrier.

if !rd:
return

# Create our shader
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Create our shader
# Create our shader.


func _initialize_compute():
rd = RenderingServer.get_rendering_device()
if !rd:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if !rd:
if not rd:

@Calinou
Copy link
Member

Calinou commented May 22, 2024

I've updated the demo for 4.3.dev6 by pushing an additional commit, which incorporates @AThousandShips' suggestions. The demo works out of the box now.

However, I get error spam when MSAA 3D is disabled in the project settings (despite the scene rendering correctly):

 Image (binding: 0, index 0) needs the TEXTURE_USAGE_STORAGE_BIT usage flag set in order to be used as uniform.
  servers/rendering/renderer_rd/uniform_set_cache_rd.h:130 - Condition "rid.is_null()" is true. Returning: rid
  servers/rendering/rendering_device.cpp:4092 - Parameter "uniform_set" is null.
  Uniforms were never supplied for set (0) at the time of drawing, which are required by the pipeline.
  Image (binding: 0, index 0) needs the TEXTURE_USAGE_STORAGE_BIT usage flag set in order to be used as uniform.
  servers/rendering/renderer_rd/uniform_set_cache_rd.h:130 - Condition "rid.is_null()" is true. Returning: rid
  servers/rendering/rendering_device.cpp:4092 - Parameter "uniform_set" is null.
  Uniforms were never supplied for set (0) at the time of drawing, which are required by the pipeline.

PS: Is there a way to prevent the reflection from showing up on the sky? I've tried decreasing Max Distance in the SSSR effect but I need to decrease it to a very low value to prevent this, which breaks the reflection on other objects.

@BastiaanOlij
Copy link
Contributor Author

This has been replaced by #1058

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