Enter Stage simplified & Ensemble condition - #310
Conversation
WalkthroughThe changes introduce enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 1
🧹 Nitpick comments (2)
Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs (2)
154-154: Address TODO comment about Prestige exp.The TODO comment indicates that Prestige exp functionality is missing. Would you like me to help implement this feature or create an issue to track this task?
284-291: LGTM! Improved stage boundary detection with TriggerBox.The change from hardcoded coordinates to TriggerBox is a good improvement. However, consider documenting or defining the magic numbers as constants:
- TriggerBox ID: 101
- Portal IDs: 802, 803
+private const int STAGE_TRIGGER_BOX_ID = 101; +private const int STAGE_ENTRY_PORTAL_ID = 802; +private const int STAGE_EXIT_PORTAL_ID = 803; -session.Field.TriggerObjects.Boxes.TryGetValue(101, out TriggerBox? triggerBox); +session.Field.TriggerObjects.Boxes.TryGetValue(STAGE_TRIGGER_BOX_ID, out TriggerBox? triggerBox); -session.Field.MoveToPortal(session, insideStage ? 802 : 803); +session.Field.MoveToPortal(session, insideStage ? STAGE_ENTRY_PORTAL_ID : STAGE_EXIT_PORTAL_ID);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs(3 hunks)Maple2.Server.Game/Util/ConditionUtil.cs(2 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- Maple2.Server.Game/Util/ConditionUtil.cs
- Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: format
- GitHub Check: build
🔇 Additional comments (10)
Maple2.Server.Game/Util/ConditionUtil.cs (3)
129-129: LGTM! Addition of music_play_ensemble condition.The new condition follows the established pattern for music-related conditions and is consistently implemented in both CheckCode and CheckTarget methods.
Also applies to: 226-226
129-129: LGTM! Consistent implementation of the new ensemble condition.The new
music_play_ensemblecondition is correctly implemented and follows the established pattern for similar condition types.Also applies to: 226-226
129-129: LGTM! Addition of music_play_ensemble condition.The new condition follows the established pattern for music-related conditions and is consistently implemented in both CheckCode and CheckTarget methods.
Also applies to: 226-226
Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs (7)
284-290: LGTM! Improved stage entry/exit implementation.The change from hardcoded coordinates to TriggerBox is a significant improvement:
- Uses proper collision detection
- Adds proper null safety checks
- Makes the code more maintainable
216-216: LGTM! Well-placed condition update.The condition update is correctly placed after all validation checks pass and the ensemble successfully starts, ensuring accurate state tracking.
268-268:Details
❓ Verification inconclusive
Consider maintaining defensive null checks.
The removal of null-conditional operators (
?.) when accessingsession.Field.MapIdassumes Field is never null. While this might be true in the current implementation, maintaining defensive null checks could prevent potential NullReferenceException if the contract changes in the future.Also applies to: 274-274, 280-280, 294-294
🏁 Script executed:
#!/bin/bash # Search for other Field.MapId usages to verify the pattern rg -A 2 "Field\.MapId" --type csLength of output: 183
I'll need to gather more context on how the
Field.MapIdpattern is used in this file before finalizing a recommendation. The previousrgcommand didn’t return results due to a file filtering issue. To get a complete picture, please run the following script which searches directly in the intended file:Once we have the output from this script, we can confidently assess whether the removal of defensive null checks is safe or if they should be reinstated.
🏁 Script executed:
#!/bin/bash # Searching for Field.MapId occurrences in InstrumentHandler.cs to verify null-safety assumptions. rg "Field\.MapId" Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs -nLength of output: 172
I'll need to verify the location of the file to ensure we are examining the proper code. Since the previous lookup for
Field.MapIdused an explicit file path that returned "No such file or directory," let's search for theInstrumentHandler.csfile across the repository.Once we have the correct location, we can verify the usage of
Field.MapIdwithin the file. Please provide the output so we can proceed with a confident assessment of the null-safety changes.
🏁 Script executed:
#!/bin/bash # Searching for InstrumentHandler.cs to confirm its correct location find . -type f -name "InstrumentHandler.cs"Length of output: 43
I'll need to verify the location and content of the file before providing a final recommendation on the null-safety changes. The previous search for
InstrumentHandler.csdidn't return any file, which suggests that the file might be located elsewhere or renamed. Please run this script to search for any C# file containing "InstrumentHandler" in its name:Once you provide the output, we can confirm the file's existence and verify its usage of
Field.MapIdto properly assess whether defensive null checks are needed.
🏁 Script executed:
#!/bin/bash # Searching for any C# file with "InstrumentHandler" in the filename in the repository find . -type f -iname "*InstrumentHandler*.cs"Length of output: 48
Action Required: Confirm File Location & Reassess Defensive Null Checks
It appears that our searches did not locate the expected file (
InstrumentHandler.cs). Please verify that the file exists under its expected path (or has been renamed/moved) so we can accurately assess the removal of null-conditional operators. As noted, removing the null-conditional operators onsession.Field.MapIdassumes thatFieldis always non-null. Given potential future changes to this contract, please double-check:
- The existence and correct location of the InstrumentHandler file.
- That
Fieldis indeed guaranteed non-null across all usages (including lines 274, 280, and 294) before this safeguard is permanently removed.
16-16: LGTM! Proper integration of ensemble condition update.The condition update is correctly placed after successfully starting the ensemble score.
Also applies to: 216-216
284-290: LGTM! Improved stage entry logic using TriggerBox.The change from hardcoded coordinates to TriggerBox is a significant improvement:
- More maintainable by removing hardcoded values
- More robust using proper collision detection
- Properly handles the case when TriggerBox is not found
268-268:Details
❓ Verification inconclusive
Verify Field non-null assumption.
The removal of null-conditional operators (
?.) when accessingsession.Field.MapIdassumes thatFieldis guaranteed to be non-null. While there are null checks at the beginning of these methods, please verify this assumption holds true throughout the execution path.Also applies to: 274-274, 280-280, 294-294
🏁 Script executed:
#!/bin/bash # Search for places where Field might be set to null ast-grep --pattern 'session.Field = null' # Search for other methods that might modify Field rg -A 5 'Field\s*='Length of output: 214
Verify Field Non-Null Guarantee
The direct access to
session.Field.MapId(line 268) assumes thatFieldis guaranteed to be non-null, which is why the null-conditional operator (?.) was removed. Although the method contains initial null checks, it’s crucial to confirm that no execution path reassignsFieldto null and that its initialization is robust.Please verify the following:
- Ensure that there are no cases where
session.Fieldgets set tonullor unexpectedly reassigned.- Double-check that all code paths accessing
session.Field(including those on lines 274, 280, and 294) are protected by sufficient checks prior to this usage.The above shell script attempts should help, but manual verification may still be warranted to ensure overall safety.
🏁 Script executed:
#!/bin/bash # Re-run search for assignments setting session.Field to null, # ensuring all files are scanned by removing ignore patterns. rg --no-ignore --hidden -A 5 'session\.Field\s*=\s*null' rg --no-ignore --hidden -A 5 'session\.Field\s*='Length of output: 110
Verify Field Non-Null Guarantee
The earlier searches returned no output, but it appears that ripgrep might have skipped files. To be certain that no execution path inadvertently sets
session.Fieldto null (or modifies it unexpectedly), please run the above script. Once you've confirmed that no assignments tosession.Fieldoccur—including null assignments—across the entire codebase, you can confidently assume that the direct access tosession.Field.MapIdis safe.If the results confirm no modifications to
session.Field, then the removal of the null-conditional operator is justified. Otherwise, please update the checks accordingly.
🏁 Script executed:
#!/bin/bash # Search all C# files for any assignments to session.Field (including potential null assignments) fd -e cs | xargs rg --no-ignore -n 'session\.Field\s*='Length of output: 206
Action Required: Verify Non-Null Assumption of
session.FieldThe removal of the null-conditional operator (
?.) when accessingsession.Field.MapIdassumes thatFieldwill always be non-null. Our automated searches for assignments (including possible null assignments) tosession.Fieldacross the codebase did not return results. However, since the search did not actually process any files (as indicated by the output), this result remains inconclusive.Please manually verify the following in the codebase:
- Confirm that all execution paths initializing or modifying
session.Fieldguarantee a non-null value.- Check that no code (especially within the methods surrounding lines 268, 274, 280, and 294 in
Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs) reassignssession.Fieldto null or otherwise invalidates its assumption of being non-null.Once these checks are confirmed, you can be confident that directly accessing
session.Field.MapIdis safe. Otherwise, consider reintroducing the null-conditional operator or additional safeguards where necessary.
216-216: LGTM! Condition update for ensemble music.The condition update is well-placed after successful instrument usage and score broadcast, ensuring proper state tracking.
Zintixx
left a comment
Zintixx
left a comment
There was a problem hiding this comment.
imo instead of having a constant for this, we put this all in a PerformanceStageManager that is in field.
When the field is instructed, we check if the map is queenstown and if so, we init a stage manager.
I say this all because there's a queue and time limit that needs to be handled for the stage
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs (2)
18-24: Extract magic numbers into constants.The trigger box ID (101) and portal IDs (802, 803) should be defined as constants to improve maintainability and readability.
+ private const int STAGE_TRIGGER_BOX_ID = 101; + private const int STAGE_ENTRY_PORTAL_ID = 802; + private const int STAGE_EXIT_PORTAL_ID = 803; + public void EnterExitStage(GameSession session) { - Field.TriggerObjects.Boxes.TryGetValue(101, out TriggerBox? triggerBox); + Field.TriggerObjects.Boxes.TryGetValue(STAGE_TRIGGER_BOX_ID, out TriggerBox? triggerBox); if (triggerBox is null) { return; } bool insideStage = triggerBox.Contains(session.Player.Position); - Field.MoveToPortal(session, insideStage ? 802 : 803); + Field.MoveToPortal(session, insideStage ? STAGE_ENTRY_PORTAL_ID : STAGE_EXIT_PORTAL_ID); }
7-7: Consider implementing queue management and time limit.The TODO comment indicates important features that should be implemented for better stage management:
- Queue system to handle multiple players wanting to enter the stage
- Time limit to prevent a single player from monopolizing the stage
Would you like me to help design and implement these features or create an issue to track this work?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Maple2.Server.Game/Manager/Field/FieldManager.cs(2 hunks)Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs(1 hunks)Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: format
- GitHub Check: build
🔇 Additional comments (2)
Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs (1)
17-25: LGTM! Clean implementation of stage entry/exit logic.The implementation effectively uses trigger boxes to determine player position relative to the stage and moves them to appropriate portals. This is a clean and maintainable solution.
Maple2.Server.Game/Manager/Field/FieldManager.cs (1)
57-57: LGTM! Clean integration of PerformanceStageManager.The
PerformanceStageManageris properly integrated:
- Declared as nullable readonly field
- Initialized only for the specific performance map
- Uses proper null safety patterns
Also applies to: 99-101
2 participants
Added music_play_ensemble condition
Simplified enter stage with proper trigger box
Summary by CodeRabbit
New Features
Refactor