[Fix] Waddle / Походка Клоуна #107
Conversation
WalkthroughThe changes involve the complete removal of the Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
Content.Server/_White/Animations/WaddleAnimationSystem.cs (1)
6-17: Consider adding XML documentation.To improve code maintainability, consider adding XML documentation to describe:
- The purpose of this animation system
- The expected behavior of PlayAnimation and StopAnimation methods
- The relationship with the network events
Example addition:
+/// <summary> +/// Server-side implementation of the waddle animation system. +/// Handles raising network events for animation state changes. +/// </summary> public sealed class WaddleAnimationSystem : SharedWaddledAnimationSystem { + /// <summary> + /// Starts the waddle animation for the specified user by raising a network event. + /// </summary> + /// <param name="user">The entity to animate</param> protected override void PlayAnimation(EntityUid user) { RaiseNetworkEvent(new StartedWaddlingEvent(GetNetEntity(user))); } + /// <summary> + /// Stops the waddle animation for the specified user by raising a network event. + /// </summary> + /// <param name="user">The entity to stop animating</param> protected override void StopAnimation(EntityUid user) { RaiseNetworkEvent(new StoppedWaddlingEvent(GetNetEntity(user))); } }Content.Shared/Movement/Components/WaddleAnimationComponent.cs (2)
8-14: Enhance parameter documentation clarityThe implementation looks good with proper network serialization support and event inheritance. However, the parameter documentation could be more descriptive.
-/// <param name="user">The newly be-waddled.</param> +/// <param name="user">The entity that started waddling.</param>
17-24: Enhance parameter documentation clarityThe implementation mirrors StartedWaddlingEvent appropriately. Consider improving the parameter documentation for consistency.
-/// <param name="user">The former waddle-er.</param> +/// <param name="user">The entity that stopped waddling.</param>Content.Shared/_White/Animations/SharedWaddledAnimationSystem.cs (2)
12-15: Ensure consistent access modifiers for dependency fieldsThe dependency fields have inconsistent access modifiers:
Timingisprotected, while_standingState,_gravity, and_buckleareprivate. For consistency, consider using the same access modifier for all dependency fields. If derived classes need access to these dependencies, they should all beprotected; otherwise, they can all beprivate.
48-50: Use consistent parameter names forEntityUidThe methods
PlayAnimationandStopAnimationuse the parameter nameuserforEntityUid, whereas the rest of the code usesuid. For consistency and clarity, consider renaming the parameter touid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- Content.Client/Movement/Systems/WaddleAnimationSystem.cs (0 hunks)
- Content.Client/_White/Animations/WaddleAnimationSystem.cs (1 hunks)
- Content.Server/_White/Animations/WaddleAnimationSystem.cs (1 hunks)
- Content.Shared/Movement/Components/WaddleAnimationComponent.cs (1 hunks)
- Content.Shared/_White/Animations/SharedWaddledAnimationSystem.cs (1 hunks)
- Content.Shared/_White/Clothing/EntitySystems/WaddleClothingSystem.cs (1 hunks)
💤 Files with no reviewable changes (1)
- Content.Client/Movement/Systems/WaddleAnimationSystem.cs
✅ Files skipped from review due to trivial changes (1)
- Content.Shared/_White/Clothing/EntitySystems/WaddleClothingSystem.cs
🔇 Additional comments (4)
Content.Server/_White/Animations/WaddleAnimationSystem.cs (1)
1-4: LGTM! Clean and minimal imports.The imports and namespace structure are well-organized and follow the server-side conventions.
Content.Shared/Movement/Components/WaddleAnimationComponent.cs (2)
2-2: LGTM: Required using statement added for serialization supportThe addition of the Serialization namespace is necessary for the new network serialization attributes.
11-12: Verify network event handling implementationThe events are now network-serializable. Let's ensure all systems properly handle these networked events.
Also applies to: 21-22
✅ Verification successful
Let me gather more information about the event handling implementation by checking the actual systems.
Network event handling is properly implemented
The verification shows that the network events are correctly implemented across the client-server architecture:
- Server raises network events using
RaiseNetworkEventfor both start and stop events- Client subscribes to these events using
SubscribeAllEventand handles them appropriately- Events are properly serializable with the required attributes
- The component and systems maintain proper separation of concerns between client, server, and shared code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for proper event handling implementation in both client and server systems # Look for systems that raise these events echo "Systems that raise these events:" rg -l "StartedWaddlingEvent|StoppedWaddlingEvent" --type cs # Check for proper event subscriptions echo -e "\nEvent subscriptions:" ast-grep --pattern 'Subscribe<$_(StartedWaddlingEvent|StoppedWaddlingEvent)>'Length of output: 452
Script:
#!/bin/bash # Check the implementation details of the systems echo "=== Server System Implementation ===" cat Content.Server/_White/Animations/WaddleAnimationSystem.cs echo -e "\n=== Client System Implementation ===" cat Content.Client/_White/Animations/WaddleAnimationSystem.cs echo -e "\n=== Shared Component Definition ===" cat Content.Shared/Movement/Components/WaddleAnimationComponent.csLength of output: 7345
Content.Shared/_White/Animations/SharedWaddledAnimationSystem.cs (1)
24-46: Logic inOnMovementInputmethod is correct and well-structuredThe
OnMovementInputmethod effectively manages the waddling animation state based on movement input and entity conditions. The use of early returns enhances readability and performance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Content.Client/_White/Animations/WaddleAnimationSystem.cs (1)
39-39: Use Constants for Magic NumbersFor better readability and maintainability, consider defining a constant for the value
360.Add the constant at the class level:
private const float FullRotationDegrees = 360f;Then update the line:
-var tumbleIntensity = component.LastStep ? 360 - component.TumbleIntensity : component.TumbleIntensity; +var tumbleIntensity = component.LastStep ? FullRotationDegrees - component.TumbleIntensity : component.TumbleIntensity;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- Content.Client/_White/Animations/WaddleAnimationSystem.cs (1 hunks)
- Content.Server/_White/Animations/WaddleAnimationSystem.cs (1 hunks)
- Content.Shared/_White/Animations/SharedWaddleAnimationSystem.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Content.Server/_White/Animations/WaddleAnimationSystem.cs
🔇 Additional comments (7)
Content.Shared/_White/Animations/SharedWaddleAnimationSystem.cs (4)
12-15: Dependencies are correctly injected.The dependencies are properly declared and injected using the
[Dependency]attribute, ensuring that all necessary systems are available for this class.
17-22: Initialization correctly subscribes to movement events.The
Initializemethod appropriately callsbase.Initialize()and subscribes to theMoveInputEventfor theWaddleAnimationComponent, which is essential for handling movement input.
24-46: Effective handling of movement input for waddling animation.The
OnMovementInputmethod efficiently manages the logic to start and stop the waddling animation based on the entity's state and directional movement input.
48-50: Abstract methods enforce consistent animation behavior in derived classes.Defining
PlayAnimationandStopAnimationas abstract methods ensures that all inheriting classes implement these methods, promoting a consistent and extendable design for animation handling.Content.Client/_White/Animations/WaddleAnimationSystem.cs (3)
24-25: Previous Comment Still Applicable: ReplaceGetEntity(ev.User)withev.UserThe method
GetEntityused inPlayAnimation(GetEntity(ev.User))andStopAnimation(GetEntity(ev.User))is not defined and may not be necessary. Ifev.Useris already anEntityUid, you can pass it directly.
30-30: Previous Comment Still Applicable: InjectIGameTimingand Update Reference toTimingThe
Timingobject used in the condition!Timing.IsFirstTimePredictedis not defined within the scope of this class. You need to injectIGameTimingvia dependency injection to access timing information.
57-59: Previous Comment Still Applicable: Correct KeyFrame Times to Complete the AnimationThe
KeyFramesin yourAnimationTrackshave duplicate times atlen/3, causing the animation to reset prematurely. The final keyframe should occur atlento match the total length of the animation.Also applies to: 69-71
Описание PR
Исправление анимации ботинок клоуна
Изменения
🆑 Spatison