Implement motion blur feature based on the community plugin#115027
Implement motion blur feature based on the community plugin#115027HydrogenC wants to merge 1 commit 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: |
0fe58f0 to
787e16d
Compare
|
It's mostly probable that the problem is caused by network failure in the midst of rebase. It's pretty tricky and I find it difficult to make a fix. So I squashed the commits to solve the problem. Now at least it returns normal, at a cost of losing detailed commits (they are backed up on another branch though). |
|
Make sure to use |
I was using the Github App GUI to do the rebase, and it left a mess if the network fails in the middle. 😞 |
787e16d to
cf45e1e
Compare
When looking at a PR for the first time, I like to review the newly exposed public facing API by simply looking at the docs files to see what was added. Would it be possible to run |
@AR-DEV-1 Will you take this when you have time? |
|
@AR-DEV-1 the latest commit seems broken. Roque curly brackets. |
|
It feels like this excellent plugin is finally nearing completion, and the revival of HDDAGI is just around the corner. |
Ye though a bit off topic , but keep expectation low for hddagi , nothing can be sure at all until there’s a new pr or some progress being shown. |
|
One of the main reasons for a feature PR to be merged is that the feature cannot be implemented as a plugin or there are substantial compromises to implementing it as a plugin. In the description, I see a mention that performance should be better with this PR compared to the plugin, but it would be good to see some real world and benchmark performance comparisons between this PR and the plugin to demonstrate how substantial the performance gains are. This will help validate that this PR is worth the long term maintenance cost. It would also be good to have this PR's description list and detail any other improvements or benefits to having this feature added to Godot compared to what was possible with the plugin. These will, again, help demonstrate why the long term maintenance of the feature is truly required and cannot be handled by a plugin. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Let's stay on topic |
|
File formatting & etc, failing. Will format them |
|
Oh my, only noticed this PR today, you're the man @HydrogenC! Unfortunately busy with my day job but as soon as I'm able, I'm gonna get around to testing this PR thoroughly! ❤️ Regarding motion blur in the editor, IIRC in Unreal motion blur is rendered in editor, I think their reasoning is that it can affect how things like animations and camera pans look for example and you'd ideally wanna see how they look without having to run your game. However, in Unity, it's not rendered in editor. Ideally of course a project option that allows you to chose would be best. Regarding the full suite of options @sphynx-owner has implemented in the plugin, I think for an initial motion blur implementation into core, just the core, fundamental effect should be the priority. Once that's done, we can then implement things like framerate independent motion blur, camera/object separated motion blur, ect, as additional PRs afterwards. I think this is also more in line with how features/enhancements are generally tackled with Godot development? |
Yeah, the current status is that all of these additional and advanced stuff are actually already implemented in this PR. Now the problem is that the number of parameters is above the number that keeps the effect simple so we have to scale them down (which means that the hardcoded parameters will possibility never be added back as configurable ones). |
|
I personally believe that the current implementation is complete and coherent. There is still some discussion around the tile size property that needs resolving, but given the documentation I added we should be on the same page on what the different proeprties mean at least. |
I think these are key features that shouldn't be stripped out of this PR. I hope the maintainers won't decide to remove them for the "initial" implementation. What parameters will be configurable by users can and should be discussed and stripped down if it makes sense, but since motion blur is mostly an artistic tool, I'd argue, it should be treated as such (instead of only a tool to smoothen things out for 30fps games) since not all games will need the same look. |
8d718f6 to
6839494
Compare
7b20b1b to
4d828f1
Compare
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>
4d828f1 to
96db643
Compare




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.