Implement motion blur feature based on the community plugin#115027
Implement motion blur feature based on the community plugin#115027HydrogenC wants to merge 6 commits intogodotengine:masterfrom
Conversation
|
Already feature complete and workable, but API might be subject to change. We may want the API to get reviewed first before marking this PR as ready. |
|
All the third-party content needs to be correctly attributed, this currently violates their license Edit: especially seeing how it's so directly copied you even copied typos and TODOs from the original 🙃 |
I'll do it. |
|
I apologize for the bother, I should have been cleaner and clearer about the setup. |
|
@HydrogenC Please update the PR's description with the following: (EDIT: THE CHANGES REQUESTED HAVE BEEN MERGED BY THE OP) |
Not really. I made plenty of modifications actually. The TODOs are there because they are not yet resolved so I keep them to hint future contributors. For the TODOs that I have resolved, I already removed them. This doesn't mean that this PR is underworked. |
Nor did I say so! |
|
Only works for PracticalCameraAttr? Also I notice that then v-sync is disabled I can barely notice the blur |
We can move it elsewhere if required, like to environment. Now it's under practical camera attributes. |
Yeah I mean, I dont mind that is there, but its not present in PhysicalCameraAttr |
|
When rendering driver is d3d12 this happens: Properties do not appear until you reopen camera_attributes resource: This one is hard to see but when you run the project with motion blur enabled a little flickering can be seen: Only camera rotation gives effect: I think some properties should be changed in rendering/camera instead of in the camera_attributes: |
|
Wow this is a lot. The second issue can be fixed with calling |
Intensity is set to 100.0 in that clip if you are talking about that |
I think it is fine in PracticalCameraAttr, as the base class (CameraAttr) has only stuff regarding exposure & auto-exposure & PhysicalCameraAttr also has the same, only with frustum added. So, if required, we could change it. |
@AThousandShips I have a small question. I've done some debugging on this problem and I found removing specification constants fixes the problem. Is specification constants unsupported in the D3D12 backend? |
You may want to also ask @clayjohn |
Is fixed with |
|
I unfortunately can't help with rendering questions clay would be a better person to respond, and I'm sure he will when he has the time |
9f428ff to
5c284ec
Compare
|
@HydrogenC I have a fix for the 4th issue: |
|
sphynx-owner, I'm very happy to see your involvement in this PR, even if just from a testing and review perspective. This sort of testing and insight from an expert on the subject is invaluable to the development of these features! Thank you! |
|
Every setting in rendering has this Edit: |
4d828f1 to
96db643
Compare
|
I forgot to mention: thanks for providing the generated docs for this PR! That makes it much easier for everyone to start review of the PR. I have been overwhelmed working on HDR ouptut, so I'm not sure if I'll be the person to review this PR, but the public-facing API is critically important to review, as this cannot easily change, if at all, between Godot versions. I noticed that my previous comment received a lot of confused emojis, so I understand that there might be some confusion about what I wrote. I'll provide some clarification: I wrote that note from the perspective that is generally taken with all PRs in Godot. This process helps us ensure a high quality, bug-free, maintainable game engine in the long run and is why we ask "Is there a reason why this should be core and not an add-on in the asset library?" in the proposal template. I noticed that the proposal for this PR has notes about how this PR should be better than the existing plugin. All that said, I was not aware that a decision may have been made to include a motion blur feature in the Godot engine without the need for a plugin. So assuming that this decision has been made, it is still important to verify the performance characteristics of this PR and any differences/improvements/etc. that have been made compared to the plugin. This not only gives validation that integrating the feature into the core engine is valuable, but also ensures that the quality and characteristics of this PR are as-expected. So, regardless of the exact reasons, seeing some performance comparisons and details regarding any other improvements or benefits compared to the plugin this PR is based off of will still be extremely valuable in the review of this PR. If others who are interested in this PR are able, it would be good to test this PR with your own project to see how it performs, etc. Once a bug, problem, or limitation has been merged into Godot it becomes much harder to fix later on, so lots of testing of PRs is always appreciated! |
Graphics part by @HydrogenC, editor part mainly by @AR-DEV-1 & docs mainly by @sphynx-owner Co-Authored-By: AR <ardev1.deverson@proton.me> Co-Authored-By: sphynx-owner <61445300+sphynx-owner@users.noreply.github.com>
At the godot priorities, they mention the motion blur pr and that they wanted to add common effects as built in . See https://godotengine.org/priorities/#significantly-improve-post-processing-effects |
In our priorities, it is clearly mentioned the need for built-in common effects & the motion blur PR is also mentioned there. (Edit: Saul said the same thing as me, I just found about it now) |
|
Tested, works as expected on my end (MBP M1 Max). I already used @sphynx-owner's plugin and found it to give production-ready quality and performance, at least in my testing.
The current set of settings seems mostly fine. Object/movement/rotation multipliers are essential imo, velocity thresholds and framerate modes seem useful too. The reason why I think it's best to give more control here is that motion blur is a bit of a divisive topic and it's mostly the camera rotation blur that people are more likely to find annoying. It's possible that most don't even know that separate object blur is possible and would prefer to keep it if it was an option in game settings. Framerate modes on the other hand seem to be useful to devs because how blur relates to the framerate is often a hot topic among devs. I'm unsure about I have a nitpick regarding the names of properties and UI, it would be nice to be explicit about movement and rotation referring to camera movement and rotation, and properties could be grouped in inspector like this: You could name properties from generic to specific like this:
And then let
|
QbieShay
left a comment
There was a problem hiding this comment.
I'm looking at this and I have some comments.
1st comment: good job everyone! Makes me happy to see motion blur progressing. it was a long and large team effort and it makes me happy to see everyone working together!
A couple of question
- Why is it a camera effect and not a world environment effect? Was this dicussed somewhere? what's the rationale?
- I see that
show in editorfor motion blur is a project setting. I wonder if it should be editor setting. Thoughts? If it's project setting it will be synced across collaborators on a project with git.
Critique:
- I still feel like the properties about the tile (size and clamping) is not necessary to be exposed to the user. What are the different usecase for those? If they really need to be exposed, they should have more comprehensible names. I still don't understand what they're for.
- What passivestar said about the property names. camera stuff should have
cameraprefix because right now it's extremely unclear of what is what without reading the tooltip - The default camera rotation blur imo should be much lower. I'm part of those people that get motion sick very easily and it's very disorienting as it is now.
- I don't understand the usecase for fixed framerate. Sounds like something that was exposed "because why not" more than an actual usecase? If that's indeed the case, it should be removed until there's demand. I may 100% be wrong on this, so if there is a usecase please supply it.
- I am a bit confused about reference framerate at 30, can somebody elaborate on why 30 and not 60?
- Velocities threshold should be under a toggle instead of changing behaviour when they're both at 0 (unless i misunderstood how they work). It looks like a niche usecase to want to wiggle those manually.
Final thoughts:
I know we've discussed this at length but I still think there should be clearer options for what type of game needs what settings at what value. I understand that racing games, fps games, top down games etc will all have different needs, but as it is right now it's hard to understand the correlation between a property and the kind of game it suits at what value. The naming is a bit aseptic and I think we should sacrifice a bit of precision and technicality in the naming in favor of better clarity for someone that doesn't know in-depth how motion blur works.
Some cameras would need to have it disabled or with different values, for example: if you had low framerate security camera in your game you would want to make it have less intensity or disable it
I agree that it should be in editor settings, I did not think of that before but now that I do it makes a lot of sense, but where should it be put?
I will use fixed framerate mode in my game, if a player has 500 fps and 120 hz for example they would almost not see or even fully not see any motion blur, fixed framerate mode prevents that |
I understand if the documentation I added is insufficient, but I am not sure how to improve upon their name, open to suggestions. Tiles size is a "quantity over quality" parameter, and clamping could be removed and be always enabled behind the scenes.
I agree with all of passivestar's suggestions.
I think since this is a relatively obscure feature, it should not be varied from its complementary movment and object blur parameters by default, however I do think the default intensity in general is way to high. It is set to FULL by default currently, should be at 0.1-0.3.
The reference framerate at 30, combined with the default
I agree that setting the thresholds to 0 being the way to "turn them off" is janky. I am not sure if you are suggesting getting rid of the sliders and replatcing them with toggles, which would not make sense, but I do agree that having a toggle to expose-and-enable/unexpose-and-disable them makes sense. |
Something like "detailed - balanced - intense"?
Ok, I need to think more about this but it sounds like it could be "minimum reference fps" then? If I understood correctly
I was suggesting to remove the sliders unless the feature is "toggled on" with a bool. One of those bool categories. |
More like refined - balanced - sparse, perhaps.
Depending on the framerate mode. In
sounds good then. |
realistic - balanced - stylized? |
'Cause motion blur is like DoF, it's a effect that originates from the camera / human eye instead of something that really exists in the environment. I put it in camera attributes only because DoF is in camera attributes. To be clear, there isn't any blockers that forbid motion blur to be part of environment instead of camera attributes. If it being in environment is more convenient for real-life applications we could migrate it right now.
It certainly could be removed. AFAIK both Unity and Unreal hardcode this value to a constant. But if we do that we would have to guide those users who really need to tweak this to clone the engine source and edit the shader in the docs.
Nice catch. You're right on this. |
servers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glsl
Outdated
Show resolved
Hide resolved
| vec2 current_uv = uvn + vec2(float(i) / render_size.x, 0); | ||
| vec4 velocity_sample = textureLod(velocity_sampler, current_uv, 0.0); | ||
|
|
||
| // If the depth at the potential dominant velocity is infinity (background or skybox) |
There was a problem hiding this comment.
this comment should be changed to reflect that it's part of our code base now
servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glsl
Outdated
Show resolved
Hide resolved
| <member name="editors/3d/view_plane_rotation_gizmo_scale" type="float" setter="" getter=""> | ||
| The scale of the outer circle of the rotation gizmo as view plane rotation in the 3D editor. If set to [code]2.0[/code], the outer circle has twice the radius of the XYZ rotation gizmo sphere. | ||
| </member> | ||
| <member name="editors/3d/viewport_visuals/show_motion_blur_in_editor" type="bool" setter="" getter=""> |
|
Please use the batch suggestion feature to not create a ton of unnecessary commits and noise |
|
@AR-DEV-1 You can squash all suggestion commits locally and then force push. |
|
After applying the remaining changes |
|
Related issue, posting it here since motion blur will likely have the same issue. #54518 |
|
yep, tile size is in pixels, so at higher resolutions, the blur's dilation range would be relatively smaller. This is described in the documentation and expected. The effect itself should still be consistent in other aspects. |
Co-Authored-By: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>





This PR implements the motion blur post processing based on the community plugin created by @sphynx-owner . The original repos could be found here: https://github.com/sphynx-owner/godot-motion-blur-addon-simplified and https://github.com/sphynx-owner/JFA_driven_motion_blur_addon.
Closes godotengine/godot-proposals#12258 and godotengine/godot-proposals#2933.
There are some differences compared to the original plugin, notably:
This PR is co-authored with @AR-DEV-1 , with me doing the graphics and shaders part and @AR-DEV-1 working with the editor settings parts.
Supported backend: Forward+.
Preview:

Docs not yet written, since the API itself may be subject to change during review. Docs will be finished by @AR-DEV-1 after the API itself gets approved.The current blocker is that the current implementation exposes too many parameters, we may have to scale the number down and make some of them constant instead.
Note
godot-cppwill also be modified with these changes after the the API itself gets approved. The build for the editor with mono on Linux can be ignored for now.