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 hooks into renderer #80214

Merged
merged 1 commit into from Feb 20, 2024

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Aug 3, 2023

Implementation based on a suggestion from Reduz

This adds the ability to register rendering effects as callbacks in the rendering engine. These callbacks will be called at predetermined places in the rendering engine and allow the user to implement additional render passes to be performed on the data.

A new resource class CompositorEffect is added within Godot that can be used to implement such an effect, it can then be registered as part of a new compositor resource which can be set on the WorldEnvironment or Camera3D. Any viewport using that compositor will apply those effects.

Note that this PR only adds full support to the Forward+ renderer and limited support on the mobile renderer.
Support for the compatibility renderer is possible but requires a lot of structural changes as the compatibility renderer currently does not manage buffers through it's RenderSceneBuffer object and lacks RenderData and RenderSceneData implementations. It would be worth adding that structure.

Additional follow up ID: Also add a "Pre canvas" and "Post canvas" mode and add these callbacks to the rendering effect. This will require a separate callback in the RenderingEffect object, but it should be really straight forward.

Test project that turns the output of the renderer to grayscale in a post effect:
godotengine/godot-demo-projects#942

image

Full implementation showing a multi-part post process: https://github.com/BastiaanOlij/RERadialSunRays
Stream that does a deep dive into this project: https://youtube.com/live/insa9VY0ruA

image

Proposal for a future implementation of rendering effects to simplify single pass effects using Godots build in material system:
godotengine/godot-proposals#7849

@BastiaanOlij BastiaanOlij added this to the 4.2 milestone Aug 3, 2023
@BastiaanOlij BastiaanOlij self-assigned this Aug 3, 2023
@BastiaanOlij BastiaanOlij force-pushed the rendering_effect branch 4 times, most recently from 3d21c9c to 445bdcd Compare August 8, 2023 08:54
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.

Looks great so far! Its amazing how simple this was in the end

@BastiaanOlij BastiaanOlij force-pushed the rendering_effect branch 2 times, most recently from dda0b2d to ed6597d Compare August 9, 2023 04:36
@BastiaanOlij
Copy link
Contributor Author

@clayjohn @reduz I've added the baseline implementation for exposing RenderData and RenderSceneData into this.
RenderData actually encapsulates RenderSceneData and RenderSceneBuffers so we don't need to send extra parameters to our callback.

I've only provided access to some of the base information held in these objects. I think most of the rest of the data, if we want to expose those, should be exposed in the RD implementation as they are unlikely to be shared on other renderers.

That said, this approach would allow us to create similar classes for GLES and add this logic to GLES as well.

At this moment though, I'm only exposing this on Forward+. I think that should be our goal for this PR, with looking at adding it to mobile and/or compatibility later on.

I've also added the Extension classes even though they are not usable until we allow creating renderers through GDExtension. So that is a bit of future music.

I'll be cleaning up CI errors later today and I'll add a simple example project.

@BastiaanOlij
Copy link
Contributor Author

@clayjohn @reduz another thing I'm wondering about, should we add some hooks into the 2D renderer as well? We'll need a different callback for it returning an object with data related to the 2D rendering, but it shouldn't be hard to add.

@BastiaanOlij
Copy link
Contributor Author

cc @henriksod, you might want to start playing around with this to optimise your planet example. That would be a great test and showcase for this logic (I might work on that over the weekend)

@BastiaanOlij
Copy link
Contributor Author

Updated the example so it properly cleans up our shader when our RenderingEffect is being destroyed.

Also added an enabled settings.

@BastiaanOlij
Copy link
Contributor Author

@clayjohn any idea what the CI fail is about?

@akien-mga
Copy link
Member

@clayjohn any idea what the CI fail is about?

Check the Mono glue generation logs which explain why the Mono build fails:

Generating Environment.cs...
ERROR: Return type from getter doesn't match first argument of setter for property: 'Environment.rendering_effects'.
   at: _generate_cs_property (modules/mono/editor/bindings_generator.cpp:1922)
ERROR: Failed to generate property 'rendering_effects' for class 'Environment'.
   at: _generate_cs_type (modules/mono/editor/bindings_generator.cpp:1528)
ERROR: Generation of the Core API C# project failed.
   at: generate_cs_api (modules/mono/editor/bindings_generator.cpp:1325)
ERROR: --generate-mono-glue: Failed to generate the C# API.
   at: handle_cmdline_options (modules/mono/editor/bindings_generator.cpp:4086)

