refactor!: make Resource.GetOriginal() nullable and add RequireOriginal() - #2205
Open
yuto-trd wants to merge 2 commits into
Open
refactor!: make Resource.GetOriginal() nullable and add RequireOriginal()#2205yuto-trd wants to merge 2 commits into
yuto-trd wants to merge 2 commits into
Conversation
…al() A resource built through its public constructor rather than through EngineObject.ToResource() has no backing engine object, but GetOriginal() declared a non-nullable return and handed out a null. In-tree production code already mints such detached resources -- Color.ToBrushResource() reached from TextElementsBuilder, the SolidColorBrush.Resource and Pen.Resource that FormattedTextParser builds for a stroke tag, and the GradientStop.Resource the Avalonia editor adapters build -- so the declared type was a lie at every one of those call sites. GetOriginal() now returns a nullable reference and RequireOriginal() throws InvalidOperationException when the resource is detached. Call sites that dispatch to the backing object use RequireOriginal(), because a detached resource cannot serve the call; call sites that compare identity or tolerate null keep GetOriginal(). IsAttached exposes the same distinction without forcing a null check. The generated per-type Resource class mirrors both members, so a generated GetOriginal() is nullable and a generated RequireOriginal() is not. BREAKING CHANGE: EngineObject.Resource.GetOriginal() now returns a nullable reference, and the generated per-type override does the same. Call sites that dispatch to the backing engine object should call RequireOriginal() instead, which throws when the resource is detached. Call sites that compare identity or already tolerate null can keep GetOriginal() and handle the null. Affects Beutl.Engine, Beutl.NodeGraph, Beutl.Editor.Components, and Beutl.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesOriginal-resource access
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Code Review BotNo comment/code divergences or documentation drift detected. Reviewed 46 file(s); skipped 1. |
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix(nodegraph): drop the unused original..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.NodeGraph/Nodes/Group/GroupInput.cs`:
- Line 74: Remove the unused RequireOriginal() call and its node variable from
GroupInput.Resource.Update, allowing detached resources with OuterInputValues to
continue assigning those values to ItemValues without requiring an attached
original.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00d56cdc-15ad-4164-a320-5d27f653a96c
📒 Files selected for processing (47)
src/Beutl.Editor.Components/PathEditorTab/ViewModels/PathEditorViewModel.cssrc/Beutl.Editor.Components/PathEditorTab/Views/PathEditorTabView.axaml.cssrc/Beutl.Engine.SourceGenerators/Emit/ResourceClassEmitter.cssrc/Beutl.Engine/Audio/Composing/Composer.cssrc/Beutl.Engine/Audio/SoundGroup.cssrc/Beutl.Engine/Engine/EngineObject.cssrc/Beutl.Engine/Graphics/AudioVisualizers/AudioVisualizerDrawable.cssrc/Beutl.Engine/Graphics/BrushConstructor.cssrc/Beutl.Engine/Graphics/DrawablePresenter.cssrc/Beutl.Engine/Graphics/DrawableTimeController.cssrc/Beutl.Engine/Graphics/FilterEffects/DelayAnimationEffect.cssrc/Beutl.Engine/Graphics/FilterEffects/DisplacementMapEffect.cssrc/Beutl.Engine/Graphics/FilterEffects/FilterEffectGroup.cssrc/Beutl.Engine/Graphics/FilterEffects/FilterEffectPresenter.cssrc/Beutl.Engine/Graphics/ImmediateCanvas.cssrc/Beutl.Engine/Graphics/Particles/ParticleRenderNode.cssrc/Beutl.Engine/Graphics/Rendering/FilterEffectRenderNode.cssrc/Beutl.Engine/Graphics/Rendering/GraphicsContext2D.cssrc/Beutl.Engine/Graphics/Rendering/Renderer.cssrc/Beutl.Engine/Graphics3D/Meshes/Mesh.cssrc/Beutl.Engine/Graphics3D/Textures/DrawableTextureSource.cssrc/Beutl.Engine/Media/Geometry/Geometry.cssrc/Beutl.Engine/Media/Geometry/PathFigure.cssrc/Beutl.Engine/Media/Geometry/PathGeometry.cssrc/Beutl.Engine/Media/TextFormatting/FormattedText.cssrc/Beutl.NodeGraph/Composition/GraphSnapshot.cssrc/Beutl.NodeGraph/Nodes/ConfigureNode.cssrc/Beutl.NodeGraph/Nodes/FactoryNode.cssrc/Beutl.NodeGraph/Nodes/FilterEffectNode.cssrc/Beutl.NodeGraph/Nodes/GeometryNode.cssrc/Beutl.NodeGraph/Nodes/Group/GroupInput.cssrc/Beutl.NodeGraph/Nodes/Group/GroupNode.cssrc/Beutl.NodeGraph/Nodes/LayerInputNode.cssrc/Beutl.NodeGraph/Nodes/TextNode.cssrc/Beutl.NodeGraph/Nodes/TransformNode.cssrc/Beutl.NodeGraph/Nodes/Utilities/ExpressionNode.cssrc/Beutl.NodeGraph/Nodes/Utilities/MatrixNode.cssrc/Beutl.NodeGraph/Nodes/Utilities/PreviewNode.cssrc/Beutl.NodeGraph/Nodes/Utilities/TimeNode.cssrc/Beutl/Helpers/AvaloniaTypeConverter.cstests/Beutl.UnitTests/Engine/DetachedResourceTests.cstests/Beutl.UnitTests/Engine/FormattedTextDisposalTests.cstests/Beutl.UnitTests/Engine/Graphics/Rendering/Golden/GoldenImageHarness.cstests/Beutl.UnitTests/Engine/Graphics/Rendering/SourceEffectiveScaleFlowTests.cstests/Beutl.UnitTests/Engine/Graphics/Rendering/StrokeEffectOffsetBoundsTests.cstests/Beutl.UnitTests/NodeGraph/GraphSnapshotTests.cstests/SourceGeneratorTest/EngineObject.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors EngineObject.Resource so the backing engine object relationship is correctly represented in the type system: GetOriginal() becomes nullable for detached resources, and new APIs (IsAttached, RequireOriginal()) make “must be attached” call sites explicit. This propagates through generated resource types and updates in-tree call sites and tests accordingly.
Changes:
- Updated
EngineObject.Resourceto exposeIsAttached, makeGetOriginal()nullable, and addRequireOriginal()that throws on detached resources. - Updated source generator output to mirror the new nullable/required accessors and to use
RequireOriginal()where dispatch is required. - Updated call sites across Engine/Editor/NodeGraph plus added unit tests covering detached vs attached behavior.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SourceGeneratorTest/EngineObject.cs | Updates generator test fixture to match nullable GetOriginal() and new RequireOriginal() contract. |
| tests/Beutl.UnitTests/NodeGraph/GraphSnapshotTests.cs | Uses RequireOriginal() for resource-backed node dispatch in tests. |
| tests/Beutl.UnitTests/Engine/Graphics/Rendering/StrokeEffectOffsetBoundsTests.cs | Uses RequireOriginal() to safely dispatch to effect implementation. |
| tests/Beutl.UnitTests/Engine/Graphics/Rendering/SourceEffectiveScaleFlowTests.cs | Uses RequireOriginal() for filter effect dispatch during scale-flow test. |
| tests/Beutl.UnitTests/Engine/Graphics/Rendering/Golden/GoldenImageHarness.cs | Uses RequireOriginal() when rendering via drawable original. |
| tests/Beutl.UnitTests/Engine/FormattedTextDisposalTests.cs | Uses RequireOriginal() when accessing original glyph path owners. |
| tests/Beutl.UnitTests/Engine/DetachedResourceTests.cs | Adds tests for detached resource behavior and typing of generated GetOriginal(). |
| src/Beutl/Helpers/AvaloniaTypeConverter.cs | Switches adapter dispatch paths to RequireOriginal(). |
| src/Beutl.NodeGraph/Nodes/Utilities/TimeNode.cs | Uses RequireOriginal() for node update dispatch. |
| src/Beutl.NodeGraph/Nodes/Utilities/PreviewNode.cs | Uses RequireOriginal() for node update dispatch. |
| src/Beutl.NodeGraph/Nodes/Utilities/MatrixNode.cs | Uses RequireOriginal() for node update dispatch. |
| src/Beutl.NodeGraph/Nodes/Utilities/ExpressionNode.cs | Uses RequireOriginal() for node update dispatch. |
| src/Beutl.NodeGraph/Nodes/TransformNode.cs | Uses RequireOriginal() for node update dispatch. |
| src/Beutl.NodeGraph/Nodes/TextNode.cs | Uses RequireOriginal() for node update dispatch. |
| src/Beutl.NodeGraph/Nodes/LayerInputNode.cs | Uses RequireOriginal() for node update dispatch. |
| src/Beutl.NodeGraph/Nodes/Group/GroupNode.cs | Uses RequireOriginal() for initialize/uninitialize/update dispatch. |
| src/Beutl.NodeGraph/Nodes/Group/GroupInput.cs | Uses RequireOriginal() for update dispatch. |
| src/Beutl.NodeGraph/Nodes/GeometryNode.cs | Uses RequireOriginal() for update dispatch. |
| src/Beutl.NodeGraph/Nodes/FilterEffectNode.cs | Uses RequireOriginal() for update dispatch. |
| src/Beutl.NodeGraph/Nodes/FactoryNode.cs | Uses RequireOriginal() for update dispatch. |
| src/Beutl.NodeGraph/Nodes/ConfigureNode.cs | Uses RequireOriginal() for update dispatch. |
| src/Beutl.NodeGraph/Composition/GraphSnapshot.cs | Uses RequireOriginal() where snapshot logic must access node members. |
| src/Beutl.Engine/Media/TextFormatting/FormattedText.cs | Uses RequireOriginal() for path disposal and glyph path updates. |
| src/Beutl.Engine/Media/Geometry/PathGeometry.cs | Uses RequireOriginal() when applying child figures into geometry contexts. |
| src/Beutl.Engine/Media/Geometry/PathFigure.cs | Uses RequireOriginal() when applying segments into geometry contexts. |
| src/Beutl.Engine/Media/Geometry/Geometry.cs | Uses RequireOriginal() when rebuilding cached paths from original geometry. |
| src/Beutl.Engine/Graphics3D/Textures/DrawableTextureSource.cs | Uses RequireOriginal() when rendering drawable into texture. |
| src/Beutl.Engine/Graphics3D/Meshes/Mesh.cs | Uses RequireOriginal() when applying mesh data to cached buffers. |
| src/Beutl.Engine/Graphics/Rendering/Renderer.cs | Uses RequireOriginal() for drawable dispatch and z-index boundary filtering. |
| src/Beutl.Engine/Graphics/Rendering/GraphicsContext2D.cs | Uses RequireOriginal() when drawing a drawable resource. |
| src/Beutl.Engine/Graphics/Rendering/FilterEffectRenderNode.cs | Uses RequireOriginal() when applying filter effects during render processing. |
| src/Beutl.Engine/Graphics/Particles/ParticleRenderNode.cs | Uses RequireOriginal() when rendering drawables into intermediate targets. |
| src/Beutl.Engine/Graphics/ImmediateCanvas.cs | Uses RequireOriginal() for immediate-mode drawable rendering. |
| src/Beutl.Engine/Graphics/FilterEffects/FilterEffectPresenter.cs | Uses RequireOriginal() when dispatching to nested effect target. |
| src/Beutl.Engine/Graphics/FilterEffects/FilterEffectGroup.cs | Uses RequireOriginal() when dispatching child filter effects. |
| src/Beutl.Engine/Graphics/FilterEffects/DisplacementMapEffect.cs | Uses RequireOriginal() when applying transform-backed displacement mapping. |
| src/Beutl.Engine/Graphics/FilterEffects/DelayAnimationEffect.cs | Uses RequireOriginal() when dispatching delayed child effect. |
| src/Beutl.Engine/Graphics/DrawableTimeController.cs | Uses RequireOriginal() when dispatching render/measure to target drawable. |
| src/Beutl.Engine/Graphics/DrawablePresenter.cs | Uses RequireOriginal() when dispatching render/measure to target drawable. |
| src/Beutl.Engine/Graphics/BrushConstructor.cs | Uses RequireOriginal() when rendering a drawable brush into paint/targets. |
| src/Beutl.Engine/Graphics/AudioVisualizers/AudioVisualizerDrawable.cs | Uses RequireOriginal() when composing samples from the sound source. |
| src/Beutl.Engine/Engine/EngineObject.cs | Implements nullable original storage + IsAttached/GetOriginal()/RequireOriginal() contract. |
| src/Beutl.Engine/Audio/SoundGroup.cs | Uses RequireOriginal() when composing child sounds. |
| src/Beutl.Engine/Audio/Composing/Composer.cs | Uses RequireOriginal() when composing from sound resource. |
| src/Beutl.Engine.SourceGenerators/Emit/ResourceClassEmitter.cs | Emits nullable GetOriginal() plus non-null RequireOriginal() and updates node-port binding generation. |
| src/Beutl.Editor.Components/PathEditorTab/Views/PathEditorTabView.axaml.cs | Uses RequireOriginal() when converting geometry to SVG path for background display. |
| src/Beutl.Editor.Components/PathEditorTab/ViewModels/PathEditorViewModel.cs | Uses RequireOriginal() for transform matrix and figure-context lookup. |
Suppressed comments (1)
tests/Beutl.UnitTests/Engine/DetachedResourceTests.cs:25
- This test creates a
SolidColorBrush.Resource(IDisposable) without disposing it. Use ausingdeclaration to keep the tests leak-free.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
GroupInput.Resource.Update read the backing node and never used it. After GetOriginal() gained RequireOriginal(), that dead lookup became a throw: a detached resource carrying OuterInputValues could no longer propagate them to ItemValues even though the copy loop needs nothing from the backing node. Also dispose the detached resources the new tests construct.
Contributor
|
No TODO comments were found. |
Contributor
Minimum allowed line rate is |
8 tasks
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
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.
Description
EngineObject.Resource.GetOriginal()declared a non-nullable return type but could hand outnull. OnlyUpdateattaches a backing engine object, so a resource built through its public constructor rather than throughToResource()is detached — and in-tree production code already mints and consumes exactly those:Color.ToBrushResource(), reached fromTextElementsBuilderSolidColorBrush.ResourceandPen.ResourcethatFormattedTextParserbuilds for a stroke tagGradientStop.Resourcethe Avalonia editor adapters buildAt each of those call sites the declared type was a lie, and the nullable analysis had nothing to warn about.
This change splits the accessor into the two things call sites actually want:
GetOriginal()returns a nullable reference, for call sites that compare identity or already toleratenull.RequireOriginal()throwsInvalidOperationExceptionwhen the resource is detached, for call sites that dispatch to the backing object and cannot proceed without it.IsAttachedexposes the same distinction without forcing a null check.Every existing call site was classified individually rather than mechanically rewritten. The ~47 remaining
GetOriginal()uses are identity comparisons (ResourceReconciler, the shape/mesh cache-invalidation checks,GraphSnapshot.FindSlotIndex) or null-tolerant lookups (PlayerViewgizmo hit-testing,Scene3DRenderNode); the dispatch sites moved toRequireOriginal().The source generator mirrors both members onto the generated per-type
Resource, so a generatedGetOriginal()is nullable and a generatedRequireOriginal()is not. GeneratedBindNodePortValuesusesRequireOriginal(), since port binding cannot run against a detached resource.Affected 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 touches
Beutl.Engine.SourceGenerators(the generatedResourceclass shape).Breaking changes
EngineObject.Resource.GetOriginal()now returns a nullable reference, and the generated per-type override does the same.Migration:
RequireOriginal(), which throws when the resource is detached.nullcan keepGetOriginal()and handle thenull.IsAttachedis available when the caller wants to branch without a null check.This is a source-breaking change for out-of-tree plugins that call
GetOriginal()in a non-nullable context. It surfaces anullthose callers could already receive at runtime, so the fix is a compile-time signal rather than a behavior change.Test plan
tests/Beutl.UnitTests/Engine/DetachedResourceTests.cscovering the new contract:ToBrushResource()produces a detached resource (IsAttached == false,GetOriginal() == null),RequireOriginal()throws on a detached resource,ToResource()produces an attached one, and the generatedGetOriginal()stays typed to the declaring engine object.dotnet build Beutl.slnx— 0 warnings, 0 errors. The nullable analysis produced 32 warnings after the signature change; each was classified as dispatch or comparison and resolved individually rather than suppressed.dotnet test tests/Beutl.UnitTests— 4978 passed, 0 failed, 3 skipped.dotnet test tests/SourceGeneratorTest— 11 passed, 0 failed.Fixed issues / References
Split out of the feature 004 (GPU pass fusion) stack so it can be reviewed on its own merits; it has no dependency on that feature's rendering work.
Summary by CodeRabbit
New Features
Bug Fixes
Tests