refactor!: read generated resource defaults from declaration state - #2210
Open
yuto-trd wants to merge 1 commit into
Open
refactor!: read generated resource defaults from declaration state#2210yuto-trd wants to merge 1 commit into
yuto-trd wants to merge 1 commit into
Conversation
…state A detached generated Resource started every value property at default(T) regardless of what the owner declared, so a resource built through its public constructor disagreed with the same owner's declared defaults. Nothing made the generator's assumption explicit either: it needed the owner's IProperty instances to be readable without running a user constructor, but silently produced wrong defaults when they were not. Generated resources now chain through the owner's declared defaults. A concrete generated Resource() reads them from __CreateResourceDefaultValues(), the attached ToResource path uses a separate constructor that skips default evaluation it would immediately overwrite, and EngineObject.Resource exposes the matching protected constructors. Four diagnostics make the previously implicit requirement checkable. BESG003 reports a property whose IProperty is not available from declaration-time state or is replaced in a constructor, and BESG004 reports a primary constructor on that automatic path. [ResourceDefaultValuesProvider] is the escape hatch for both: BESG005 rejects an invalid or ambiguous provider, and BESG006 requires a derived type to declare its own rather than inherit one that would evaluate the base owner's defaults. BESG004 already fires on in-tree code: FaultingDrawable in RendererExceptionSafetyTests used a primary constructor, and is migrated to an ordinary one here. BREAKING CHANGE: A resource-generating EngineObject subclass without an explicit defaults provider must expose every generated IProperty from stable declaration-time state - an auto-property with a declaration initializer, or a computed getter returning a declaration-initialized readonly field - and no ordinary constructor may replace that storage. BESG003 reports unsupported or constructor-replaced storage and BESG004 reports a primary constructor on this path. Authors move property creation to a supported declaration shape, move primary-constructor logic to an ordinary constructor, or declare exactly one [ResourceDefaultValuesProvider] static parameterless non-generic method returning the declaring owner; BESG005 rejects an invalid or ambiguous provider and BESG006 requires each generated derived type in a provider-backed hierarchy to declare its own. Suppressing generation and implementing the Resource / ToResource contract manually remains the alternative. BREAKING CHANGE: Generated abstract Resource types no longer expose a protected parameterless constructor. A hand-written or generation-suppressed attached resource must chain to base(skipDefaultInitialization: true) before its first Update; a hand-written detached resource that promises declared-default parity chains to base(defaultValues). This replaces the implicit base() path that left abstract-base properties at default(T). The attached-only ParticleEmitter, ShakeEffect, DelayAnimationEffect, NodeGraphDrawable, NodeGraphFilterEffect, and RenderNodeDrawable Resource parameterless constructors are now internal; callers construct the owner and call ToResource(CompositionContext) instead of exposing a pre-Update resource. Affects Beutl.Engine and Beutl.NodeGraph.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 |
Contributor
|
No TODO comments were found. |
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defects identified. The generated detached and attached constructor paths remain distinct, inherited resource defaults flow through the generated base chain, and the affected handwritten resources explicitly adopt the new attached-only constructor contract. Important Files Changed
Reviews (1): Last reviewed commit: "refactor(engine)!: read generated resour..." | Re-trigger Greptile |
Code Review BotNo comment/code divergences or documentation drift detected. Reviewed 21 file(s); skipped 0. |
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
1 participant
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
A detached generated
Resourcestarted every value property atdefault(T)regardless of what the owner declared. A resource built through its public constructor therefore disagreed with the same owner's declared defaults — aSolidColorBrush.Resourcebuilt directly did not carry theOpacityitsIPropertydeclares.The generator also had an unstated requirement. To read those defaults it needs the owner's
IPropertyinstances to be available without running a user constructor. Nothing checked that, so a type whose properties were built in a constructor silently produced wrong defaults rather than failing.This change does two things:
Generated resources chain through the declared defaults. A concrete generated
Resource()reads them from__CreateResourceDefaultValues(). The attachedToResourcepath uses a separate constructor that skips evaluating defaults it would immediately overwrite.EngineObject.Resourcegains the matching protected constructors.Four diagnostics make the implicit requirement checkable.
BESG003IPropertynot available from declaration-time state, or replaced in a constructorBESG004BESG005[ResourceDefaultValuesProvider]BESG006[ResourceDefaultValuesProvider]is the escape hatch for the first two: a static parameterless non-generic method returning the declaring owner. It is what a primary-constructor type uses to keep generation.BESG004already fires on in-tree code —FaultingDrawableinRendererExceptionSafetyTestsused a primary constructor — which is migrated to an ordinary constructor here.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.Breaking changes
Declaration-time defaults are required. A resource-generating
EngineObjectsubclass without an explicit provider must expose every generatedIPropertyfrom stable declaration-time state: an auto-property with a declaration initializer, or a computed getter returning a declaration-initialized readonly field, with no ordinary constructor replacing that storage.Migration: move property creation to a supported declaration shape, move primary-constructor logic to an ordinary constructor, or declare one
[ResourceDefaultValuesProvider]static parameterless non-generic method returning the declaring owner. Suppressing generation and implementingResource/ToResourcemanually remains the alternative.Generated abstract
Resourcetypes no longer expose a protected parameterless constructor. A hand-written or generation-suppressed attached resource chains tobase(skipDefaultInitialization: true)before its firstUpdate; a hand-written detached resource that promises declared-default parity chains tobase(defaultValues). This replaces the implicitbase()path that left abstract-base properties atdefault(T).The attached-only
ParticleEmitter.Resource,ShakeEffect.Resource,DelayAnimationEffect.Resource,NodeGraphDrawable.Resource,NodeGraphFilterEffect.Resource, andRenderNodeDrawable.Resourceparameterless constructors are now internal. Callers construct the owner and callToResource(CompositionContext)rather than exposing a pre-Updateresource.Test plan
tests/SourceGeneratorTest/ResourceDefaultValuesTests.cs(7 tests): the generated constructor chain and per-property default seeding, the attached fast path, one scenario per diagnostic (BESG003–BESG006), and a valid-provider case proving the escape hatch keeps generation working for a primary-constructor type.dotnet build Beutl.slnx— 0 warnings, 0 errors.dotnet test tests/Beutl.UnitTests— 4978 passed, 0 failed, 3 skipped.dotnet test tests/SourceGeneratorTest— 18 passed, 0 failed.Fixed issues / References
Split out of the feature 004 (GPU pass fusion) stack. The generator changes reference no feature-004 type, so this stands on its own; the ownership-transfer seam that shipped alongside it in that branch is deliberately not included here.