@BastiaanOlij BastiaanOlij force-pushed the rendering_effect branch 2 times, most recently from 089e4b0 to c819903 Compare February 12, 2024 23:50
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner February 12, 2024 23:50
@BastiaanOlij
Copy link
Contributor Author

I have no idea why I requested a review from 2d nodes... github, you drunk

@akien-mga akien-mga removed the request for review from a team February 13, 2024 09:09
@clayjohn
Copy link
Member

https://github.com/Khasehemwy/godot

Oh, that's amazing! I didn't realize they were still working on that.

@Khasehemwy Would you be willing to make a PR with your reversed-z changes? Taking a look at your branch, it looks pretty good already

@Khasehemwy
Copy link
Contributor

https://github.com/Khasehemwy/godot

Oh, that's amazing! I didn't realize they were still working on that.

@Khasehemwy Would you be willing to make a PR with your reversed-z changes? Taking a look at your branch, it looks pretty good already

@clayjohn I changed the rendering process of all rendering modes to reversed-z, and tested it with a custom shader without any problems. The sky and all the built-in lights of the Spatial Shader also have been adapted.

However, I tested effects like SSAO, there will be problems after changing to reversed-z. If we want to fully support reversed-z, there should be many places that need to be modified. I'm not going to continue looking for problems and fixing them ( I have other work to do... ).
Can I make a PR now? In fact, I haven't made a PR yet.

@BastiaanOlij
Copy link
Contributor Author

@Khasehemwy I think your concerns there are valid however we still have several weeks to fix any fallout.

We would definitely appreciate the PR, just make clear in the description what it covers and that you haven't gone through all effects to deal with any changes. If it works solidly we can merge it and then help with cleaning up any effects that will break. In most cases it's just going to be reversing a sign or a < to a >.

@BastiaanOlij
Copy link
Contributor Author

Also @kebabskal , please see discussion above, I do not know if you use the depth buffer in any of your effects but be ready for that change. I know that my examples need to be adjusted.

scene/2d/animated_sprite_2d.cpp Outdated Show resolved Hide resolved
scene/2d/sprite_2d.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.cpp Outdated Show resolved Hide resolved
servers/rendering/renderer_canvas_cull.h Outdated Show resolved Hide resolved
servers/rendering/renderer_viewport.cpp Outdated Show resolved Hide resolved
@BastiaanOlij
Copy link
Contributor Author

Ok @clayjohn so turns out I somehow messed up the last time I updated the commit. I must have been rushing through it and instead of finishing the rebase, I just commited my changes into the last commit.

I was able to salvage it, but I may have lost a change of my own. I hope that one I messed up was just a rebase, as far as I can tell it was but damn this is scary stuff

@BastiaanOlij BastiaanOlij force-pushed the rendering_effect branch 2 times, most recently from 541f557 to bbd610e Compare February 17, 2024 04:13
@stuartcarnie
Copy link
Contributor

@BastiaanOlij you can still compare all your force-push changes, by looking back up this history for all the "Force pushed" comments. Each one has a compare link.

@BastiaanOlij
Copy link
Contributor Author

@BastiaanOlij you can still compare all your force-push changes, by looking back up this history for all the "Force pushed" comments. Each one has a compare link.

Yup, thats how I found the push that was still good, I did some fixes based on feedback from Akien, force pushed it, then realised I needed to do another rebase due to a merge conflict, and I screwed that one up somehow.

Was able to revert to the previous force push, redo the rebase properly this time, and we're home free :)

Just a minor heart attack in the meanwhile :P

@BastiaanOlij BastiaanOlij force-pushed the rendering_effect branch 3 times, most recently from c36c029 to ea230a4 Compare February 18, 2024 10:52
@novaplusplus
Copy link
Contributor

The last few days I've been doing a fair bit of experimenting and picking at and digesting of this PR in my local engine copy - are things at a stage where "end user" type feedback is wanted or are things expected to change to the extent that it's a bit of a moot point?

@octanejohn
Copy link

better give feedback early for planing, than later in merging faze

@novaplusplus
Copy link
Contributor

Well, in general my testing has been going well but there are some tasks that require a lot of intimate knowledge of how things are done internally to do here. I know this is a very low-level thing but I feel there could be a couple of extra things added that would help certain things go easier. Maybe not right out the door, but down the road.

Plus it's entirely possible I'm trying to use this in ways for things that are not intended. But here goes regardless.

