-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Conversation
3d21c9c
to
445bdcd
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.
Looks great so far! Its amazing how simple this was in the end
servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp
Outdated
Show resolved
Hide resolved
dda0b2d
to
ed6597d
Compare
@clayjohn @reduz I've added the baseline implementation for exposing 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 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. |
1edc688
to
32cad33
Compare
32cad33
to
3b451a1
Compare
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) |
3b451a1
to
3fb0d69
Compare
Updated the example so it properly cleans up our shader when our RenderingEffect is being destroyed. Also added an enabled settings. |
3fb0d69
to
a1ba437
Compare
@clayjohn any idea what the CI fail is about? |
a1ba437
to
554dd1e
Compare
Check the Mono glue generation logs which explain why the Mono build fails:
|
I have no idea why I requested a review from 2d nodes... github, you drunk |
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... ). |
@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 |
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. |
servers/rendering/renderer_rd/forward_mobile/render_forward_mobile.cpp
Outdated
Show resolved
Hide resolved
c819903
to
2814b0e
Compare
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 |
541f557
to
bbd610e
Compare
@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 |
c36c029
to
ea230a4
Compare
ea230a4
to
5a98845
Compare
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? |
better give feedback early for planing, than later in merging faze |
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:
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:
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 |
@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 Do note that it's double included like so:
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 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: |
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. |
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.
Everything looks good to me now. Let's go ahead and merge this
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! 🎉 |
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 lacksRenderData
andRenderSceneData
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
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
Proposal for a future implementation of rendering effects to simplify single pass effects using Godots build in material system:
godotengine/godot-proposals#7849