Add motion blur support#1201
Open
Shakai-Dev wants to merge 8 commits into
Open
Conversation
WalkthroughAdds motion blur support across rendering, camera attributes, project settings, and editor wiring. Also adds ChangesMotion Blur Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
servers/rendering_server.cpp (1)
3190-3216:⚠️ Potential issue | 🟠 MajorMissing
BIND_ENUM_CONSTANTentries forMotionBlurFramerateMode.
camera_attributes_set_motion_blur_framerate_modeaccepts aMotionBlurFramerateModeparameter (line 3190), and the enum is declared inrendering_server.hwith valuesNATIVE,CAPPED, andFIXED(lines 1408–1412). However, unlikeMotionBlurQualityandMotionBlurTileSize(lines 3209–3216), there are no correspondingBIND_ENUM_CONSTANTcalls for theMOTION_BLUR_FRAMERATE_MODE_*values in_bind_methods(). Script users would be forced to pass raw integers (0, 1, 2) instead of named constants.🔧 Proposed addition (after the existing TileSize constants)
BIND_ENUM_CONSTANT(MOTION_BLUR_TILE_SIZE_EXTRA_LARGE); + + BIND_ENUM_CONSTANT(MOTION_BLUR_FRAMERATE_MODE_NATIVE); + BIND_ENUM_CONSTANT(MOTION_BLUR_FRAMERATE_MODE_CAPPED); + BIND_ENUM_CONSTANT(MOTION_BLUR_FRAMERATE_MODE_FIXED);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering_server.cpp` around lines 3190 - 3216, Add missing BIND_ENUM_CONSTANT entries for the MotionBlurFramerateMode enum so script users can use named constants instead of raw integers: locate _bind_methods() where camera_attributes_set_motion_blur_framerate_mode is bound and append BIND_ENUM_CONSTANT(MOTION_BLUR_FRAMERATE_MODE_NATIVE), BIND_ENUM_CONSTANT(MOTION_BLUR_FRAMERATE_MODE_CAPPED), and BIND_ENUM_CONSTANT(MOTION_BLUR_FRAMERATE_MODE_FIXED) (matching the enum declared for MotionBlurFramerateMode) alongside the other motion blur enum bindings.
🧹 Nitpick comments (3)
servers/rendering/renderer_rd/renderer_scene_render_rd.cpp (1)
546-560: Optional: avoid motion blur warnings when effects are already disabled.If
can_use_effectsis false (e.g., tiny targets or debug modes), the storage/editor checks can emit warnings even though the effect won’t run. Consider gating the checks undercan_use_effectsto avoid log noise and redundant attribute queries.♻️ Possible refactor
bool using_motion_blur = RSG::camera_attributes->camera_attributes_uses_motion_blur(p_render_data->camera_attributes); -if (using_motion_blur && !can_use_storage) { - WARN_PRINT_ONCE("Motion blur requires storage support in shader. Disabling motion blur."); - using_motion_blur = false; -} - -if (using_motion_blur && !RSG::camera_attributes->camera_attributes_get_motion_blur_show_in_editor() && Engine::get_singleton()->is_editor_hint()) { - using_motion_blur = false; -} - -if (can_use_effects && using_motion_blur) { - RENDER_TIMESTAMP("Motion Blur"); - motion_blur->motion_blur_compute(rb, p_render_data->camera_attributes, p_render_data->scene_data, p_render_data->transparent_bg, time_step, copy_effects); -} +if (can_use_effects && using_motion_blur) { + if (!can_use_storage) { + WARN_PRINT_ONCE("Motion blur requires storage support in shader. Disabling motion blur."); + using_motion_blur = false; + } + + if (using_motion_blur && !RSG::camera_attributes->camera_attributes_get_motion_blur_show_in_editor() && Engine::get_singleton()->is_editor_hint()) { + using_motion_blur = false; + } + + if (using_motion_blur) { + RENDER_TIMESTAMP("Motion Blur"); + motion_blur->motion_blur_compute(rb, p_render_data->camera_attributes, p_render_data->scene_data, p_render_data->transparent_bg, time_step, copy_effects); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/renderer_scene_render_rd.cpp` around lines 546 - 560, The motion-blur storage and editor-warning checks run even when effects are disabled, causing noisy logs; wrap the logic that queries RSG::camera_attributes and checks can_use_storage and editor hints inside the can_use_effects guard (i.e., only evaluate using_motion_blur and adjust using_motion_blur when can_use_effects is true) so that WARN_PRINT_ONCE and the editor_hint check are skipped when can_use_effects is false, leaving the final conditional that calls motion_blur->motion_blur_compute(rb, p_render_data->camera_attributes, ...) unchanged.servers/rendering/renderer_rd/effects/motion_blur.cpp (1)
275-284: TODO comment suggests unfinished design — velocity threshold multipliers use a single pair of thresholds for all three velocity types.Line 275 has
// TODO: add these multipliers to settings. Currently,velocity_lower_thresholdandvelocity_upper_thresholdare applied identically to rotation, movement, and object thresholds. Per-type thresholds would provide finer control as the TODO implies, but the current flat mapping is functional.Would you like me to open an issue to track per-velocity-type threshold settings?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/effects/motion_blur.cpp` around lines 275 - 284, The TODO notes that per-velocity-type threshold settings are missing: currently velocity_lower_threshold and velocity_upper_threshold are written into motion_blur.preprocess_push_constant.* for rotation, movement and object equally; update the implementation to read and assign distinct thresholds for each velocity type (e.g., rotation_velocity_lower_threshold/upper, movement_velocity_lower_threshold/upper, object_velocity_lower_threshold/upper) by adding new getters in RSG::camera_attributes (or new config fields) such as camera_attributes_get_motion_blur_rotation_lower_threshold, camera_attributes_get_motion_blur_movement_lower_threshold, camera_attributes_get_motion_blur_object_lower_threshold (and corresponding *_upper_threshold) and then use those values instead of the shared velocity_lower_threshold/velocity_upper_threshold when setting motion_blur.preprocess_push_constant.servers/rendering/renderer_rd/effects/motion_blur.h (1)
67-82:support_fsr2field appears unused as a runtime parameter.In the implementation (
motion_blur.cppLine 286),support_fsr2is hardcoded to1.0fand never varies. If this is a constant, it would be better as a specialization constant in the shader rather than occupying push constant space. If it's intended to be dynamic in the future, consider adding a TODO comment to clarify intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/effects/motion_blur.h` around lines 67 - 82, The push-constant struct MotionBlurPreprocessPushConstant contains support_fsr2 which is never varied at runtime (hardcoded to 1.0 in motion_blur.cpp); either remove support_fsr2 from MotionBlurPreprocessPushConstant and convert it to a shader specialization constant (update the shader and binding code accordingly) or, if you intend it to be dynamic later, leave the field but add a clear TODO comment next to support_fsr2 in MotionBlurPreprocessPushConstant and in the motion_blur.cpp code where it’s set to 1.0f explaining the intended future behavior so reviewers know it’s intentionally static for now.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@COPYRIGHT.txt`:
- Around line 201-208: The COPYRIGHT.txt currently labels the files with
"License: MIT" but the included text is the Expat (MIT/Expat) wording; update
the identifier to match the license text by replacing "License: MIT" with
"License: Expat" (or alternatively add a full MIT license block named "MIT" to
match the identifier). Locate the top-level license metadata in COPYRIGHT.txt
and change the License line for the listed shader files
(servers/rendering/renderer_rd/shaders/effects/motion_blur_*) to "Expat", and
ensure the license text block is labeled "Expat" so the identifier and text are
consistent.
In `@doc/classes/CameraAttributesPractical.xml`:
- Around line 47-48: Add a descriptive docstring for the member
"motion_blur_enabled" (member name="motion_blur_enabled",
setter="set_motion_blur_enabled", getter="is_motion_blur_enabled") inside the
CameraAttributesPractical XML entry: state that this flag enables/disables
motion blur, note that motion blur is supported only in the Forward+ renderer,
and reference the related motion blur intensity/multiplier controls (mention the
corresponding intensity/multiplier camera attributes so readers can find them);
keep the description concise and consistent with the style of the other motion
blur members.
- Line 44: The <member> tag for motion_blur_clamp_velocities_to_tile has extra
indentation; locate the tag with name="motion_blur_clamp_velocities_to_tile" and
change its leading whitespace to match sibling <member> entries (use the same
two-tab indentation as other members) so the XML formatting is consistent.
In `@doc/classes/ProjectSettings.xml`:
- Around line 2821-2822: Add a brief descriptive summary for the ProjectSettings
member rendering/camera/motion_blur/motion_blur_show_in_editor explaining what
the setting controls and the effect of toggling it (e.g., whether motion blur is
visible in the editor viewport, if it affects rendered output, and any scope
such as editor-only or both editor and runtime); update the <member> node for
rendering/camera/motion_blur/motion_blur_show_in_editor in ProjectSettings.xml
to include that description text so the setting is no longer undocumented.
- Around line 2806-2820: The documentation references to other motion blur
settings use shorthand [member] links that lack full setting paths; update the
links in the three members
(rendering/camera/motion_blur/motion_blur_framerate_mode,
rendering/camera/motion_blur/motion_blur_quality,
rendering/camera/motion_blur/motion_blur_reference_framerate) so they use the
full member path syntax (e.g. [member
rendering/camera/motion_blur/motion_blur_reference_framerate]) wherever a
related setting is mentioned, ensuring all doc links and tooltips resolve
correctly.
In `@doc/classes/RenderingServer.xml`:
- Around line 105-142: Add concise descriptions to each empty <description> for
the new motion blur API methods so generated docs are informative: for
camera_attributes_set_motion_blur explain parameters (enabled toggles motion
blur, intensity controls strength, clamp_velocities_to_tile limits velocity
influence, and the three multipliers/thresholds adjust how
object/movement/rotation velocities affect blur); for
camera_attributes_set_motion_blur_framerate_mode state what the mode enum does
and what reference_framerate is used for; for
camera_attributes_set_motion_blur_quality describe the quality enum effect on
result vs performance; for camera_attributes_set_motion_blur_show_in_editor note
it toggles editor preview; and for camera_attributes_set_motion_blur_tile_size
describe how tile size enum affects quality/performance. Use the method names
above to locate each <description>.
- Around line 5652-5664: Add descriptive documentation entries for the newly
introduced enum constants MOTION_BLUR_QUALITY_LOW, MOTION_BLUR_QUALITY_MEDIUM,
MOTION_BLUR_QUALITY_HIGH (enum: MotionBlurQuality) and
MOTION_BLUR_TILE_SIZE_SMALL, MOTION_BLUR_TILE_SIZE_MEDIUM,
MOTION_BLUR_TILE_SIZE_LARGE, MOTION_BLUR_TILE_SIZE_EXTRA_LARGE (enum:
MotionBlurTileSize) in the RenderingServer XML so the API reference is complete;
for each constant add a one-line description explaining what it controls (e.g.,
blur sample quality levels for MotionBlurQuality and tile size/resolution
trade-offs for MotionBlurTileSize), mention default or recommended use where
applicable, and ensure the descriptions follow the same wording/style and tag
format as surrounding constant entries in this file.
In `@scene/resources/camera_attributes.cpp`:
- Around line 150-229: The linker is failing because
CameraAttributesPractical::_update_motion_blur() is declared and called from
multiple setters (set_motion_blur_enabled, set_motion_blur_intensity,
set_motion_blur_clamp_velocities_to_tile,
set_motion_blur_object_velocity_multiplier,
set_motion_blur_movement_velocity_multiplier,
set_motion_blur_rotation_velocity_multiplier,
set_motion_blur_velocity_lower_threshold,
set_motion_blur_velocity_upper_threshold) but has no implementation; implement
_update_motion_blur() following the existing _update_dof_blur() pattern so it
calls RS::get_singleton()->camera_attributes_set_motion_blur(...) with get_rid()
and the nine motion blur fields (motion_blur_enabled, motion_blur_intensity,
motion_blur_clamp_velocities_to_tile, motion_blur_object_velocity_multiplier,
motion_blur_movement_velocity_multiplier,
motion_blur_rotation_velocity_multiplier, motion_blur_velocity_lower_threshold,
motion_blur_velocity_upper_threshold).
In `@scene/resources/camera_attributes.h`:
- Around line 113-129: Bind the new motion-blur setters/getters in
CameraAttributesPractical::_bind_methods(): call ClassDB::bind_method for each
new symbol (set_motion_blur_enabled, is_motion_blur_enabled,
set_motion_blur_intensity, get_motion_blur_intensity,
set_motion_blur_clamp_velocities_to_tile,
is_motion_blur_clamp_velocities_to_tile,
set_motion_blur_object_velocity_multiplier,
get_motion_blur_object_velocity_multiplier,
set_motion_blur_movement_velocity_multiplier,
get_motion_blur_movement_velocity_multiplier,
set_motion_blur_rotation_velocity_multiplier,
get_motion_blur_rotation_velocity_multiplier,
set_motion_blur_velocity_lower_threshold,
get_motion_blur_velocity_lower_threshold,
set_motion_blur_velocity_upper_threshold,
get_motion_blur_velocity_upper_threshold) and add corresponding ADD_PROPERTY
entries so each pair is exposed as a property in the inspector (use appropriate
PROPERTY_HINT/usage for booleans and floats). Ensure method names match the
declarations in camera_attributes.h and add properties with readable names and
getter/setter bindings.
In `@servers/rendering/renderer_rd/effects/motion_blur.cpp`:
- Around line 241-252: The switch handling motion blur framerate modes can
divide by zero; add defensive guards before any division using
reference_framerate or time_step: in the case for
MOTION_BLUR_FRAMERATE_MODE_CAPPED compute denom = time_step and alt_rate =
(reference_framerate > 0.f) ? 1.f / reference_framerate : 0.f and use
MIN(alt_rate, time_step) only if time_step > 0.f (otherwise skip scaling); in
MOTION_BLUR_FRAMERATE_MODE_FIXED ensure denom = reference_framerate * time_step
is > 0.f before doing intensity /= denom (otherwise skip or clamp denom to a
safe >0 value). Refer to
RSG::camera_attributes->camera_attributes_get_motion_blur_framerate_mode(), the
variables reference_framerate, time_step and intensity and keep this defensive
check in addition to fixing camera_attributes_set_motion_blur_framerate_mode().
- Around line 139-213: The "Motion blur" debug label opened with
RD::get_singleton()->draw_command_begin_label("Motion blur") is never closed;
add a matching RD::get_singleton()->draw_command_end_label() before the block
finishes (e.g. just before RD::get_singleton()->compute_list_end()) so the label
opened by draw_command_begin_label("Motion blur") is closed in the same scope
(ensure it pairs correctly with the outer label closed later in
motion_blur_compute).
- Around line 216-232: The motion blur textures are only created when
!p_render_buffers->has_texture(...) and never invalidated on viewport changes;
update motion_blur_compute to store and compare previous base_size, tiled_size
and view_count (e.g., keep previous_base_size, previous_tiled_size,
previous_view_count as members) and if any differ from current
base_size/tiled_size/view_count call
p_render_buffers->clear_context(RB_SCOPE_MOTION_BLUR) before recreating
textures; ensure you update these previous_* members after clearing/creating so
subsequent frames use the new sizes.
- Around line 154-156: Remove the unnecessary push-constant call for the
tile_max_x compute pass: delete the call to
RD::get_singleton()->compute_list_set_push_constant(compute_list, nullptr, 0)
inside the tile_max_x pass because that shader does not declare any push
constants (unlike the preprocess/blur shaders that use Params), and follow the
same pattern as tile_max_y and neighbor_max which omit push-constant calls when
not used.
In `@servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glsl`:
- Around line 58-75: The loop that samples velocities for TILE_SIZE can read
past the right edge when render width isn't divisible by TILE_SIZE; change the
for-loop in motion_blur_tile_max_x.glsl to iterate only over the remaining
pixels by computing a clamped iteration count (min(TILE_SIZE,
remaining_pixels_on_row)) before the loop and use that limit instead of
TILE_SIZE, so sampling via uvn + vec2(float(i) / render_size.x, 0) against
velocity_sampler cannot go out of bounds; update any references to TILE_SIZE in
that loop and leave max_velocity_length and max_velocity assignment logic
unchanged.
In `@servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glsl`:
- Around line 57-65: The loop unconditionally iterates TILE_SIZE which can
sample past the texture edge for border tiles; compute a clamped loop count
(e.g. int remaining = int(render_size.y) - int(floor(uvn.y * render_size.y));
int loopCount = max(0, min(TILE_SIZE, remaining))) and replace the for condition
with i < loopCount so the sampling of tile_max_x (and updates to
max_velocity_length / max_velocity) never reads out of bounds.
In `@servers/rendering/storage/camera_attributes_storage.cpp`:
- Around line 78-94: The camera_attributes_set_motion_blur setter
(RendererCameraAttributes::camera_attributes_set_motion_blur) currently accepts
p_velocity_lower_threshold and p_velocity_upper_threshold without validating
order; add validation before assigning to
cam_attributes->motion_blur_velocity_lower_threshold and
motion_blur_velocity_upper_threshold to ensure lower <= upper (either swap the
values if inverted, clamp the upper to be at least the lower, or reject with a
warning/error). Implement the chosen behavior and, if keeping runtime-only
fixes, emit a WARN_PRINT/WARN_PRINT_ONCE with context (function name and
offending values) when you adjust or reject inverted thresholds so callers can
notice unexpected inputs.
- Around line 58-61: The setter
RendererCameraAttributes::camera_attributes_set_motion_blur_framerate_mode must
validate p_reference_framerate before assigning to
motion_blur_reference_framerate to prevent division-by-zero; update the method
to check that p_reference_framerate is >= 1 (or clamp it to 1) and only then
assign motion_blur_reference_framerate and motion_blur_framerate_mode, otherwise
fallback to a safe default (e.g., 1) or return early; ensure you reference the
same members motion_blur_reference_framerate and motion_blur_framerate_mode in
the change so programmatic callers cannot set zero or negative values.
In `@servers/rendering/storage/camera_attributes_storage.h`:
- Around line 60-61: The two members motion_blur_velocity_lower_threshold and
motion_blur_velocity_upper_threshold currently both default to 0.0 which creates
a zero-width range and causes a divide-by-zero in the shader; fix this by either
(A) documenting the intended semantics with a comment on those symbols (e.g.,
“0.0 disables the threshold” and ensure the shader checks for the disable case)
or (B) changing the defaults to an asymmetric non-zero band (match the style of
the DoF defaults) so the shader’s sharp_step() never divides by zero; update
either the member declarations or add the explanatory comment next to
motion_blur_velocity_lower_threshold and motion_blur_velocity_upper_threshold
accordingly.
---
Outside diff comments:
In `@servers/rendering_server.cpp`:
- Around line 3190-3216: Add missing BIND_ENUM_CONSTANT entries for the
MotionBlurFramerateMode enum so script users can use named constants instead of
raw integers: locate _bind_methods() where
camera_attributes_set_motion_blur_framerate_mode is bound and append
BIND_ENUM_CONSTANT(MOTION_BLUR_FRAMERATE_MODE_NATIVE),
BIND_ENUM_CONSTANT(MOTION_BLUR_FRAMERATE_MODE_CAPPED), and
BIND_ENUM_CONSTANT(MOTION_BLUR_FRAMERATE_MODE_FIXED) (matching the enum declared
for MotionBlurFramerateMode) alongside the other motion blur enum bindings.
---
Nitpick comments:
In `@servers/rendering/renderer_rd/effects/motion_blur.cpp`:
- Around line 275-284: The TODO notes that per-velocity-type threshold settings
are missing: currently velocity_lower_threshold and velocity_upper_threshold are
written into motion_blur.preprocess_push_constant.* for rotation, movement and
object equally; update the implementation to read and assign distinct thresholds
for each velocity type (e.g., rotation_velocity_lower_threshold/upper,
movement_velocity_lower_threshold/upper, object_velocity_lower_threshold/upper)
by adding new getters in RSG::camera_attributes (or new config fields) such as
camera_attributes_get_motion_blur_rotation_lower_threshold,
camera_attributes_get_motion_blur_movement_lower_threshold,
camera_attributes_get_motion_blur_object_lower_threshold (and corresponding
*_upper_threshold) and then use those values instead of the shared
velocity_lower_threshold/velocity_upper_threshold when setting
motion_blur.preprocess_push_constant.
In `@servers/rendering/renderer_rd/effects/motion_blur.h`:
- Around line 67-82: The push-constant struct MotionBlurPreprocessPushConstant
contains support_fsr2 which is never varied at runtime (hardcoded to 1.0 in
motion_blur.cpp); either remove support_fsr2 from
MotionBlurPreprocessPushConstant and convert it to a shader specialization
constant (update the shader and binding code accordingly) or, if you intend it
to be dynamic later, leave the field but add a clear TODO comment next to
support_fsr2 in MotionBlurPreprocessPushConstant and in the motion_blur.cpp code
where it’s set to 1.0f explaining the intended future behavior so reviewers know
it’s intentionally static for now.
In `@servers/rendering/renderer_rd/renderer_scene_render_rd.cpp`:
- Around line 546-560: The motion-blur storage and editor-warning checks run
even when effects are disabled, causing noisy logs; wrap the logic that queries
RSG::camera_attributes and checks can_use_storage and editor hints inside the
can_use_effects guard (i.e., only evaluate using_motion_blur and adjust
using_motion_blur when can_use_effects is true) so that WARN_PRINT_ONCE and the
editor_hint check are skipped when can_use_effects is false, leaving the final
conditional that calls motion_blur->motion_blur_compute(rb,
p_render_data->camera_attributes, ...) unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glslis excluded by!**/*.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glslis excluded by!**/*.glsl
📒 Files selected for processing (20)
COPYRIGHT.txtdoc/classes/CameraAttributesPractical.xmldoc/classes/ProjectSettings.xmldoc/classes/RenderingServer.xmleditor/editor_node.cppscene/resources/camera_attributes.cppscene/resources/camera_attributes.hservers/rendering/renderer_rd/effects/motion_blur.cppservers/rendering/renderer_rd/effects/motion_blur.hservers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cppservers/rendering/renderer_rd/renderer_scene_render_rd.cppservers/rendering/renderer_rd/renderer_scene_render_rd.hservers/rendering/renderer_rd/shaders/effects/motion_blur_neighbor_max.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glslservers/rendering/rendering_server_default.hservers/rendering/storage/camera_attributes_storage.cppservers/rendering/storage/camera_attributes_storage.hservers/rendering_server.cppservers/rendering_server.h
Comment on lines
+201
to
+208
| Files: servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glsl | ||
| servers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glsl | ||
| servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glsl | ||
| servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glsl | ||
| servers/rendering/renderer_rd/shaders/effects/motion_blur_neighbor_max.glsl | ||
| Comment: sphynx-owner's motion blur effect | ||
| Copyright: 2025, sphynx-owner | ||
| License: MIT |
There was a problem hiding this comment.
License identifier mismatch: “MIT” not defined in file.
The file includes an “Expat” license text but no explicit “MIT” license block. In Debian copyright format, the license identifier should correspond to a license text in the file. Consider switching to “Expat” (MIT/Expat equivalent) or adding a “License: MIT” section.
✅ Suggested fix (align with existing Expat license text)
-Files: servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glsl
- servers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glsl
- servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glsl
- servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glsl
- servers/rendering/renderer_rd/shaders/effects/motion_blur_neighbor_max.glsl
-Comment: sphynx-owner's motion blur effect
-Copyright: 2025, sphynx-owner
-License: MIT
+Files: servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glsl
+ servers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glsl
+ servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glsl
+ servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glsl
+ servers/rendering/renderer_rd/shaders/effects/motion_blur_neighbor_max.glsl
+Comment: sphynx-owner's motion blur effect
+Copyright: 2025, sphynx-owner
+License: Expat📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Files: servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glsl | |
| servers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glsl | |
| servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glsl | |
| servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glsl | |
| servers/rendering/renderer_rd/shaders/effects/motion_blur_neighbor_max.glsl | |
| Comment: sphynx-owner's motion blur effect | |
| Copyright: 2025, sphynx-owner | |
| License: MIT | |
| Files: servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glsl | |
| servers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glsl | |
| servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glsl | |
| servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glsl | |
| servers/rendering/renderer_rd/shaders/effects/motion_blur_neighbor_max.glsl | |
| Comment: sphynx-owner's motion blur effect | |
| Copyright: 2025, sphynx-owner | |
| License: Expat |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@COPYRIGHT.txt` around lines 201 - 208, The COPYRIGHT.txt currently labels the
files with "License: MIT" but the included text is the Expat (MIT/Expat)
wording; update the identifier to match the license text by replacing "License:
MIT" with "License: Expat" (or alternatively add a full MIT license block named
"MIT" to match the identifier). Locate the top-level license metadata in
COPYRIGHT.txt and change the License line for the listed shader files
(servers/rendering/renderer_rd/shaders/effects/motion_blur_*) to "Expat", and
ensure the license text block is labeled "Expat" so the identifier and text are
consistent.
Comment on lines
+2821
to
+2822
| <member name="rendering/camera/motion_blur/motion_blur_show_in_editor" type="bool" setter="" getter="" default="true"> | ||
| </member> |
There was a problem hiding this comment.
Add a brief description for motion_blur_show_in_editor.
This new setting is undocumented, which makes it unclear what toggling it does.
📝 Suggested doc text
<member name="rendering/camera/motion_blur/motion_blur_show_in_editor" type="bool" setter="" getter="" default="true">
+ If [code]true[/code], motion blur settings are visible in the editor.
</member>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <member name="rendering/camera/motion_blur/motion_blur_show_in_editor" type="bool" setter="" getter="" default="true"> | |
| </member> | |
| <member name="rendering/camera/motion_blur/motion_blur_show_in_editor" type="bool" setter="" getter="" default="true"> | |
| If [code]true[/code], motion blur settings are visible in the editor. | |
| </member> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/classes/ProjectSettings.xml` around lines 2821 - 2822, Add a brief
descriptive summary for the ProjectSettings member
rendering/camera/motion_blur/motion_blur_show_in_editor explaining what the
setting controls and the effect of toggling it (e.g., whether motion blur is
visible in the editor viewport, if it affects rendered output, and any scope
such as editor-only or both editor and runtime); update the <member> node for
rendering/camera/motion_blur/motion_blur_show_in_editor in ProjectSettings.xml
to include that description text so the setting is no longer undocumented.
Comment on lines
+58
to
+75
| for (int i = 0; i < TILE_SIZE; i++) { | ||
| 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) | ||
| // then it will never go in front of other geometry, and can be skipped. | ||
| // TODO @sphynx-owner: enable when considering ignoring skybox for dominant velocity | ||
| // if(velocity_sample.w == (-1.0 / 0.0)) | ||
| // { | ||
| // continue; | ||
| // } | ||
|
|
||
| float current_velocity_length = dot(velocity_sample.xy, velocity_sample.xy); | ||
| if (current_velocity_length > max_velocity_length) { | ||
| max_velocity_length = current_velocity_length; | ||
| max_velocity = vec4(velocity_sample.xy, 0, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Clamp edge tiles to avoid out‑of‑bounds sampling.
When the render width isn’t divisible by TILE_SIZE, the loop can sample past the texture edge, skewing max velocity on border tiles. Clamp the loop length to the remaining pixels.
✅ Suggested fix (clamp loop length)
- for (int i = 0; i < TILE_SIZE; i++) {
+ int max_i = min(TILE_SIZE, render_size.x - global_uvi.x);
+ for (int i = 0; i < max_i; i++) {
vec2 current_uv = uvn + vec2(float(i) / render_size.x, 0);
vec4 velocity_sample = textureLod(velocity_sampler, current_uv, 0.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glsl`
around lines 58 - 75, The loop that samples velocities for TILE_SIZE can read
past the right edge when render width isn't divisible by TILE_SIZE; change the
for-loop in motion_blur_tile_max_x.glsl to iterate only over the remaining
pixels by computing a clamped iteration count (min(TILE_SIZE,
remaining_pixels_on_row)) before the loop and use that limit instead of
TILE_SIZE, so sampling via uvn + vec2(float(i) / render_size.x, 0) against
velocity_sampler cannot go out of bounds; update any references to TILE_SIZE in
that loop and leave max_velocity_length and max_velocity assignment logic
unchanged.
Comment on lines
+57
to
+65
| for (int i = 0; i < TILE_SIZE; i++) { | ||
| vec2 current_uv = uvn + vec2(0, float(i) / render_size.y); | ||
| vec2 velocity_sample = textureLod(tile_max_x, current_uv, 0.0).xy; | ||
| float current_velocity_length = dot(velocity_sample, velocity_sample); | ||
| if (current_velocity_length > max_velocity_length) { | ||
| max_velocity_length = current_velocity_length; | ||
| max_velocity = vec4(velocity_sample, 0, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Clamp edge tiles to avoid out‑of‑bounds sampling.
Border tiles can sample beyond the texture when the height isn’t divisible by TILE_SIZE, which can skew max velocity.
✅ Suggested fix (clamp loop length)
- for (int i = 0; i < TILE_SIZE; i++) {
+ int max_i = min(TILE_SIZE, render_size.y - global_uvi.y);
+ for (int i = 0; i < max_i; i++) {
vec2 current_uv = uvn + vec2(0, float(i) / render_size.y);
vec2 velocity_sample = textureLod(tile_max_x, current_uv, 0.0).xy;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (int i = 0; i < TILE_SIZE; i++) { | |
| vec2 current_uv = uvn + vec2(0, float(i) / render_size.y); | |
| vec2 velocity_sample = textureLod(tile_max_x, current_uv, 0.0).xy; | |
| float current_velocity_length = dot(velocity_sample, velocity_sample); | |
| if (current_velocity_length > max_velocity_length) { | |
| max_velocity_length = current_velocity_length; | |
| max_velocity = vec4(velocity_sample, 0, 0); | |
| } | |
| } | |
| int max_i = min(TILE_SIZE, render_size.y - global_uvi.y); | |
| for (int i = 0; i < max_i; i++) { | |
| vec2 current_uv = uvn + vec2(0, float(i) / render_size.y); | |
| vec2 velocity_sample = textureLod(tile_max_x, current_uv, 0.0).xy; | |
| float current_velocity_length = dot(velocity_sample, velocity_sample); | |
| if (current_velocity_length > max_velocity_length) { | |
| max_velocity_length = current_velocity_length; | |
| max_velocity = vec4(velocity_sample, 0, 0); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glsl`
around lines 57 - 65, The loop unconditionally iterates TILE_SIZE which can
sample past the texture edge for border tiles; compute a clamped loop count
(e.g. int remaining = int(render_size.y) - int(floor(uvn.y * render_size.y));
int loopCount = max(0, min(TILE_SIZE, remaining))) and replace the for condition
with i < loopCount so the sampling of tile_max_x (and updates to
max_velocity_length / max_velocity) never reads out of bounds.
Comment on lines
+58
to
+61
| void RendererCameraAttributes::camera_attributes_set_motion_blur_framerate_mode(RenderingServer::MotionBlurFramerateMode p_mode, int p_reference_framerate) { | ||
| motion_blur_framerate_mode = p_mode; | ||
| motion_blur_reference_framerate = p_reference_framerate; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for reference_framerate usage in motion_blur.cpp and related files
echo "=== Searching for reference_framerate usage ==="
rg -n "reference_framerate" --type=cpp -C 3
echo ""
echo "=== Checking motion_blur.cpp specifically ==="
fd -e cpp -e h | xargs grep -l "motion_blur" | head -10Repository: Redot-Engine/redot-engine
Length of output: 11078
🏁 Script executed:
#!/bin/bash
# Check camera_attributes.cpp for upstream validation
echo "=== Checking for validation in camera_attributes.cpp ==="
fd -e cpp -e h -p "*camera_attributes*" | xargs cat 2>/dev/null | head -100Repository: Redot-Engine/redot-engine
Length of output: 441
🏁 Script executed:
#!/bin/bash
# Search for where camera_attributes_set_motion_blur_framerate_mode is called
echo "=== Finding callers of camera_attributes_set_motion_blur_framerate_mode ==="
rg -n "camera_attributes_set_motion_blur_framerate_mode" --type=cpp -C 5Repository: Redot-Engine/redot-engine
Length of output: 9805
Add validation to prevent division by zero with p_reference_framerate.
While the global setting motion_blur_reference_framerate has a PROPERTY_HINT_RANGE constraint (minimum 1), the setter function itself has no validation. This leaves it vulnerable to division-by-zero in motion_blur.cpp (lines 246 and 250) if called programmatically with a zero or negative value.
Proposed fix
void RendererCameraAttributes::camera_attributes_set_motion_blur_framerate_mode(RenderingServer::MotionBlurFramerateMode p_mode, int p_reference_framerate) {
motion_blur_framerate_mode = p_mode;
- motion_blur_reference_framerate = p_reference_framerate;
+ motion_blur_reference_framerate = MAX(1, p_reference_framerate);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/rendering/storage/camera_attributes_storage.cpp` around lines 58 -
61, The setter
RendererCameraAttributes::camera_attributes_set_motion_blur_framerate_mode must
validate p_reference_framerate before assigning to
motion_blur_reference_framerate to prevent division-by-zero; update the method
to check that p_reference_framerate is >= 1 (or clamp it to 1) and only then
assign motion_blur_reference_framerate and motion_blur_framerate_mode, otherwise
fallback to a safe default (e.g., 1) or return early; ensure you reference the
same members motion_blur_reference_framerate and motion_blur_framerate_mode in
the change so programmatic callers cannot set zero or negative values.
Comment on lines
+78
to
+94
| void RendererCameraAttributes::camera_attributes_set_motion_blur(RID p_camera_attributes, bool p_enable, float p_intensity, bool p_clamp_velocities_to_tile, float p_object_velocity_multiplier, float p_movement_velocity_multiplier, float p_rotation_velocity_multiplier, float p_velocity_lower_threshold, float p_velocity_upper_threshold) { | ||
| CameraAttributes *cam_attributes = camera_attributes_owner.get_or_null(p_camera_attributes); | ||
| ERR_FAIL_NULL(cam_attributes); | ||
| #ifdef DEBUG_ENABLED | ||
| if ((OS::get_singleton()->get_current_rendering_method() == "gl_compatibility" || OS::get_singleton()->get_current_rendering_method() == "mobile") && p_enable) { | ||
| WARN_PRINT_ONCE_ED("Motion blur is only available when using the Forward+ renderer."); | ||
| } | ||
| #endif | ||
| cam_attributes->motion_blur_enabled = p_enable; | ||
| cam_attributes->motion_blur_intensity = p_intensity; | ||
| cam_attributes->motion_blur_clamp_velocities_to_tile = p_clamp_velocities_to_tile; | ||
| cam_attributes->motion_blur_object_velocity_multiplier = p_object_velocity_multiplier; | ||
| cam_attributes->motion_blur_movement_velocity_multiplier = p_movement_velocity_multiplier; | ||
| cam_attributes->motion_blur_rotation_velocity_multiplier = p_rotation_velocity_multiplier; | ||
| cam_attributes->motion_blur_velocity_lower_threshold = p_velocity_lower_threshold; | ||
| cam_attributes->motion_blur_velocity_upper_threshold = p_velocity_upper_threshold; | ||
| } |
There was a problem hiding this comment.
No validation on velocity threshold ordering.
If p_velocity_upper_threshold < p_velocity_lower_threshold, the transition range between non-blurred and fully-blurred motion would be inverted, likely producing unexpected visual artifacts. Consider adding a guard or at least a warning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/rendering/storage/camera_attributes_storage.cpp` around lines 78 -
94, The camera_attributes_set_motion_blur setter
(RendererCameraAttributes::camera_attributes_set_motion_blur) currently accepts
p_velocity_lower_threshold and p_velocity_upper_threshold without validating
order; add validation before assigning to
cam_attributes->motion_blur_velocity_lower_threshold and
motion_blur_velocity_upper_threshold to ensure lower <= upper (either swap the
values if inverted, clamp the upper to be at least the lower, or reject with a
warning/error). Implement the chosen behavior and, if keeping runtime-only
fixes, emit a WARN_PRINT/WARN_PRINT_ONCE with context (function name and
offending values) when you adjust or reject inverted thresholds so callers can
notice unexpected inputs.
Comment on lines
+60
to
+61
| float motion_blur_velocity_lower_threshold = 0.0; | ||
| float motion_blur_velocity_upper_threshold = 0.0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "velocity_lower_threshold|velocity_upper_threshold|lower_threshold|upper_threshold" --type=cpp --type=glsl -C 3Repository: Redot-Engine/redot-engine
Length of output: 97
🏁 Script executed:
# Also search for the CameraAttributes struct definition and the file in question
fd "camera_attributes_storage.h" -x cat -n {}Repository: Redot-Engine/redot-engine
Length of output: 10855
🏁 Script executed:
# Search for motion blur shader files and their usage of velocity
fd -e glsl -e frag -e vert | xargs rg -l "motion.blur|velocity" 2>/dev/null | head -20Repository: Redot-Engine/redot-engine
Length of output: 675
🏁 Script executed:
# Search for calls to camera_attributes_uses_motion_blur and context
rg -n "camera_attributes_uses_motion_blur|set_motion_blur" --type=cpp -C 5Repository: Redot-Engine/redot-engine
Length of output: 27595
🏁 Script executed:
fd -e glsl -path "*motion_blur*" | head -10Repository: Redot-Engine/redot-engine
Length of output: 237
🏁 Script executed:
# Read the motion blur preprocess and blur shaders to understand threshold usage
find . -name "motion_blur_*.glsl" -type f | xargs wc -l | sort -nRepository: Redot-Engine/redot-engine
Length of output: 238
🏁 Script executed:
# Check motion_blur_preprocess.glsl for threshold usage
cat servers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glslRepository: Redot-Engine/redot-engine
Length of output: 7838
🏁 Script executed:
# Check motion_blur_blur.glsl for threshold usage
cat servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glslRepository: Redot-Engine/redot-engine
Length of output: 9088
🏁 Script executed:
# Search for any inline comments near the threshold declarations
grep -B 5 -A 5 "motion_blur_velocity.*threshold" servers/rendering/storage/camera_attributes_storage.hRepository: Redot-Engine/redot-engine
Length of output: 1471
Add clarifying documentation for motion blur velocity thresholds or revise their defaults.
The thresholds motion_blur_velocity_lower_threshold and motion_blur_velocity_upper_threshold both default to 0.0, which creates a zero-width band. The shader's sharp_step() function divides by (upper - lower), making a [0, 0] range mathematically invalid and producing undefined behavior. Either document the intended semantics (e.g., "0.0 disables the threshold") with a comment, or use asymmetric non-zero defaults like the DoF parameters to ensure valid shader behavior out-of-the-box.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@servers/rendering/storage/camera_attributes_storage.h` around lines 60 - 61,
The two members motion_blur_velocity_lower_threshold and
motion_blur_velocity_upper_threshold currently both default to 0.0 which creates
a zero-width range and causes a divide-by-zero in the shader; fix this by either
(A) documenting the intended semantics with a comment on those symbols (e.g.,
“0.0 disables the threshold” and ensure the shader checks for the disable case)
or (B) changing the defaults to an asymmetric non-zero band (match the style of
the DoF defaults) so the shader’s sharp_step() never divides by zero; update
either the member declarations or add the explanatory comment next to
motion_blur_velocity_lower_threshold and motion_blur_velocity_upper_threshold
accordingly.
Member
Author
|
Will not be applying any of CodeRabbit's suggestions as it is still a WIP |
Contributor
All good! I have been following your PR upstream, and have been seeing lots of progress and changes over the last few weeks, so I understand this isnt ready for merge or for review on the Redot side yet. I'll go ahead and mark this PR as a draft for now till you have things finalized, which should shut Code Rabbit up for the time being. |
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 <sphynx-owner@users.noreply.github.com>
Shakai-Dev
force-pushed
the
motion-blur
branch
4 times, most recently
from
344377c to
11a0881
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/config/engine.cpp`:
- Around line 131-133: The new divisor-facing API in
Engine::get_effective_time_scale() can return 0.0 when frozen, which breaks the
motion blur path that divides by it. Update Engine::get_effective_time_scale()
and/or the caller in motion_blur.cpp so the frozen case never reaches a
divide-by-zero path: either make the accessor return a non-zero value suitable
for divisor use, or add an explicit frozen-state branch before using
Engine::get_singleton()->get_effective_time_scale() in the motion blur intensity
calculation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23a684d1-a656-4ae9-a4d0-3d370971e72b
📒 Files selected for processing (3)
COPYRIGHT.txtcore/config/engine.cppcore/config/engine.h
🚧 Files skipped from review as they are similar to previous changes (1)
- COPYRIGHT.txt
Comment on lines
+131
to
+133
| double Engine::get_effective_time_scale() const { | ||
| return freeze_time_scale ? 0.0 : _time_scale; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid returning zero from this new divisor-facing API.
Line 132 returns 0.0 while frozen, but the new motion-blur path immediately divides by Engine::get_singleton()->get_effective_time_scale() in servers/rendering/renderer_rd/effects/motion_blur.cpp:248-249. Suspending the scene therefore reaches a p_time_step / 0 path and feeds inf/NaN into the blur intensity math. Either keep this accessor non-zero for divisor use, or make the caller branch on the frozen case before dividing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/config/engine.cpp` around lines 131 - 133, The new divisor-facing API in
Engine::get_effective_time_scale() can return 0.0 when frozen, which breaks the
motion blur path that divides by it. Update Engine::get_effective_time_scale()
and/or the caller in motion_blur.cpp so the frozen case never reaches a
divide-by-zero path: either make the accessor return a non-zero value suitable
for divisor use, or add an explicit frozen-state branch before using
Engine::get_singleton()->get_effective_time_scale() in the motion blur intensity
calculation.
Member
Author
There was a problem hiding this comment.
What would happen if it's left & it returns zero? Would the engine pause/crash? Anyways, this is from Godot & no issues like this have been reported there.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate uses of get_effective_time_scale =="
rg -n -C3 '\bget_effective_time_scale\s*\(' .
echo
echo "== Outline motion_blur.cpp =="
ast-grep outline servers/rendering/renderer_rd/effects/motion_blur.cpp --view expanded || true
echo
echo "== Show motion_blur.cpp around the cited division =="
sed -n '220,280p' servers/rendering/renderer_rd/effects/motion_blur.cpp
echo
echo "== Show any nearby guards related to time_step / freeze / motion blur enablement =="
rg -n -C4 'freeze_time_scale|time_step|motion_blur_intensity|effective_time_scale|camera_attributes_uses_motion_blur' \
servers/rendering/renderer_rd/effects/motion_blur.cpp \
servers/rendering/renderer_rd/renderer_scene_render_rd.cpp \
servers/rendering/storage/camera_attributes_storage.h \
servers/rendering/storage/camera_attributes_storage.cpp \
core/config/engine.cpp \
core/config/engine.hLength of output: 24295
@Shakai-Dev I checked the current branch, and the zero path is still reachable:
core/config/engine.cppLine 132 returns0.0when frozen.servers/rendering/renderer_rd/effects/motion_blur.cppLine 249 immediately doesp_time_step / time_scale.
So if it’s left as-is, the most likely outcome is not a full engine crash. On most platforms, floating-point division by zero here produces inf/NaN, not a hard exception. The risk is that those values then flow into the motion blur intensity math on Line 255 and Line 259 and from there into shader push constants, which can lead to broken/unstable blur output rather than a pause or crash.
So the practical answer is:
- Probably no crash
- Possible visual corruption / invalid blur values
- Still a real bug path, even if it hasn’t been reported upstream
It being inherited from Godot is useful context, but based on the current code, I don’t think that makes this specific concern invalid. It may just be an edge case that isn’t commonly hit.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/classes/ProjectSettings.xml`:
- Around line 2806-2827: Document that these motion blur project settings are
Forward+ only, since the renderer implementation does not apply them on
Mobile/Compatibility. Update the descriptions for
rendering/camera/motion_blur/motion_blur_framerate_mode,
rendering/camera/motion_blur/motion_blur_quality,
rendering/camera/motion_blur/motion_blur_reference_framerate, and
rendering/camera/motion_blur/motion_blur_tile_size in ProjectSettings.xml to
explicitly mention the Forward+ limitation and that they have no effect on
unsupported renderers.
In `@doc/classes/RenderingServer.xml`:
- Around line 135-140: The description for
camera_attributes_set_motion_blur_show_in_editor contains duplicated words that
break the generated docs; update the text to remove the repeated "is" and "be"
so the sentence reads cleanly, and make the same wording correction in the
mirrored EditorSettings entry so both doc definitions stay consistent.
In `@scene/resources/camera_attributes.cpp`:
- Around line 426-429: The three motion blur velocity multiplier properties in
CameraAttributesPractical are incorrectly capped to 1.0 in the Inspector even
though the setters only enforce a minimum of 0 and the render path uses the
stored values directly. Update the PropertyInfo hints for
motion_blur_velocity_multiplier_object,
motion_blur_velocity_multiplier_camera_movement, and
motion_blur_velocity_multiplier_camera_rotation in
CameraAttributes::add_properties so the editor allows values above 1.0 while
keeping the existing non-negative constraint.
In `@servers/rendering_server.h`:
- Line 1472: Validate p_reference_framerate inside
camera_attributes_set_motion_blur_framerate_mode before storing it, since this
script-exposed setter can currently accept 0 and pass an invalid value into the
render path. Update the MotionBlurFramerateMode handling in the
camera_attributes_set_motion_blur_framerate_mode implementation to clamp or
reject non-positive reference framerates, and ensure the stored value used by
the RD effect is always safe for capped/fixed modes.
In `@servers/rendering/renderer_rd/effects/motion_blur.cpp`:
- Around line 246-260: Guard the `time_scale` normalization in `motion_blur.cpp`
before computing `time_step` in the motion blur setup.
`Engine::get_singleton()->get_effective_time_scale()` can be 0.0 when time is
frozen, so update the logic around
`camera_attributes_get_motion_blur_framerate_mode()` to skip the division or use
a safe fallback when `time_scale` or the resulting frame time is zero. Make sure
`intensity` and `time_step` stay finite before they are used by the
push-constant paths.
In
`@servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp`:
- Around line 1761-1768: In
`RendererSceneRenderRD::_render_buffers_post_process_and_tonemap()`, the
`motion_vectors_required` decision is still tied directly to `using_motion_blur`
even though motion blur is later gated off when storage support is unavailable.
Update the motion-vector/MSAA velocity resolve gating so it follows the same
motion-blur capability check used for `using_motion_blur`, while still allowing
`using_debug_mvs` to force it on. Make the condition consistent with the
motion-blur support gate to avoid allocating or resolving velocity data when
motion blur cannot run.
In `@servers/rendering/renderer_rd/renderer_scene_render_rd.cpp`:
- Around line 1702-1705: The motion blur tile-size setting is only updated on
RSG::camera_attributes, but RendererRD::MotionBlur still keeps the original
constructor-captured value, so runtime changes leave the blur pipeline stale.
Update the motion blur handling in RendererSceneRenderRD/RendererRD::MotionBlur
so changes to the tile-size setting either rebuild the MotionBlur instance or
add a setter and reconfigure its internal state whenever the public setting
changes. Use the existing motion blur setup code path and the
camera_attributes_set_motion_blur_tile_size call as the integration point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8b9b128-7707-419d-b6e7-f9175d461e13
⛔ Files ignored due to path filters (5)
servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glslis excluded by!**/*.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_neighbor_max.glslis excluded by!**/*.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glslis excluded by!**/*.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glslis excluded by!**/*.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glslis excluded by!**/*.glsl
📒 Files selected for processing (23)
COPYRIGHT.txtcore/config/engine.cppcore/config/engine.hdoc/classes/CameraAttributesPractical.xmldoc/classes/EditorSettings.xmldoc/classes/ProjectSettings.xmldoc/classes/RenderingServer.xmleditor/editor_node.cppeditor/scene/3d/node_3d_editor_plugin.cppeditor/settings/editor_settings.cppscene/resources/camera_attributes.cppscene/resources/camera_attributes.hservers/rendering/renderer_rd/effects/motion_blur.cppservers/rendering/renderer_rd/effects/motion_blur.hservers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cppservers/rendering/renderer_rd/pipeline_deferred_rd.hservers/rendering/renderer_rd/renderer_scene_render_rd.cppservers/rendering/renderer_rd/renderer_scene_render_rd.hservers/rendering/rendering_server_default.hservers/rendering/storage/camera_attributes_storage.cppservers/rendering/storage/camera_attributes_storage.hservers/rendering_server.cppservers/rendering_server.h
Comment on lines
+2806
to
+2827
| <member name="rendering/camera/motion_blur/motion_blur_framerate_mode" type="int" setter="" getter="" default="1"> | ||
| Defines the framerate-based behavior of the motion blur. Uses [member rendering/camera/motion_blur/motion_blur_reference_framerate] as the reference framerate for the different modes. | ||
| - [code]MOTION_BLUR_FRAMERATE_MODE_NATIVE[/code] applies the blur based on the game's framerate as is, and does not use the reference framerate. | ||
| - [code]MOTION_BLUR_FRAMERATE_MODE_CAPPED[/code] applies the blur based on the game's framerate, but below a certain framerate (which means larger time gap and thus more intense blurring) it will cap the blur to emulate the blur that will be generated at the reference framerate. This is the default value, and it means that if the game lags, the blur will be kept under control. | ||
| - [code]MOTION_BLUR_FRAMERATE_MODE_FIXED[/code] enforces the reference framerate blur regardless of the game's framerate. This can lead to overblurring when the game's framerate is higher than the reference framerate. | ||
| </member> | ||
| <member name="rendering/camera/motion_blur/motion_blur_quality" type="int" setter="" getter="" default="1"> | ||
| Controls the quality of the motion blur. The motion blur uses noise to smoothen the transition between samples to prevent banding, but it can only do so much. Larger sample counts will mitigate the perceivable noise, at the cost of performance. The sample counts are as follows: | ||
| - [b]Low:[/b] 4 samples | ||
| - [b]Medium:[/b] 8 samples | ||
| - [b]High:[/b] 16 samples | ||
| </member> | ||
| <member name="rendering/camera/motion_blur/motion_blur_reference_framerate" type="int" setter="" getter="" default="30"> | ||
| Defines a framerate to be used as a reference by [member rendering/camera/motion_blur/motion_blur_framerate_mode]. | ||
| </member> | ||
| <member name="rendering/camera/motion_blur/motion_blur_tile_size" type="int" setter="" getter="" default="1"> | ||
| Defines, in pixels, the size of the velocity dilation tiles to be used by the motion blur. These tiles collect dominant velocities from neighboring tiles so that fast moving objects can be blurred beyond their original silhouettes. Changing the tile size has little performance cost, rather the cost and effect come in the form of detail vs range. Large tile sizes can blur objects further, but will do so in less detail. The sizes are: | ||
| - [b]Small:[/b] 20 pixels | ||
| - [b]Medium:[/b] 40 pixels | ||
| - [b]Large:[/b] 60 pixels | ||
| - [b]Extra Large:[/b] 80 pixels | ||
| </member> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the Forward+ limitation for these settings.
The renderer-side implementation warns that motion blur is only available on Forward+, but none of these new project-setting entries mention that. On Mobile/Compatibility this reads like supported functionality even though it won't apply.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/classes/ProjectSettings.xml` around lines 2806 - 2827, Document that
these motion blur project settings are Forward+ only, since the renderer
implementation does not apply them on Mobile/Compatibility. Update the
descriptions for rendering/camera/motion_blur/motion_blur_framerate_mode,
rendering/camera/motion_blur/motion_blur_quality,
rendering/camera/motion_blur/motion_blur_reference_framerate, and
rendering/camera/motion_blur/motion_blur_tile_size in ProjectSettings.xml to
explicitly mention the Forward+ limitation and that they have no effect on
unsupported renderers.
Comment on lines
+135
to
+140
| <method name="camera_attributes_set_motion_blur_show_in_editor"> | ||
| <return type="void" /> | ||
| <param index="0" name="enabled" type="bool" /> | ||
| <description> | ||
| If [code]true[/code], and motion blur is enabled,, it will be be applied in the editor scene view as well. When [code]false[/code], the motion blur will only be applied at runtime. See also [member EditorSettings.editors/3d/viewport_visuals/show_motion_blur_in_editor] | ||
| </description> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the duplicated words in this description.
This currently renders a broken sentence in the generated docs, and the same wording is mirrored in doc/classes/EditorSettings.xml.
Suggested wording
- If [code]true[/code], and motion blur is enabled,, it will be be applied in the editor scene view as well. When [code]false[/code], the motion blur will only be applied at runtime. See also [member EditorSettings.editors/3d/viewport_visuals/show_motion_blur_in_editor]
+ If [code]true[/code] and motion blur is enabled, it will be applied in the editor scene view as well. When [code]false[/code], motion blur will only be applied at runtime. See also [member EditorSettings.editors/3d/viewport_visuals/show_motion_blur_in_editor].📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <method name="camera_attributes_set_motion_blur_show_in_editor"> | |
| <return type="void" /> | |
| <param index="0" name="enabled" type="bool" /> | |
| <description> | |
| If [code]true[/code], and motion blur is enabled,, it will be be applied in the editor scene view as well. When [code]false[/code], the motion blur will only be applied at runtime. See also [member EditorSettings.editors/3d/viewport_visuals/show_motion_blur_in_editor] | |
| </description> | |
| <method name="camera_attributes_set_motion_blur_show_in_editor"> | |
| <return type="void" /> | |
| <param index="0" name="enabled" type="bool" /> | |
| <description> | |
| If [code]true[/code] and motion blur is enabled, it will be applied in the editor scene view as well. When [code]false[/code], motion blur will only be applied at runtime. See also [member EditorSettings.editors/3d/viewport_visuals/show_motion_blur_in_editor]. | |
| </description> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/classes/RenderingServer.xml` around lines 135 - 140, The description for
camera_attributes_set_motion_blur_show_in_editor contains duplicated words that
break the generated docs; update the text to remove the repeated "is" and "be"
so the sentence reads cleanly, and make the same wording correction in the
mirrored EditorSettings entry so both doc definitions stay consistent.
Comment on lines
+426
to
+429
| ADD_SUBGROUP("Velocity Multipliers", "motion_blur_velocity_multiplier_"); | ||
| ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_object", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_motion_blur_object_velocity_multiplier", "get_motion_blur_object_velocity_multiplier"); | ||
| ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_movement", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_motion_blur_movement_velocity_multiplier", "get_motion_blur_movement_velocity_multiplier"); | ||
| ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_rotation", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_motion_blur_rotation_velocity_multiplier", "get_motion_blur_rotation_velocity_multiplier"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't cap the velocity multipliers at 1.0 in the Inspector.
These three setters only clamp to >= 0, and the RD path consumes the stored multipliers verbatim, but the property hints hard-limit editor users to 0..1. Right now CameraAttributesPractical resources created in the Inspector are less capable than script/RenderingServer callers.
Suggested diff
- ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_object", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_motion_blur_object_velocity_multiplier", "get_motion_blur_object_velocity_multiplier");
- ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_movement", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_motion_blur_movement_velocity_multiplier", "get_motion_blur_movement_velocity_multiplier");
- ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_rotation", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_motion_blur_rotation_velocity_multiplier", "get_motion_blur_rotation_velocity_multiplier");
+ ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_object", PROPERTY_HINT_RANGE, "0.0,1.0,0.01,or_greater"), "set_motion_blur_object_velocity_multiplier", "get_motion_blur_object_velocity_multiplier");
+ ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_movement", PROPERTY_HINT_RANGE, "0.0,1.0,0.01,or_greater"), "set_motion_blur_movement_velocity_multiplier", "get_motion_blur_movement_velocity_multiplier");
+ ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_rotation", PROPERTY_HINT_RANGE, "0.0,1.0,0.01,or_greater"), "set_motion_blur_rotation_velocity_multiplier", "get_motion_blur_rotation_velocity_multiplier");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ADD_SUBGROUP("Velocity Multipliers", "motion_blur_velocity_multiplier_"); | |
| ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_object", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_motion_blur_object_velocity_multiplier", "get_motion_blur_object_velocity_multiplier"); | |
| ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_movement", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_motion_blur_movement_velocity_multiplier", "get_motion_blur_movement_velocity_multiplier"); | |
| ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_rotation", PROPERTY_HINT_RANGE, "0.0,1.0,0.01"), "set_motion_blur_rotation_velocity_multiplier", "get_motion_blur_rotation_velocity_multiplier"); | |
| ADD_SUBGROUP("Velocity Multipliers", "motion_blur_velocity_multiplier_"); | |
| ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_object", PROPERTY_HINT_RANGE, "0.0,1.0,0.01,or_greater"), "set_motion_blur_object_velocity_multiplier", "get_motion_blur_object_velocity_multiplier"); | |
| ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_movement", PROPERTY_HINT_RANGE, "0.0,1.0,0.01,or_greater"), "set_motion_blur_movement_velocity_multiplier", "get_motion_blur_movement_velocity_multiplier"); | |
| ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_velocity_multiplier_camera_rotation", PROPERTY_HINT_RANGE, "0.0,1.0,0.01,or_greater"), "set_motion_blur_rotation_velocity_multiplier", "get_motion_blur_rotation_velocity_multiplier"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scene/resources/camera_attributes.cpp` around lines 426 - 429, The three
motion blur velocity multiplier properties in CameraAttributesPractical are
incorrectly capped to 1.0 in the Inspector even though the setters only enforce
a minimum of 0 and the render path uses the stored values directly. Update the
PropertyInfo hints for motion_blur_velocity_multiplier_object,
motion_blur_velocity_multiplier_camera_movement, and
motion_blur_velocity_multiplier_camera_rotation in
CameraAttributes::add_properties so the editor allows values above 1.0 while
keeping the existing non-negative constraint.
| virtual void camera_attributes_set_motion_blur(RID p_camera_attributes, bool p_enable, float p_intensity, bool p_clamp_velocities_to_tile, float p_object_velocity_multiplier, float p_movement_velocity_multiplier, float p_rotation_velocity_multiplier, float p_velocity_lower_threshold, float p_velocity_upper_threshold) = 0; | ||
|
|
||
| virtual void camera_attributes_set_motion_blur_show_in_editor(bool p_enabled) = 0; | ||
| virtual void camera_attributes_set_motion_blur_framerate_mode(MotionBlurFramerateMode p_mode, int p_reference_framerate) = 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate reference_framerate in the public motion-blur setter.
camera_attributes_set_motion_blur_framerate_mode() is script-exposed, but the storage-side implementation currently stores p_reference_framerate unchecked while the RD effect later divides by it in capped/fixed mode. Passing 0 here will feed invalid blur intensity into the render path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@servers/rendering_server.h` at line 1472, Validate p_reference_framerate
inside camera_attributes_set_motion_blur_framerate_mode before storing it, since
this script-exposed setter can currently accept 0 and pass an invalid value into
the render path. Update the MotionBlurFramerateMode handling in the
camera_attributes_set_motion_blur_framerate_mode implementation to clamp or
reject non-positive reference framerates, and ensure the stored value used by
the RD effect is always safe for capped/fixed modes.
Comment on lines
+246
to
+260
| float intensity = RSG::camera_attributes->camera_attributes_get_motion_blur_intensity(p_camera_attributes); | ||
| int reference_framerate = RSG::camera_attributes->camera_attributes_get_motion_blur_reference_framerate(); | ||
| const double time_scale = Engine::get_singleton()->get_effective_time_scale(); | ||
| float time_step = p_time_step / (float)time_scale; | ||
| switch (RSG::camera_attributes->camera_attributes_get_motion_blur_framerate_mode()) { | ||
| case RS::MOTION_BLUR_FRAMERATE_MODE_NATIVE: | ||
| // Use raw intensity, ignore frame time | ||
| break; | ||
| case RS::MOTION_BLUR_FRAMERATE_MODE_CAPPED: | ||
| intensity *= MIN(1.f / reference_framerate, time_step) / time_step; | ||
| break; | ||
| case RS::MOTION_BLUR_FRAMERATE_MODE_FIXED: | ||
| // Scale intensity by frame time | ||
| intensity /= reference_framerate * time_step; | ||
| break; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard frozen time before normalizing p_time_step.
Engine::get_effective_time_scale() returns 0.0 when time is frozen, so Line 249 can divide by zero before the framerate-mode switch even runs and then propagate inf/nan into both motion-blur push-constant blocks.
Suggested diff
float intensity = RSG::camera_attributes->camera_attributes_get_motion_blur_intensity(p_camera_attributes);
int reference_framerate = RSG::camera_attributes->camera_attributes_get_motion_blur_reference_framerate();
const double time_scale = Engine::get_singleton()->get_effective_time_scale();
- float time_step = p_time_step / (float)time_scale;
+ float time_step = 0.0f;
+ if (time_scale > 0.0 && p_time_step > 0.0f) {
+ time_step = p_time_step / (float)time_scale;
+ }
switch (RSG::camera_attributes->camera_attributes_get_motion_blur_framerate_mode()) {
case RS::MOTION_BLUR_FRAMERATE_MODE_NATIVE:
// Use raw intensity, ignore frame time
break;
case RS::MOTION_BLUR_FRAMERATE_MODE_CAPPED:
- intensity *= MIN(1.f / reference_framerate, time_step) / time_step;
+ if (time_step > 0.0f && reference_framerate > 0) {
+ intensity *= MIN(1.f / reference_framerate, time_step) / time_step;
+ } else {
+ intensity = 0.0f;
+ }
break;
case RS::MOTION_BLUR_FRAMERATE_MODE_FIXED:
// Scale intensity by frame time
- intensity /= reference_framerate * time_step;
+ if (time_step > 0.0f && reference_framerate > 0) {
+ intensity /= reference_framerate * time_step;
+ } else {
+ intensity = 0.0f;
+ }
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| float intensity = RSG::camera_attributes->camera_attributes_get_motion_blur_intensity(p_camera_attributes); | |
| int reference_framerate = RSG::camera_attributes->camera_attributes_get_motion_blur_reference_framerate(); | |
| const double time_scale = Engine::get_singleton()->get_effective_time_scale(); | |
| float time_step = p_time_step / (float)time_scale; | |
| switch (RSG::camera_attributes->camera_attributes_get_motion_blur_framerate_mode()) { | |
| case RS::MOTION_BLUR_FRAMERATE_MODE_NATIVE: | |
| // Use raw intensity, ignore frame time | |
| break; | |
| case RS::MOTION_BLUR_FRAMERATE_MODE_CAPPED: | |
| intensity *= MIN(1.f / reference_framerate, time_step) / time_step; | |
| break; | |
| case RS::MOTION_BLUR_FRAMERATE_MODE_FIXED: | |
| // Scale intensity by frame time | |
| intensity /= reference_framerate * time_step; | |
| break; | |
| float intensity = RSG::camera_attributes->camera_attributes_get_motion_blur_intensity(p_camera_attributes); | |
| int reference_framerate = RSG::camera_attributes->camera_attributes_get_motion_blur_reference_framerate(); | |
| const double time_scale = Engine::get_singleton()->get_effective_time_scale(); | |
| float time_step = 0.0f; | |
| if (time_scale > 0.0 && p_time_step > 0.0f) { | |
| time_step = p_time_step / (float)time_scale; | |
| } | |
| switch (RSG::camera_attributes->camera_attributes_get_motion_blur_framerate_mode()) { | |
| case RS::MOTION_BLUR_FRAMERATE_MODE_NATIVE: | |
| // Use raw intensity, ignore frame time | |
| break; | |
| case RS::MOTION_BLUR_FRAMERATE_MODE_CAPPED: | |
| if (time_step > 0.0f && reference_framerate > 0) { | |
| intensity *= MIN(1.f / reference_framerate, time_step) / time_step; | |
| } else { | |
| intensity = 0.0f; | |
| } | |
| break; | |
| case RS::MOTION_BLUR_FRAMERATE_MODE_FIXED: | |
| // Scale intensity by frame time | |
| if (time_step > 0.0f && reference_framerate > 0) { | |
| intensity /= reference_framerate * time_step; | |
| } else { | |
| intensity = 0.0f; | |
| } | |
| break; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@servers/rendering/renderer_rd/effects/motion_blur.cpp` around lines 246 -
260, Guard the `time_scale` normalization in `motion_blur.cpp` before computing
`time_step` in the motion blur setup.
`Engine::get_singleton()->get_effective_time_scale()` can be 0.0 when time is
frozen, so update the logic around
`camera_attributes_get_motion_blur_framerate_mode()` to skip the division or use
a safe fallback when `time_scale` or the resulting frame time is zero. Make sure
`intensity` and `time_step` stay finite before they are used by the
push-constant paths.
Comment on lines
+1761
to
+1768
| bool using_motion_blur = RSG::camera_attributes->camera_attributes_uses_motion_blur(p_render_data->camera_attributes); | ||
|
|
||
| // check if we need motion vectors | ||
| bool motion_vectors_required; | ||
| if (using_debug_mvs) { | ||
| motion_vectors_required = true; | ||
| } else if (using_motion_blur) { | ||
| motion_vectors_required = true; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Keep motion-vector work in sync with the motion-blur capability gate.
RendererSceneRenderRD::_render_buffers_post_process_and_tonemap() disables motion blur when storage support is missing, but this path still turns on motion vectors — and the MSAA velocity resolve — as soon as the camera attribute is enabled. On those configs we still allocate/write/resolve velocity data for an effect that can never run.
Suggested fix
- bool using_motion_blur = RSG::camera_attributes->camera_attributes_uses_motion_blur(p_render_data->camera_attributes);
+ bool using_motion_blur = _render_buffers_can_be_storage() &&
+ RSG::camera_attributes->camera_attributes_uses_motion_blur(p_render_data->camera_attributes);
...
- bool resolve_velocity_buffer = (using_taa || using_motion_blur || using_upscaling || ce_needs_motion_vectors) && rb->has_velocity_buffer(true);
+ bool resolve_velocity_buffer = (using_taa || using_motion_blur || using_upscaling || ce_needs_motion_vectors) && rb->has_velocity_buffer(true);Also applies to: 2402-2402
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp`
around lines 1761 - 1768, In
`RendererSceneRenderRD::_render_buffers_post_process_and_tonemap()`, the
`motion_vectors_required` decision is still tied directly to `using_motion_blur`
even though motion blur is later gated off when storage support is unavailable.
Update the motion-vector/MSAA velocity resolve gating so it follows the same
motion-blur capability check used for `using_motion_blur`, while still allowing
`using_debug_mvs` to force it on. Make the condition consistent with the
motion-blur support gate to avoid allocating or resolving velocity data when
motion blur cannot run.
Comment on lines
+1702
to
+1705
| RSG::camera_attributes->camera_attributes_set_motion_blur_framerate_mode(RS::MotionBlurFramerateMode(int(GLOBAL_GET("rendering/camera/motion_blur/motion_blur_framerate_mode"))), int(GLOBAL_GET("rendering/camera/motion_blur/motion_blur_reference_framerate"))); | ||
| RSG::camera_attributes->camera_attributes_set_motion_blur_quality(RS::MotionBlurQuality(int(GLOBAL_GET("rendering/camera/motion_blur/motion_blur_quality")))); | ||
| RSG::camera_attributes->camera_attributes_set_motion_blur_tile_size(RS::MotionBlurTileSize(int(GLOBAL_GET("rendering/camera/motion_blur/motion_blur_tile_size")))); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recreate motion_blur when the tile-size setting changes.
RendererRD::MotionBlur captures the tile size in its constructor, but the new motion-blur tile-size setting is only pushed into RSG::camera_attributes here. After init, any runtime update to that public setting leaves the stored value and the actual blur pipeline out of sync unless this object is rebuilt or given a setter.
Also applies to: 1734-1734
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@servers/rendering/renderer_rd/renderer_scene_render_rd.cpp` around lines 1702
- 1705, The motion blur tile-size setting is only updated on
RSG::camera_attributes, but RendererRD::MotionBlur still keeps the original
constructor-captured value, so runtime changes leave the blur pipeline stale.
Update the motion blur handling in RendererSceneRenderRD/RendererRD::MotionBlur
so changes to the tile-size setting either rebuild the MotionBlur instance or
add a setter and reconfigure its internal state whenever the public setting
changes. Use the existing motion blur setup code path and the
camera_attributes_set_motion_blur_tile_size call as the integration point.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
core/config/engine.cpp (1)
131-133: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep
get_effective_time_scale()non-zero for divisor callers.Line 132 still returns
0.0while frozen, and the motion-blur path divides by this accessor. That leaves the frozen case feedinginf/NaNinto blur math instead of cleanly disabling the effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/config/engine.cpp` around lines 131 - 133, The `Engine::get_effective_time_scale()` accessor should not return `0.0` when `freeze_time_scale` is enabled, because callers like the motion-blur divisor path rely on it being non-zero. Update `get_effective_time_scale()` to return a safe non-zero fallback while frozen, and adjust any freeze-aware logic in `Engine` so the blur code can treat the frozen state as disabled without producing `inf` or `NaN`.doc/classes/CameraAttributesPractical.xml (1)
46-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the Forward+-only limitation on
motion_blur_enabled.This member now has text, but it still doesn't say motion blur is only supported in the Forward+ renderer or point readers to the related intensity/velocity controls. The class reference currently overstates where this toggle works.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/classes/CameraAttributesPractical.xml` around lines 46 - 49, Update the CameraAttributesPractical::motion_blur_enabled member description to state that motion blur is only supported in the Forward+ renderer, and mention the related intensity/velocity controls so readers know how it is configured. Keep the existing motion-sickness note, and adjust the documentation text to accurately reflect the renderer limitation without changing the member name or API.scene/resources/camera_attributes.cpp (2)
380-396: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExpose
motion_blur_clamp_velocities_to_tilein_bind_methods()and the property list.This flag is declared in
scene/resources/camera_attributes.hand forwarded in_update_motion_blur(), but it is never bound or added as anADD_PROPERTYhere. As-is, the setting is part of the runtime state but can't be edited from script or the inspector.Proposed fix
ClassDB::bind_method(D_METHOD("set_motion_blur_intensity", "intensity"), &CameraAttributesPractical::set_motion_blur_intensity); ClassDB::bind_method(D_METHOD("get_motion_blur_intensity"), &CameraAttributesPractical::get_motion_blur_intensity); + ClassDB::bind_method(D_METHOD("set_motion_blur_clamp_velocities_to_tile", "clamp_velocities_to_tile"), &CameraAttributesPractical::set_motion_blur_clamp_velocities_to_tile); + ClassDB::bind_method(D_METHOD("is_motion_blur_clamp_velocities_to_tile"), &CameraAttributesPractical::is_motion_blur_clamp_velocities_to_tile); ClassDB::bind_method(D_METHOD("set_motion_blur_object_velocity_multiplier", "multiplier"), &CameraAttributesPractical::set_motion_blur_object_velocity_multiplier); ClassDB::bind_method(D_METHOD("get_motion_blur_object_velocity_multiplier"), &CameraAttributesPractical::get_motion_blur_object_velocity_multiplier); @@ ADD_GROUP("Motion Blur", "motion_blur_"); ADD_PROPERTY(PropertyInfo(Variant::BOOL, "motion_blur_enabled", PROPERTY_HINT_GROUP_ENABLE), "set_motion_blur_enabled", "is_motion_blur_enabled"); ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "motion_blur_intensity", PROPERTY_HINT_RANGE, "0.0,1.0,0.01,or_greater"), "set_motion_blur_intensity", "get_motion_blur_intensity"); + ADD_PROPERTY(PropertyInfo(Variant::BOOL, "motion_blur_clamp_velocities_to_tile"), "set_motion_blur_clamp_velocities_to_tile", "is_motion_blur_clamp_velocities_to_tile"); ADD_SUBGROUP("Velocity Multipliers", "motion_blur_velocity_multiplier_");Also applies to: 421-434
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/resources/camera_attributes.cpp` around lines 380 - 396, The `motion_blur_clamp_velocities_to_tile` flag is implemented in `CameraAttributesPractical` but is not exposed to scripting or the inspector. Update `CameraAttributesPractical::_bind_methods()` to bind the getter and setter for this property, and add the corresponding `ADD_PROPERTY` entry in the property list alongside the other motion blur settings so it can be edited like the rest of the `motion_blur_*` fields.
426-429: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't cap the velocity multipliers at
1.0in the inspector.The setters accept any non-negative value, so these
0.0,1.0hints still make inspector-authored resources less capable than script or RenderingServer callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scene/resources/camera_attributes.cpp` around lines 426 - 429, The motion blur velocity multiplier properties in CameraAttributes are incorrectly limited by the inspector range hints, since the setters allow any non-negative value. Update the PropertyInfo range hints for motion_blur_velocity_multiplier_object, motion_blur_velocity_multiplier_camera_movement, and motion_blur_velocity_multiplier_camera_rotation so they are not capped at 1.0, keeping the inspector consistent with the setter behavior in CameraAttributes.servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp (1)
1761-1768: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winGate motion-vector work behind the same motion-blur capability check.
using_motion_bluris derived directly from the camera attributes here, so this path still turns on motion-vector generation and MSAA velocity resolves even when the post-process motion blur pass cannot run. That keeps the extra velocity-buffer work alive for no effect.Proposed fix
- bool using_motion_blur = RSG::camera_attributes->camera_attributes_uses_motion_blur(p_render_data->camera_attributes); + bool using_motion_blur = _render_buffers_can_be_storage() && + RSG::camera_attributes->camera_attributes_uses_motion_blur(p_render_data->camera_attributes);Also applies to: 2402-2402
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp` around lines 1761 - 1768, The motion-vector enablement in the forward clustered render path is being driven only by camera attributes, so it can still turn on velocity-buffer and MSAA resolve work even when motion blur itself is not supported. Update the motion-vector decision in render_forward_clustered.cpp to gate the `motion_vectors_required` path behind the same motion-blur capability check used for the post-process pass, while preserving the `using_debug_mvs` override; use the existing `using_motion_blur` and `motion_vectors_required` logic to keep the behavior aligned.servers/rendering/storage/camera_attributes_storage.cpp (1)
63-65: 🩺 Stability & Availability | 🟠 MajorStill unresolved: clamp
p_reference_frameratebefore storing it.RenderingServer callers can pass
0or a negative value here, andmotion_blur_compute()later divides by this value in capped/fixed modes. Please reject or clamp invalid inputs at the setter so the render path never sees an impossible denominator.Suggested fix
void RendererCameraAttributes::camera_attributes_set_motion_blur_framerate_mode(RS::MotionBlurFramerateMode p_mode, int p_reference_framerate) { motion_blur_framerate_mode = p_mode; - motion_blur_reference_framerate = p_reference_framerate; + motion_blur_reference_framerate = MAX(1, p_reference_framerate); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/rendering/storage/camera_attributes_storage.cpp` around lines 63 - 65, The setter in RendererCameraAttributes::camera_attributes_set_motion_blur_framerate_mode currently stores p_reference_framerate directly, which can later feed an invalid denominator into motion_blur_compute(). Update this setter to validate the incoming reference framerate and clamp or reject non-positive values before assigning motion_blur_reference_framerate, while still storing motion_blur_framerate_mode as-is.servers/rendering/renderer_rd/effects/motion_blur.cpp (3)
142-216: 🩺 Stability & Availability | 🔴 CriticalStill unresolved: close the inner GPU label in
motion_blur_process().The label opened at Line 142 is never ended before
compute_list_end(), so the outerdraw_command_end_label()inmotion_blur_compute()closes the wrong scope.Suggested fix
RD::get_singleton()->compute_list_dispatch_threads(compute_list, p_buffers.base_size.x, p_buffers.base_size.y, 1); RD::get_singleton()->compute_list_add_barrier(compute_list); + RD::get_singleton()->draw_command_end_label(); RD::get_singleton()->compute_list_end(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/rendering/renderer_rd/effects/motion_blur.cpp` around lines 142 - 216, The inner GPU label in motion_blur_process() is opened with draw_command_begin_label but never closed before compute_list_end(), causing the outer label in motion_blur_compute() to terminate the wrong scope. Add the matching draw_command_end_label call in motion_blur_process() after the motion blur dispatch/barrier sequence, using the existing RD::get_singleton() flow and the motion_blur_process/motion_blur_compute label pairing to place it correctly.
246-259: 🩺 Stability & Availability | 🔴 CriticalStill unresolved: guard frozen time and zero frame time before normalizing blur intensity.
Engine::get_effective_time_scale()can be0.0, so Line 249 can divide by zero before the framerate-mode switch runs.p_time_step == 0also makes the capped/fixed branches unsafe at Lines 255 and 259.Suggested fix
float intensity = RSG::camera_attributes->camera_attributes_get_motion_blur_intensity(p_camera_attributes); int reference_framerate = RSG::camera_attributes->camera_attributes_get_motion_blur_reference_framerate(); const double time_scale = Engine::get_singleton()->get_effective_time_scale(); - float time_step = p_time_step / (float)time_scale; + float time_step = 0.0f; + if (time_scale > 0.0 && p_time_step > 0.0f) { + time_step = p_time_step / (float)time_scale; + } switch (RSG::camera_attributes->camera_attributes_get_motion_blur_framerate_mode()) { case RS::MOTION_BLUR_FRAMERATE_MODE_NATIVE: break; case RS::MOTION_BLUR_FRAMERATE_MODE_CAPPED: - intensity *= MIN(1.f / reference_framerate, time_step) / time_step; + if (time_step > 0.0f && reference_framerate > 0) { + intensity *= MIN(1.f / reference_framerate, time_step) / time_step; + } else { + intensity = 0.0f; + } break; case RS::MOTION_BLUR_FRAMERATE_MODE_FIXED: - intensity /= reference_framerate * time_step; + if (time_step > 0.0f && reference_framerate > 0) { + intensity /= reference_framerate * time_step; + } else { + intensity = 0.0f; + } break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/rendering/renderer_rd/effects/motion_blur.cpp` around lines 246 - 259, Guard the motion blur normalization in the motion blur intensity path so frozen time and zero delta cannot divide by zero. In motion blur::? around the code that reads Engine::get_singleton()->get_effective_time_scale(), p_time_step, and the framerate-mode switch, add early handling for time_scale == 0 or p_time_step == 0 before computing time_step or applying the CAPPED/FIXED scaling. Keep the existing switch behavior in the non-zero case and use the same motion blur helper logic to preserve native, capped, and fixed modes safely.
228-239: 🩺 Stability & Availability | 🔴 CriticalStill unresolved: invalidate the motion blur context when size or view count changes.
This cache only checks
has_texture(). After a viewport resize or multiview-count change, Lines 241-243 and 307-316 use the newbase_size/tiled_size/view layout against the old images, which can send the compute passes to stale dimensions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/rendering/renderer_rd/effects/motion_blur.cpp` around lines 228 - 239, The motion blur buffer cache only keys off has_texture() and must also be invalidated when the viewport size or multiview count changes. Update the motion blur setup logic around p_render_buffers and the RB_SCOPE_MOTION_BLUR/RB_TEX_* textures to track the last base_size, tiled_size, and view layout, and recreate the textures when any of those change instead of reusing stale images. Ensure the compute pass setup later in the same motion blur flow always uses textures matching the current dimensions and view configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/classes/EditorSettings.xml`:
- Around line 438-440: Fix the duplicated word in the description for the
show_motion_blur_in_editor setting under EditorSettings.xml. Update the text in
the member entry for editors/3d/viewport_visuals/show_motion_blur_in_editor so
it reads naturally by removing the extra repeated word in the sentence about
motion blur being applied in the editor scene view.
---
Duplicate comments:
In `@core/config/engine.cpp`:
- Around line 131-133: The `Engine::get_effective_time_scale()` accessor should
not return `0.0` when `freeze_time_scale` is enabled, because callers like the
motion-blur divisor path rely on it being non-zero. Update
`get_effective_time_scale()` to return a safe non-zero fallback while frozen,
and adjust any freeze-aware logic in `Engine` so the blur code can treat the
frozen state as disabled without producing `inf` or `NaN`.
In `@doc/classes/CameraAttributesPractical.xml`:
- Around line 46-49: Update the CameraAttributesPractical::motion_blur_enabled
member description to state that motion blur is only supported in the Forward+
renderer, and mention the related intensity/velocity controls so readers know
how it is configured. Keep the existing motion-sickness note, and adjust the
documentation text to accurately reflect the renderer limitation without
changing the member name or API.
In `@scene/resources/camera_attributes.cpp`:
- Around line 380-396: The `motion_blur_clamp_velocities_to_tile` flag is
implemented in `CameraAttributesPractical` but is not exposed to scripting or
the inspector. Update `CameraAttributesPractical::_bind_methods()` to bind the
getter and setter for this property, and add the corresponding `ADD_PROPERTY`
entry in the property list alongside the other motion blur settings so it can be
edited like the rest of the `motion_blur_*` fields.
- Around line 426-429: The motion blur velocity multiplier properties in
CameraAttributes are incorrectly limited by the inspector range hints, since the
setters allow any non-negative value. Update the PropertyInfo range hints for
motion_blur_velocity_multiplier_object,
motion_blur_velocity_multiplier_camera_movement, and
motion_blur_velocity_multiplier_camera_rotation so they are not capped at 1.0,
keeping the inspector consistent with the setter behavior in CameraAttributes.
In `@servers/rendering/renderer_rd/effects/motion_blur.cpp`:
- Around line 142-216: The inner GPU label in motion_blur_process() is opened
with draw_command_begin_label but never closed before compute_list_end(),
causing the outer label in motion_blur_compute() to terminate the wrong scope.
Add the matching draw_command_end_label call in motion_blur_process() after the
motion blur dispatch/barrier sequence, using the existing RD::get_singleton()
flow and the motion_blur_process/motion_blur_compute label pairing to place it
correctly.
- Around line 246-259: Guard the motion blur normalization in the motion blur
intensity path so frozen time and zero delta cannot divide by zero. In motion
blur::? around the code that reads
Engine::get_singleton()->get_effective_time_scale(), p_time_step, and the
framerate-mode switch, add early handling for time_scale == 0 or p_time_step ==
0 before computing time_step or applying the CAPPED/FIXED scaling. Keep the
existing switch behavior in the non-zero case and use the same motion blur
helper logic to preserve native, capped, and fixed modes safely.
- Around line 228-239: The motion blur buffer cache only keys off has_texture()
and must also be invalidated when the viewport size or multiview count changes.
Update the motion blur setup logic around p_render_buffers and the
RB_SCOPE_MOTION_BLUR/RB_TEX_* textures to track the last base_size, tiled_size,
and view layout, and recreate the textures when any of those change instead of
reusing stale images. Ensure the compute pass setup later in the same motion
blur flow always uses textures matching the current dimensions and view
configuration.
In
`@servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp`:
- Around line 1761-1768: The motion-vector enablement in the forward clustered
render path is being driven only by camera attributes, so it can still turn on
velocity-buffer and MSAA resolve work even when motion blur itself is not
supported. Update the motion-vector decision in render_forward_clustered.cpp to
gate the `motion_vectors_required` path behind the same motion-blur capability
check used for the post-process pass, while preserving the `using_debug_mvs`
override; use the existing `using_motion_blur` and `motion_vectors_required`
logic to keep the behavior aligned.
In `@servers/rendering/storage/camera_attributes_storage.cpp`:
- Around line 63-65: The setter in
RendererCameraAttributes::camera_attributes_set_motion_blur_framerate_mode
currently stores p_reference_framerate directly, which can later feed an invalid
denominator into motion_blur_compute(). Update this setter to validate the
incoming reference framerate and clamp or reject non-positive values before
assigning motion_blur_reference_framerate, while still storing
motion_blur_framerate_mode as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91cad464-0452-455f-a3dd-f218d74fde7f
⛔ Files ignored due to path filters (5)
servers/rendering/renderer_rd/shaders/effects/motion_blur_blur.glslis excluded by!**/*.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_neighbor_max.glslis excluded by!**/*.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_preprocess.glslis excluded by!**/*.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_x.glslis excluded by!**/*.glslservers/rendering/renderer_rd/shaders/effects/motion_blur_tile_max_y.glslis excluded by!**/*.glsl
📒 Files selected for processing (23)
COPYRIGHT.txtcore/config/engine.cppcore/config/engine.hdoc/classes/CameraAttributesPractical.xmldoc/classes/EditorSettings.xmldoc/classes/ProjectSettings.xmldoc/classes/RenderingServer.xmleditor/editor_node.cppeditor/scene/3d/node_3d_editor_plugin.cppeditor/settings/editor_settings.cppscene/resources/camera_attributes.cppscene/resources/camera_attributes.hservers/rendering/renderer_rd/effects/motion_blur.cppservers/rendering/renderer_rd/effects/motion_blur.hservers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cppservers/rendering/renderer_rd/pipeline_deferred_rd.hservers/rendering/renderer_rd/renderer_scene_render_rd.cppservers/rendering/renderer_rd/renderer_scene_render_rd.hservers/rendering/rendering_server_default.hservers/rendering/storage/camera_attributes_storage.cppservers/rendering/storage/camera_attributes_storage.hservers/rendering_server.cppservers/rendering_server.h
Comment on lines
+438
to
+440
| <member name="editors/3d/viewport_visuals/show_motion_blur_in_editor" type="bool" setter="" getter=""> | ||
| If [code]true[/code], when a scene has a camera that has motion blur enabled, it will be be applied in the editor scene view as well. When [code]false[/code], the motion blur will only be applied at runtime. | ||
| </member> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the duplicated word in the setting description.
Line 439 says “will be be applied”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/classes/EditorSettings.xml` around lines 438 - 440, Fix the duplicated
word in the description for the show_motion_blur_in_editor setting under
EditorSettings.xml. Update the text in the member entry for
editors/3d/viewport_visuals/show_motion_blur_in_editor so it reads naturally by
removing the extra repeated word in the sentence about motion blur being applied
in the editor scene view.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. There are some differences compared to the original plugin, notably:
Supported backend: Forward+.
Graphics part by @HydrogenC, editor part mainly by @Shakai-Dev & docs mainly by @sphynx-owner
Preview:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores