refactor(engine)!: GPU pass fusion stack 2/5 — rendering pipeline - #2167
refactor(engine)!: GPU pass fusion stack 2/5 — rendering pipeline#2167yuto-trd wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Confidence Score: 4/5The PR does not appear safe to merge while repaired recovered elements can still silently lose valid edits. The displayed reply, whose author is blank, says PR #2159 supersedes this recovery implementation, but current HEAD still sets IsStorageWriteSuppressed permanently and all explicit-save and autosave StoreToUri calls continue returning without persisting repaired elements. Files Needing Attention: src/Beutl.ProjectSystem/ProjectSystem/Scene.cs, src/Beutl.Core/Serialization/CoreSerializer.cs, src/Beutl.Editor/AutoSaveService.cs Important Files Changed
Reviews (17): Last reviewed commit: "refactor(engine)!: stack 2/5 — record-th..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ef886221b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3ef8862 to
8cb333b
Compare
8cb333b to
f23b914
Compare
f23b914 to
4f7cdfe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f7cdfe182
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4f7cdfe to
c8a396b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8a396b9ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
c8a396b to
ed9bda0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed9bda004a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ed9bda0 to
7bbb011
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bbb0111ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7bbb011 to
31992b2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31992b2473
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
31992b2 to
01a0310
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01a03104b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
01a0310 to
fdc8f99
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdc8f99a91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fdc8f99 to
959e4d4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 959e4d45cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| EnsureFallbackProjection(fallback); | ||
| } | ||
|
|
||
| MarkRecoveredElement(element, rawText); |
There was a problem hiding this comment.
When a valid element contains only a nested fallback object, this marks the entire successfully parsed Element as storage-write-suppressed. Subsequent edits to the element's valid fields or healthy sibling objects are therefore skipped by Scene.Serialize, so Save reports success but reopening restores their old values. Reserve raw-sidecar suppression for wholly unreadable element JSON, or serialize parsed elements while retaining each fallback's preserved Json.
Useful? React with 👍 / 👎.
959e4d4 to
85b3797
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85b37979ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -84,10 +224,12 @@ static void SafeStep(string step, Action action) | |||
|
|
|||
| _isDisposed = true; | |||
| SafeStep(nameof(OnDispose), () => OnDispose(false)); | |||
| SafeStep(nameof(_frameRenderer), () => _frameRenderer?.Dispose()); | |||
There was a problem hiding this comment.
If a GPU-backed Renderer is abandoned without an explicit Dispose(), this finalizer invokes RenderNodeRenderer.Dispose() directly on the finalizer thread. That disposes _targetRegistry, whose pool evicts retained RenderTarget backend resources and explicitly requires renderer-thread disposal, risking context-thread violations or native failures. The earlier disposal fix only dispatches the normal Dispose() path; this finalizer call is fresh evidence that not all render-owned cleanup is marshalled. Queue _frameRenderer cleanup through RenderThread.Dispatcher, like the entry finalizer does.
Useful? React with 👍 / 👎.
| var entries = _nodeCache.ToArray(); | ||
| _nodeCache.Clear(); | ||
| _allCurrentEntries.Clear(); |
There was a problem hiding this comment.
When ClearAllCaches() runs after a frame that required intermediate targets or compiled shaders, this clears only the per-drawable entries and leaves _frameRenderer untouched. The per-entry renderers are used for metadata queries, while _frameRenderer.Render() owns the frame execution pool, structural plans, and GPU program cache, so its retained targets and programs survive a call whose purpose is to release all caches (including the call from OutputViewModel.ClearEditViewModelCaches). Dispose and recreate the frame renderer here, or otherwise explicitly clear all of its owned caches without changing CacheOptions.
Useful? React with 👍 / 👎.
85b3797 to
9fbd500
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fbd5003ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| return element; | ||
| } | ||
| catch (JsonException ex) |
There was a problem hiding this comment.
When a syntactically valid sidecar has a known but non-Element top-level discriminator, CoreSerializer.RestoreFromUri<Element> creates that type and the generic cast throws InvalidCastException; other top-level conversion failures can likewise escape as non-JsonException types. This catch therefore misses the failure and one malformed .belm still aborts opening the entire project instead of producing the disabled recovery element. Handle the relevant deserialization/type failures here (or validate assignability in CoreSerializer) without swallowing unrelated I/O errors.
Useful? React with 👍 / 👎.
| internal static async Task DisposeThumbnailRenderResourcesAsync(params IDisposable?[] resources) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(resources); | ||
| await RenderThread.Dispatcher.InvokeAsync(() => |
There was a problem hiding this comment.
When thumbnail enumeration is finalized after RenderThread.Dispatcher has begun shutting down, this InvokeAsync enqueues work and awaits it with a non-cancelable token. The repository dispatcher sets _running to false before raising ShutdownStarted and never drains newly queued operations afterward, so the returned task never completes and application shutdown can hang while thumbnails are active. Arrange cleanup before shutdown or explicitly handle the shutdown state without awaiting an operation that cannot run.
Useful? React with 👍 / 👎.
9fbd500 to
0ee15b3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ee15b3fbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0ee15b3 to
ebe4bc1
Compare
|
No TODO comments were found. |
ebe4bc1 to
5564e90
Compare
|
No TODO comments were found. |
1 similar comment
|
No TODO comments were found. |
yuto-trd
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5564e907b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Stacked-PR slice 2 of 5: RenderNode.Process records into RenderNodeContext as the sole transactional recorder; sealed RenderFragmentHandle replaces the executable RenderNodeOperation lifecycle; recording lowers into metadata, ROI, cache substitution, island planning, fusion, and one execution; consumers use the disposable RenderNodeRenderer with immutable per-call RenderNodeRenderRequest descriptors. All in-tree RenderNode consumers migrate in this slice, existing tests are migrated, the public authoring surface is pinned by tests/Beutl.PublicApiContractTests, and the authoring guidance that documents the landed API lands here with it. Existing FilterEffect call sites retain their legacy CustomEffect placement contract. BREAKING CHANGE: Beutl.Engine and out-of-tree Engine/plugin RenderNode implementations must migrate from RenderNodeOperation[] Process(RenderNodeContext) to void Process(RenderNodeContext), publish context-owned RenderFragmentHandle values, and use the high-level request APIs (RenderNodeRenderer with RenderNodeRenderRequest); see docs/specs/004-gpu-pass-fusion/contracts/breaking-changes.md. BREAKING CHANGE: RenderNodeRenderRequest.UseRenderCache is removed; use CacheOptions = RenderCacheOptions.Enabled/Disabled, or new RenderCacheOptions(enabled, rules) for custom admission rules. BREAKING CHANGE: Beutl.Engine target captures now use TargetCaptureScaleContract instead of RenderScaleContract; migrate to MaterializeAtWorkingScale, Custom, or PreserveTargetSupply. BREAKING CHANGE: Beutl.Engine custom ShaderBindingBuilder binder overloads replace RenderRuntimeIdentity with ShaderBindingCachePolicy; use RequestUnique or the non-capturing ReuseFromSnapshot policy.
5564e90 to
080fa9e
Compare
|
No TODO comments were found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 080fa9e444
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| executionCanvas.Transform = destination.Transform; | ||
| executionCanvas.Opacity = destination.Opacity; | ||
| executionCanvas.BlendMode = destination.BlendMode; |
There was a problem hiding this comment.
When target access expands ExecutionTargetBounds beyond SelectedOutputBounds and the caller has an active PushClip, this offscreen canvas receives the destination transform, opacity, and blend mode but not its clip; the clip is applied only during the final commit. Consequently, an earlier target command can write outside the caller's clip and a later blur, backdrop, or other sampling stage can feed those pixels back inside it, unlike direct rendering and despite Render() promising to preserve the active clip. Mirror the destination clip onto the execution canvas before executing the graph.
Useful? React with 👍 / 👎.
| foreach (Entry entry in pendingEntries) | ||
| { | ||
| RevalidateAll(entry.Node); | ||
| entry.InvalidateBounds(); | ||
| _allCurrentEntries.Add(entry); |
There was a problem hiding this comment.
When RevalidateAll throws while invalidating a cached target—for example because a custom/backend render target fails during disposal—the frame has already rendered and _allCurrentEntries was cleared, but this loop has published only the entries preceding the failure. Subsequent boundary queries therefore expose a partial frame. Fresh evidence beyond the fixed preparation and UpdateFrame paths is this separate post-render publication loop; revalidate every pending entry first, then add the complete list atomically.
Useful? React with 👍 / 👎.
| CaptureCleanupFailure( | ||
| () => RenderThread.Dispatcher.Invoke( | ||
| () => renderResourceFailure = DisposeRenderResourcesCore()), | ||
| ref primary); |
There was a problem hiding this comment.
When a Renderer is disposed from a non-render thread after RenderThread.Dispatcher has begun shutting down—for example when a late export continuation releases its SceneRenderer during application teardown—this non-cancelable Invoke enqueues cleanup and waits forever. QueueSynchronizationContext.Shutdown() sets _running to false before raising ShutdownStarted, and its execution loop never drains operations posted afterward, so this can hang shutdown. Fresh evidence beyond the render-thread marshaling fix is that the synchronous disposal path does not check HasShutdownStarted; arrange disposal before shutdown or avoid waiting on an operation that can no longer run.
Useful? React with 👍 / 👎.
Description
Stacked-PR slice 2/5 of
speckit/004-gpu-pass-fusion(204 files, +49,286/-4,000).This slice replaces executable render-node pulling with transactional
RenderNodeContextrecording, complete-request DAG planning, ROI propagation, cache substitution, execution-island planning, typed Shader/Geometry lowering, renderer-wide CurrentPixel fusion, and one ordered executor. All in-tree render-node consumers migrate with the public API change.FilterEffect.ApplyTo(FilterEffectContext, Resource)remains the authoring entry point. Existing FilterEffect operation calls remain available, whileFilterEffectContext.ShaderandGeometryprovide the new deferred opt-in surface. Built-in FilterEffect call sites are intentionally not migrated here; that work is isolated in slice 5. Blur and DropShadow retain their legacy implementations throughout the stack.Stack
speckit/004-s1-spec— specification and contractsspeckit/004-s3-tests— pipeline and rendering test suitesspeckit/004-s4-evidence— benchmarks, paired evidence, and acceptancespeckit/004-s5-shader-migration— built-in Shader migration, excluding Blur and DropShadowAffected areas
Beutl.Engine(rendering / scene / track)Beutl.ProjectSystem(project / document persistence)Beutl.Editor,Beutl.Editor.Components,Beutl.Controls)Beutl.Extensibility(plugin abstractions)Beutl.NodeGraph(node editor)Beutl.FFmpegIpc/Beutl.FFmpegWorker(media IPC boundary)Beutl.Api(server API client)Also affects
Beutl.AgentToolkitand application render consumers.Breaking changes
Custom
RenderNode.Processimplementations and executableRenderNodeOperationconsumers migrate tovoid Process(RenderNodeContext), context-owned fragment handles, andRenderNodeRenderer. Seedocs/specs/004-gpu-pass-fusion/contracts/breaking-changes.mdin slice 1.Ordinary FilterEffect operation calls remain source-compatible unless they directly consumed removed operation-backed
EffectTargetmembers.Test plan
Fixed issues / References