Add pipeline_deffered_rd.h for the motion blur PR#1202
Open
AR-DEV-1 wants to merge 7 commits intoRedot-Engine:masterfrom
Open
Add pipeline_deffered_rd.h for the motion blur PR#1202AR-DEV-1 wants to merge 7 commits intoRedot-Engine:masterfrom
pipeline_deffered_rd.h for the motion blur PR#1202AR-DEV-1 wants to merge 7 commits intoRedot-Engine:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new header declaring PipelineDeferredRD, a class that defers RD render/compute pipeline compilation to a worker pool, stores the resulting RID, and exposes APIs to create (render/compute), wait-for/get, and free pipelines with debug-time lifecycle checks. (28 words) Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant PipelineDeferredRD
participant WorkerThreadPool
participant RenderingDevice
Client->>PipelineDeferredRD: create_render_pipeline(...) / create_compute_pipeline(...)
PipelineDeferredRD->>PipelineDeferredRD: store CreationParameters
PipelineDeferredRD->>WorkerThreadPool: queue background task (TaskID)
WorkerThreadPool->>PipelineDeferredRD: invoke _create(params) (background)
PipelineDeferredRD->>RenderingDevice: request pipeline compilation (render/compute)
RenderingDevice-->>PipelineDeferredRD: return Pipeline RID
PipelineDeferredRD->>PipelineDeferredRD: store pipeline RID, clear task
Client->>PipelineDeferredRD: get_rid()
PipelineDeferredRD->>PipelineDeferredRD: _wait() (blocks until task complete)
PipelineDeferredRD-->>Client: return Pipeline RID
Client->>PipelineDeferredRD: free()
PipelineDeferredRD->>PipelineDeferredRD: _wait()
PipelineDeferredRD->>RenderingDevice: release pipeline RID
PipelineDeferredRD->>PipelineDeferredRD: clear pipeline state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 3
🧹 Nitpick comments (1)
servers/rendering/renderer_rd/pipeline_deferred_rd.h (1)
82-84: Nit: remove vacuous constructor comment.
// Default constructor.adds no information over the empty body itself.✏️ Proposed fix
PipelineDeferredRD() { - // Default constructor. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` around lines 82 - 84, Remove the vacuous inline comment inside the default constructor for PipelineDeferredRD; edit the PipelineDeferredRD() { /* ... */ } definition to have an empty body with no "// Default constructor." comment so the constructor remains concise and self-explanatory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Around line 132-133: The file ends with the closing token "};" but is missing
a trailing newline; to fix the CI failure update the file so that after the
final "};" there is a single newline character at EOF (ensure the file
terminates with a newline), e.g., modify the end of pipeline_deferred_rd.h to
add a trailing newline after the closing "};".
- Line 127: There's a typo in the error message string inside the
ERR_FAIL_COND_MSG call that checks
RD::get_singleton()->render_pipeline_is_valid(pipeline) ||
RD::get_singleton()->compute_pipeline_is_valid(pipeline); change the message
from "`free()` must be called manually before the dependent shader is freed."
to use a single space after "called" (e.g. "`free()` must be called manually
before the dependent shader is freed.") so the message reads correctly.
- Around line 86-90: The destructor of PipelineDeferredRD must unconditionally
wait for any in-flight worker task before performing validation or returning:
call _wait() at the very start of ~PipelineDeferredRD() to ensure background
tasks (from create_render_pipeline()/create_compute_pipeline()) complete and do
not write into freed memory, then keep the DEV_ENABLED validation
(pipeline.is_valid()) after the wait; also ensure the task handle is
cleared/collected after waiting so it is not leaked. Ensure references to
pipeline and the worker task are only accessed after the _wait() call.
---
Nitpick comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Around line 82-84: Remove the vacuous inline comment inside the default
constructor for PipelineDeferredRD; edit the PipelineDeferredRD() { /* ... */ }
definition to have an empty body with no "// Default constructor." comment so
the constructor remains concise and self-explanatory.
Contributor
|
@AR-DEV-1 do you mind running |
AR-DEV-1
force-pushed
the
pipeline_deferred_rd
branch
from
c772cf6 to
3d1fdc0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
servers/rendering/renderer_rd/pipeline_deferred_rd.h (1)
86-90:⚠️ Potential issue | 🔴 CriticalCall
_wait()at the start of~PipelineDeferredRD()to prevent in-flight task UAF.Line 86 currently allows destruction without synchronizing an active worker task.
_start()submits_create()withthis, so the worker can write intopipelineafter object memory is released. Add_wait()as the first line in the destructor, then keep the DEV check.#!/bin/bash set -euo pipefail file="$(fd -t f 'pipeline_deferred_rd.h$' | head -n1)" echo "Inspecting: $file" nl -ba "$file" | sed -n '60,100p' python - <<'PY' import pathlib, sys p = next(pathlib.Path('.').rglob('pipeline_deferred_rd.h')) text = p.read_text() needle = "~PipelineDeferredRD()" idx = text.find(needle) if idx < 0: print("FAIL: destructor not found") sys.exit(1) brace = text.find("{", idx) depth = 0 end = None for i in range(brace, len(text)): if text[i] == "{": depth += 1 elif text[i] == "}": depth -= 1 if depth == 0: end = i break body = text[brace + 1:end] code_lines = [ln.strip() for ln in body.splitlines() if ln.strip() and not ln.strip().startswith("#")] first = code_lines[0] if code_lines else "<empty>" print("First executable destructor line:", first) if not first.startswith("_wait("): print("FAIL: destructor does not call _wait() first") sys.exit(2) if "add_template_task(this, &PipelineDeferredRD::_create" in text: print("PASS: task captures this; destructor wait ordering is enforced") else: print("WARN: expected add_template_task(this, ...) pattern not found") PYProposed fix
~PipelineDeferredRD() { + _wait(); `#ifdef` DEV_ENABLED ERR_FAIL_COND_MSG(pipeline.is_valid(), "'free()' must be called manually before deconstruction and before the corresponding shader is freed."); `#endif` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` around lines 86 - 90, The destructor ~PipelineDeferredRD() must call _wait() as its first action to synchronize any in-flight worker task before the object memory can be used by the worker (since _start() submits add_template_task(this, &PipelineDeferredRD::_create...) which can write into pipeline); modify the body of ~PipelineDeferredRD() to call _wait() first, then keep the existing DEV_ENABLED ERR_FAIL_COND_MSG(pipeline.is_valid(), "'free()' must be called manually...") check and existing behavior related to free() and pipeline cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Around line 86-90: The destructor ~PipelineDeferredRD() must call _wait() as
its first action to synchronize any in-flight worker task before the object
memory can be used by the worker (since _start() submits add_template_task(this,
&PipelineDeferredRD::_create...) which can write into pipeline); modify the body
of ~PipelineDeferredRD() to call _wait() first, then keep the existing
DEV_ENABLED ERR_FAIL_COND_MSG(pipeline.is_valid(), "'free()' must be called
manually...") check and existing behavior related to free() and pipeline
cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7228d543-9d6f-4c54-9057-03e3ad7428d0
📒 Files selected for processing (1)
servers/rendering/renderer_rd/pipeline_deferred_rd.h
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
servers/rendering/renderer_rd/pipeline_deferred_rd.h (2)
128-128:⚠️ Potential issue | 🟡 MinorFix typo in DEV error message.
Line 128 has a double space in "
free()must be called manually".✏️ Proposed fix
- ERR_FAIL_COND_MSG(!(RD::get_singleton()->render_pipeline_is_valid(pipeline) || RD::get_singleton()->compute_pipeline_is_valid(pipeline)), "`free()` must be called manually before the dependent shader is freed."); + ERR_FAIL_COND_MSG(!(RD::get_singleton()->render_pipeline_is_valid(pipeline) || RD::get_singleton()->compute_pipeline_is_valid(pipeline)), "`free()` must be called manually before the dependent shader is freed.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` at line 128, The DEV error message passed to ERR_FAIL_COND_MSG in pipeline_deferred_rd.h contains a double space in the string "`free()` must be called manually before the dependent shader is freed."; open the ERR_FAIL_COND_MSG call that checks RD::get_singleton()->render_pipeline_is_valid and RD::get_singleton()->compute_pipeline_is_valid and fix the typo by removing the extra space so it reads "`free()` must be called manually before the dependent shader is freed."; ensure no other punctuation or wording is changed.
134-134:⚠️ Potential issue | 🟡 MinorAdd trailing newline at EOF to satisfy pre-commit.
The file-format hook failure indicates this file still needs a final newline.
✏️ Proposed fix
}; +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` at line 134, The file is missing a trailing newline at EOF causing the pre-commit hook to fail; open the header containing the final closing token "};" (the class/struct end in pipeline_deferred_rd.h), ensure there is a single newline character after that closing brace so the file ends with a newline, save and re-run the pre-commit checks before committing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Around line 74-78: In _wait(), don't unconditionally clear task; call
WorkerThreadPool::get_singleton()->wait_for_task_completion(task) and check its
Error result, only setting task = WorkerThreadPool::INVALID_TASK_ID if the call
returns OK; if it returns ERR_INVALID_PARAMETER or ERR_BUSY, preserve the task
id and handle/report the error (e.g., log an error or assert) so the failure
isn't silently dropped—refer to the _wait method, the task member,
WorkerThreadPool::get_singleton(), wait_for_task_completion(), and
WorkerThreadPool::INVALID_TASK_ID when making the change.
---
Duplicate comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Line 128: The DEV error message passed to ERR_FAIL_COND_MSG in
pipeline_deferred_rd.h contains a double space in the string "`free()` must be
called manually before the dependent shader is freed."; open the
ERR_FAIL_COND_MSG call that checks RD::get_singleton()->render_pipeline_is_valid
and RD::get_singleton()->compute_pipeline_is_valid and fix the typo by removing
the extra space so it reads "`free()` must be called manually before the
dependent shader is freed."; ensure no other punctuation or wording is changed.
- Line 134: The file is missing a trailing newline at EOF causing the pre-commit
hook to fail; open the header containing the final closing token "};" (the
class/struct end in pipeline_deferred_rd.h), ensure there is a single newline
character after that closing brace so the file ends with a newline, save and
re-run the pre-commit checks before committing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae887d01-6b80-40e1-a603-d184efe00eb0
📒 Files selected for processing (1)
servers/rendering/renderer_rd/pipeline_deferred_rd.h
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
servers/rendering/renderer_rd/pipeline_deferred_rd.h (3)
135-135:⚠️ Potential issue | 🟡 MinorCommit the EOF newline auto-fix from pre-commit.
CI indicates file-format auto-fixed this file; ensure a trailing newline is committed after Line [135].
✏️ Proposed fix
}; +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` at line 135, The file ends with the closing brace token "};" but is missing a trailing newline at EOF; update the file (ending at the "};" token in pipeline_deferred_rd.h) to include a single newline character after the final line so the pre-commit/CI file-format check no longer auto-fixes it.
74-78:⚠️ Potential issue | 🟠 MajorHandle
wait_for_task_completion()errors before clearingtask.On Line [76], the return
Erroris ignored, and Line [77] clearstaskunconditionally. If waiting fails, the handle/state is silently dropped.🔧 Proposed fix
void _wait() { if (task != WorkerThreadPool::INVALID_TASK_ID) { - WorkerThreadPool::get_singleton()->wait_for_task_completion(task); + const Error err = WorkerThreadPool::get_singleton()->wait_for_task_completion(task); + ERR_FAIL_COND_MSG(err != OK, "Failed to wait for deferred pipeline compilation task."); task = WorkerThreadPool::INVALID_TASK_ID; } }#!/bin/bash # Verify wait_for_task_completion signature and failure paths. # Expected: returns Error and includes non-OK branches. rg -n "Error wait_for_task_completion|wait_for_task_completion\\(" core/object/worker_thread_pool.h core/object/worker_thread_pool.cpp -A8 -B3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` around lines 74 - 78, The _wait() method currently ignores the Error returned by WorkerThreadPool::wait_for_task_completion and unconditionally resets task to WorkerThreadPool::INVALID_TASK_ID; change it to call WorkerThreadPool::wait_for_task_completion(task), check the returned Error and only clear/reset task on success (or handle/report the Error via logging/returning as appropriate), ensuring task remains valid if wait_for_task_completion fails so the handle/state is not silently dropped; reference symbols: _wait(), task, WorkerThreadPool::wait_for_task_completion, WorkerThreadPool::INVALID_TASK_ID.
128-128:⚠️ Potential issue | 🟡 MinorFix typo in DEV error message (
called manually).Line [128] still has a double space in the string literal.
✏️ Proposed fix
- ERR_FAIL_COND_MSG(!(RD::get_singleton()->render_pipeline_is_valid(pipeline) || RD::get_singleton()->compute_pipeline_is_valid(pipeline)), "`free()` must be called manually before the dependent shader is freed."); + ERR_FAIL_COND_MSG(!(RD::get_singleton()->render_pipeline_is_valid(pipeline) || RD::get_singleton()->compute_pipeline_is_valid(pipeline)), "`free()` must be called manually before the dependent shader is freed.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` at line 128, The DEV error message in pipeline_deferred_rd.h uses an ERR_FAIL_COND_MSG call whose string literal contains a double space between "called" and "manually"; update the ERR_FAIL_COND_MSG invocation (the one checking RD::get_singleton()->render_pipeline_is_valid/compute_pipeline_is_valid for pipeline) to remove the extra space so the message reads "must be called manually before the dependent shader is freed." and keep the rest of the condition unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Line 130: The call uses the nonexistent C++ API method free_rid; replace
RD::get_singleton()->free_rid(pipeline) with RD::get_singleton()->free(pipeline)
so the C++ RenderingDevice API is used correctly; update the code at the site
where pipeline is released (the RD::get_singleton() call) to call free(RID)
instead of free_rid to fix the compilation error.
---
Duplicate comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Line 135: The file ends with the closing brace token "};" but is missing a
trailing newline at EOF; update the file (ending at the "};" token in
pipeline_deferred_rd.h) to include a single newline character after the final
line so the pre-commit/CI file-format check no longer auto-fixes it.
- Around line 74-78: The _wait() method currently ignores the Error returned by
WorkerThreadPool::wait_for_task_completion and unconditionally resets task to
WorkerThreadPool::INVALID_TASK_ID; change it to call
WorkerThreadPool::wait_for_task_completion(task), check the returned Error and
only clear/reset task on success (or handle/report the Error via
logging/returning as appropriate), ensuring task remains valid if
wait_for_task_completion fails so the handle/state is not silently dropped;
reference symbols: _wait(), task, WorkerThreadPool::wait_for_task_completion,
WorkerThreadPool::INVALID_TASK_ID.
- Line 128: The DEV error message in pipeline_deferred_rd.h uses an
ERR_FAIL_COND_MSG call whose string literal contains a double space between
"called" and "manually"; update the ERR_FAIL_COND_MSG invocation (the one
checking RD::get_singleton()->render_pipeline_is_valid/compute_pipeline_is_valid
for pipeline) to remove the extra space so the message reads "must be called
manually before the dependent shader is freed." and keep the rest of the
condition unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 92b0153c-8c64-46e4-acbf-def428ec2531
📒 Files selected for processing (1)
servers/rendering/renderer_rd/pipeline_deferred_rd.h
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
servers/rendering/renderer_rd/pipeline_deferred_rd.h (1)
131-131:⚠️ Potential issue | 🔴 CriticalUse
free()instead offree_rid()at Line 131.
RD::get_singleton()->free_rid(pipeline)does not match the C++RenderingDevicefree API and can fail to compile. Usefree(pipeline)here.🔧 Proposed fix
- RD::get_singleton()->free_rid(pipeline); + RD::get_singleton()->free(pipeline);#!/bin/bash # Verify free API names on RenderingDevice and current usage sites. rg -n -C2 '\bvoid\s+free\s*\(\s*RID\b|\bfree_rid\s*\(' servers/rendering/rendering_device.h rg -n -C2 '\bfree_rid\s*\(' --type=cpp --type=h servers/rendering/Expected result:
servers/rendering/rendering_device.hdeclaresfree(RID ...).- No valid C++ member declaration for
free_rid(...)in RenderingDevice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h` at line 131, The call RD::get_singleton()->free_rid(pipeline) uses a non-existent RenderingDevice API and will not compile; replace it with the correct method RD::get_singleton()->free(pipeline). Update the invocation referencing the RID variable pipeline and remove free_rid usages (search for free_rid to ensure there are no other incorrect calls), making sure the argument remains the same RID type expected by RenderingDevice::free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@servers/rendering/renderer_rd/pipeline_deferred_rd.h`:
- Line 131: The call RD::get_singleton()->free_rid(pipeline) uses a non-existent
RenderingDevice API and will not compile; replace it with the correct method
RD::get_singleton()->free(pipeline). Update the invocation referencing the RID
variable pipeline and remove free_rid usages (search for free_rid to ensure
there are no other incorrect calls), making sure the argument remains the same
RID type expected by RenderingDevice::free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9eba374c-0e9d-4b8a-9dfe-cfbaf67886d2
📒 Files selected for processing (1)
servers/rendering/renderer_rd/pipeline_deferred_rd.h
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.
TL;DR
pipeline_deffered_rd.hfor the motion blur PR (Add motion blur support #1201)Note
Contributed by 2LazyDevs.
Summary by CodeRabbit