Specifically on my mind are the various matrices and other information that normally gets fed into shaders, which are generally quite useful for post processing shaders, especially things like raymarching. Of course this is from someone not super familiar with this low level of render pipeline, so maybe I am meddling in affairs I shouldn't be (going down the rabbit hole to figure this stuff out sure has taught me stuff, though)

Still - here's one example of what I have to do to get the matrices loaded into the shader properly, basically just replicating what happens when the buffer is loaded up before being passed to ordinary shaders:

	var proj_correction := Projection().create_depth_correction(true)
	var inv_projection_matrix := (proj_correction * scn_data.get_cam_projection()).inverse()
	var inv_view_matrix := Projection(scn_data.get_cam_transform())

Not many lines after all is said and done, but it took a lot of headscratching and digging through the engine source to realize that's what I had to do. Coupled with things varying on double precision vs single precision builds (I've been running doubles), and will need more steps once the reversed Z buffer gets merged.

Loading the same buffer that the shaders get is possible too, and maybe preferable, although you have to replicate the struct in the glsl code to be able to get at it, which is kind of annoying and feels failure prone as well if it changes with engine updates. I don't know what would be an elegant method to include that though since it's working with plain GLSL code instead of godot's shader syntax. My gdscript side looked like this:

	var ubo: RID = scn_data.get_uniform_buffer()
	var param_unif := RDUniform.new()
	param_unif.uniform_type = RenderingDevice.UNIFORM_TYPE_UNIFORM_BUFFER
	param_unif.binding = 0
	param_unif.add_id(ubo)
	var param_unif_set := UniformSetCacheRD.get_cache(shader, 2, [param_unif])

This isn't neccessarily stuff that needs code changes, but something that should be considered in documentation. Though some helper methods (as part of CompositorEffect?) to automate some of these common tasks wouldn't be a bad idea I think

Although, if all of this is leading into a future extra shader type (alongside canvas, spatial, sky, fog, etc) that automates all of this then, then all of these issues sort of go out the window.

That turned out a bit longer than I intended... wish I could be more useful than just shouting from the peanut gallery but low level graphics stuff is very foreign to me

@BastiaanOlij
Copy link
Contributor Author

@novaplusplus all good stuff thanks! We're trying to get this over the line as is but there will definitely be follow up PRs to see where we can make things easier.

On the matrix issue, indeed you do have direct access to the data already loaded through RenderSceneData.get_uniform_buffer.
The missing bit is knowing what this uniform buffer contains. This is something I still want to improve upon because like you say, this uniform buffer can change from Godot version to Godot version, however you can find it in source here: https://github.com/godotengine/godot/blob/master/servers/rendering/renderer_rd/shaders/scene_data_inc.glsl

Do note that it's double included like so:

layout(set = 1, binding = 0, std140) uniform SceneDataBlock {
	SceneData data;
	SceneData prev_data;
}
scene_data_block;

The plan is to eventually have a system where standard include files in Godot are actually available for custom shaders, so you can actually do a #include "scene_data_inc.glsl", but that is future music as part of the shader template logic we're planning.

I think for the most part for now, these are just things that need to end up in the manual, or in examples. Eventually out of that convenience improvements will come forward so users don't need to reinvent the wheel when figuring out how to do these things.

But the flip side of that argument is that this logic is geared at users who do have that knowledge. We can make some things easier, but it's not entirely possible to dummy proof this. We're working directly on the internals of the rendering engine.

What I'm hoping for is that we'll eventually get plugins written on top of this that implement specific effects that other users can use without needing to know the difficult internal stuff. Take for instance the implementation I did on stream here that allows a user to just supply the core shader logic for an full screen post effect:
https://www.youtube.com/watch?v=KLmahauWeV4

@novaplusplus
Copy link
Contributor

The plan is to eventually have a system where standard include files in Godot are actually available for custom shaders, so you can actually do a #include "scene_data_inc.glsl", but that is future music as part of the shader template logic we're planning.

Yeah, that would be great! That would pretty much solve these particular difficulties.

And yeah, like I was saying, this is very much down in the guts of the rendering engine, so I understand there's only so much convenience and user friendliness that is reasonable to expect.

When the time comes I hope I can use what I've learned to help make some examples and guidelines as well.

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.

Everything looks good to me now. Let's go ahead and merge this

@akien-mga akien-mga merged commit 292f4c7 into godotengine:master Feb 20, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks for the amazing work, and the patience to go through multiple iterations :)

Now let's see how users can put this to good use! 🎉

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.

None yet