Draft
Conversation
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>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a complete motion blur feature for the rendering engine. It adds compute shader-based motion blur processing with configurable quality and tile sizes, expands camera attributes with motion blur parameters, introduces project settings for motion blur configuration, and integrates motion blur into the renderer's post-processing pipeline. Changes
Sequence DiagramsequenceDiagram
participant Camera as Camera/Editor
participant Attr as CameraAttributes
participant Storage as Camera Storage
participant Renderer as RendererRD
participant MB as MotionBlur
participant Shader as Compute Shaders
participant Output as Final Image
Camera->>Attr: set_motion_blur_*(parameters)
Attr->>Attr: _update_motion_blur()
Renderer->>Storage: camera_attributes_uses_motion_blur()
alt Motion Blur Enabled & Conditions Met
Renderer->>MB: motion_blur_compute(render_buffers, camera_attributes, ...)
MB->>Storage: Get motion blur params (intensity, multipliers, thresholds)
Storage-->>MB: Motion blur configuration
MB->>Shader: Preprocess (velocity setup)
Shader->>Shader: Compute per-pixel velocity
Shader-->>MB: Velocity buffer
MB->>Shader: Tile Max X (find max velocity per tile)
Shader-->>MB: tile_max_x
MB->>Shader: Tile Max Y (reduce to tile maxima)
Shader-->>MB: tile_max (final per-tile max)
MB->>Shader: Neighbor Max (3x3 neighborhood max)
Shader-->>MB: neighbor_max texture
MB->>Shader: Blur (apply motion blur)
Shader->>Shader: Sample neighbors using max velocity
Shader-->>MB: Blurred image
MB->>Output: Copy result back to base texture
end
Output-->>Camera: Rendered frame with motion blur
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
| <member name="dof_blur_near_transition" type="float" setter="set_dof_blur_near_transition" getter="get_dof_blur_near_transition" default="1.0"> | ||
| When positive, distance over which blur effect will scale from 0 to [member dof_blur_amount], ending at [member dof_blur_near_distance]. When negative, uses physically-based scaling so depth of field effect will scale from 0 at [member dof_blur_near_distance] and will increase in a physically accurate way as objects get closer to the [Camera3D]. | ||
| </member> | ||
| <member name="motion_blur_clamp_velocities_to_tile" type="bool" setter="set_motion_blur_clamp_velocities_to_tile" getter="is_motion_blur_clamp_velocities_to_tile" default="true"> |
There was a problem hiding this comment.
Inconsistent indentation on motion_blur_clamp_velocities_to_tile member tag.
This <member> tag uses extra indentation (\t\t\t\t) compared to sibling <member> elements which use \t\t. This will cause XML formatting inconsistencies in the documentation.
- <member name="motion_blur_clamp_velocities_to_tile" type="bool" setter="set_motion_blur_clamp_velocities_to_tile" getter="is_motion_blur_clamp_velocities_to_tile" default="true">
+ <member name="motion_blur_clamp_velocities_to_tile" type="bool" setter="set_motion_blur_clamp_velocities_to_tile" getter="is_motion_blur_clamp_velocities_to_tile" default="true">📝 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="motion_blur_clamp_velocities_to_tile" type="bool" setter="set_motion_blur_clamp_velocities_to_tile" getter="is_motion_blur_clamp_velocities_to_tile" default="true"> | |
| <member name="motion_blur_clamp_velocities_to_tile" type="bool" setter="set_motion_blur_clamp_velocities_to_tile" getter="is_motion_blur_clamp_velocities_to_tile" default="true"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/classes/CameraAttributesPractical.xml` at 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.
Comment on lines
+47
to
+48
| <member name="motion_blur_enabled" type="bool" setter="set_motion_blur_enabled" getter="is_motion_blur_enabled" default="false"> | ||
| </member> |
There was a problem hiding this comment.
motion_blur_enabled has no description.
All other new motion blur members have descriptive documentation, but this one is empty. Please add a description, e.g., noting that motion blur is only supported in the Forward+ renderer and referencing the related intensity/multiplier controls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/classes/CameraAttributesPractical.xml` around lines 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.
Comment on lines
2806
to
2820
| <member name="rendering/camera/motion_blur/motion_blur_framerate_mode" type="int" setter="" getter="" default="1"> | ||
| Define framerate-based behavior of the motion blur. Uses [member motion_blur_reference_framerate] as the reference framerate for the different modes. | ||
| MOTION_BLUR_FRAMERATE_MODE_NATIVE applies the blur based on the game's framerate as is, and does not use the reference framerate. | ||
| MOTION_BLUR_FRAMERATE_MODE_CAPPED applies the blur based on the game's framerate, but below a certain framerate (which means larger time gap and thus larger blur) 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. | ||
| MOTION_BLUR_FRAMERATE_MODE_FIXED 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: | ||
| MOTION_BLUR_QUALITY_LOW = 4 samples | ||
| MOTION_BLUR_QUALITY_MEDIUM = 8 samples | ||
| MOTION_BLUR_QUALITY_HIGH = 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 reference by [member motion_blur_framerate_mode]. | ||
| </member> |
There was a problem hiding this comment.
Fix [member] links to use full setting paths.
The references to other motion blur settings are missing their full paths, which may break doc links and tooltips. Consider using the full member path here.
🔧 Suggested doc fix
- Define framerate-based behavior of the motion blur. Uses [member motion_blur_reference_framerate] as the reference framerate for the different modes.
+ Defines 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.
@@
- Defines a framerate to be used as reference by [member motion_blur_framerate_mode].
+ Defines a framerate to be used as reference by [member rendering/camera/motion_blur/motion_blur_framerate_mode].📝 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_framerate_mode" type="int" setter="" getter="" default="1"> | |
| Define framerate-based behavior of the motion blur. Uses [member motion_blur_reference_framerate] as the reference framerate for the different modes. | |
| MOTION_BLUR_FRAMERATE_MODE_NATIVE applies the blur based on the game's framerate as is, and does not use the reference framerate. | |
| MOTION_BLUR_FRAMERATE_MODE_CAPPED applies the blur based on the game's framerate, but below a certain framerate (which means larger time gap and thus larger blur) 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. | |
| MOTION_BLUR_FRAMERATE_MODE_FIXED 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: | |
| MOTION_BLUR_QUALITY_LOW = 4 samples | |
| MOTION_BLUR_QUALITY_MEDIUM = 8 samples | |
| MOTION_BLUR_QUALITY_HIGH = 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 reference by [member motion_blur_framerate_mode]. | |
| </member> | |
| <member name="rendering/camera/motion_blur/motion_blur_framerate_mode" type="int" setter="" getter="" default="1"> | |
| Defines 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. | |
| MOTION_BLUR_FRAMERATE_MODE_NATIVE applies the blur based on the game's framerate as is, and does not use the reference framerate. | |
| MOTION_BLUR_FRAMERATE_MODE_CAPPED applies the blur based on the game's framerate, but below a certain framerate (which means larger time gap and thus larger blur) 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. | |
| MOTION_BLUR_FRAMERATE_MODE_FIXED 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: | |
| MOTION_BLUR_QUALITY_LOW = 4 samples | |
| MOTION_BLUR_QUALITY_MEDIUM = 8 samples | |
| MOTION_BLUR_QUALITY_HIGH = 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 reference by [member rendering/camera/motion_blur/motion_blur_framerate_mode]. | |
| </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 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.
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.
Contributor
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. |
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
2 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 @AR-DEV-1 & docs mainly by @sphynx-owner
Preview:
Note
The current blocker is that the current implementation exposes too many parameters, we may have to scale the number down and make some of them constant instead.
Summary by CodeRabbit