-
Notifications
You must be signed in to change notification settings - Fork 2
Dev #156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dev #156
Conversation
📝 WalkthroughWalkthroughThis change adds a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Runtime/Scripts/Scene/Components/BanterMaterial.cs (1)
40-40: Consider adding a tooltip for consistency.Other serialized fields in this class include
[Tooltip(...)]attributes for documentation. Consider adding one forcacheBustto explain its purpose.Suggested change
- [See(initial = "")][SerializeField] internal string cacheBust = ""; + [Tooltip("A value used to invalidate material caching. Changing this value forces a new material instance.")] + [See(initial = "")][SerializeField] internal string cacheBust = "";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
OraPackage/com.sidequest.ora.zipis excluded by!**/*.zip
📒 Files selected for processing (5)
Editor/Components/BanterMaterialEditor.csEditor/Scripts/VisualScripting/VsStubsAllowed.csRuntime/Scripts/Scene/BanterComponentProperty/BanterComponentPropertyNames.csRuntime/Scripts/Scene/Components/BanterMaterial.cspackage.json
🧰 Additional context used
🧬 Code graph analysis (1)
Runtime/Scripts/Scene/Components/BanterMaterial.cs (3)
Runtime/Scripts/Scene/Components/BanterAttachedObject.cs (1)
UpdateCallback(103-146)Runtime/Scripts/Scene/Components/BanterVideoPlayer.cs (2)
UpdateCallback(94-97)GetSignature(258-261)Runtime/Scripts/Scene/BanterScene.cs (1)
List(1456-1544)
🔇 Additional comments (8)
package.json (1)
3-3: LGTM!The patch version bump to 3.2.1 is appropriate for this backward-compatible addition of the
cacheBustproperty.Runtime/Scripts/Scene/BanterComponentProperty/BanterComponentPropertyNames.cs (1)
175-175: LGTM!The new
cacheBustenum member is correctly added and follows the existing naming convention.Runtime/Scripts/Scene/Components/BanterMaterial.cs (5)
90-128: Verify:cacheBustproperty changes have no effect inSetupMaterial.The
cacheBustproperty is included inReSetup,GetSignature, deserialization, and sync, butSetupMaterialdoesn't handlePropertyName.cacheBust. This means changingcacheBustvia the public setter will triggerUpdateCallback, but nothing will actually happen since none of the conditional blocks respond to it.If the intent is to force a texture reload when
cacheBustchanges, consider handling it alongsidetexture:- if (changedProperties?.Contains(PropertyName.texture) ?? false) + if (changedProperties?.Contains(PropertyName.texture) ?? changedProperties?.Contains(PropertyName.cacheBust) ?? false)Otherwise, if
cacheBustis only meant to affect the signature for external purposes, this may be intentional.
197-197: LGTM!The public accessor follows the established pattern for triggering updates when the property changes.
218-226: LGTM!
ReSetupandGetSignaturecorrectly includecacheBustin their property lists, consistent with the other component properties.
314-322: LGTM!Deserialization for
cacheBustfollows the existing pattern for string properties, correctly casting toBanterStringand checking the property name.
390-401: LGTM!
SyncPropertiescorrectly includes thecacheBustproperty update with the appropriate type (PropertyType.String).Editor/Components/BanterMaterialEditor.cs (1)
34-34: LGTM!The editor UI correctly reflects the new
cacheBustproperty in the visible properties list.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.