Open
Conversation
Contributor
WalkthroughAdds six ServerConfig properties for defense scaling (toggles and aggression factors) and implements per-combat-type dynamic defense scaling in skill-check calculations; damage event logic now selects melee or missile-specific skill-chance paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/ACE.Server/WorldObjects/SkillCheck.cs`:
- Around line 10-37: The factor-based inference in SkillCheck (using factor ==
0.03/0.07) causes non-combat callers to accidentally enable melee scaling and
ignores missile settings; change the API to accept an explicit defense type or
introduce dedicated helpers (e.g. GetMeleeCombatSkillChance,
GetMissileCombatSkillChance) and move the scaling logic there: replace the
implicit factor check in SkillCheck.cs with a switch on a new parameter (e.g.
DefenseType enum) or call site-specific helper methods, ensure scalingEnabled
and aggression read the correct ServerConfig.defense_scaling_melee_* or
ServerConfig.defense_scaling_missile_* values (use missile config for missile
helper), and update all callers to use the new parameter/helpers so non-combat
checks (lockpicking, deception, healing, crafting, mana conversion) no longer
trigger combat scaling.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Source/ACE.Server/Managers/PropertyManager.csSource/ACE.Server/WorldObjects/SkillCheck.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/ACE.Server/WorldObjects/SkillCheck.cs`:
- Around line 27-33: Add a uint overload for GetMagicSkillChance to match the
existing overloads for GetMeleeCombatSkillChance and
GetMissileCombatSkillChance; implement a public static double
GetMagicSkillChance(uint skill, int difficulty) that forwards to the existing
GetMagicSkillChance(int skill, int difficulty) (i.e., cast the uint to int and
call the int overload) so callers like Player_Magic.cs can pass a uint without
an explicit cast and API symmetry is preserved.
Comment on lines
27
to
+33
| public static double GetMagicSkillChance(int skill, int difficulty) | ||
| { | ||
| return GetSkillChance(skill, difficulty, 0.07f); | ||
| const float factor = 0.07f; | ||
| bool scalingEnabled = ServerConfig.defense_scaling_magic_enabled.Value; | ||
| double aggression = ServerConfig.defense_scaling_magic_agg.Value; | ||
| return ApplyScalingAndCompute(skill, difficulty, factor, scalingEnabled, aggression); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=cs '\bGetMagicSkillChance\s*\(' -C 2Repository: rkroska/ACECustom
Length of output: 1112
🏁 Script executed:
# Examine the SkillCheck.cs file to see the actual method definitions and overloads
fd -n "SkillCheck.cs" --type f | head -5Repository: rkroska/ACECustom
Length of output: 291
🏁 Script executed:
# Get the file location and examine it
find . -name "SkillCheck.cs" -type f | head -1 | xargs wc -lRepository: rkroska/ACECustom
Length of output: 110
🏁 Script executed:
cat -n ./Source/ACE.Server/WorldObjects/SkillCheck.csRepository: rkroska/ACECustom
Length of output: 4579
🏁 Script executed:
# Search for all callers of GetMagicSkillChance across the codebase
rg --type=cs 'GetMagicSkillChance'Repository: rkroska/ACECustom
Length of output: 323
GetMagicSkillChance is missing a uint overload for API consistency.
GetMeleeCombatSkillChance and GetMissileCombatSkillChance both have uint overloads (lines 46–48, 62–64), but GetMagicSkillChance lacks one. The method is actively called from Player_Magic.cs:537 where the caller must cast uint to int before invoking it. Adding a uint overload would eliminate this explicit cast and maintain consistency with the sibling combat methods.
Proposed uint overload
public static double GetMagicSkillChance(int skill, int difficulty)
{
const float factor = 0.07f;
bool scalingEnabled = ServerConfig.defense_scaling_magic_enabled.Value;
double aggression = ServerConfig.defense_scaling_magic_agg.Value;
return ApplyScalingAndCompute(skill, difficulty, factor, scalingEnabled, aggression);
}
+
+public static double GetMagicSkillChance(uint skill, uint difficulty)
+{
+ return GetMagicSkillChance((int)skill, (int)difficulty);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static double GetMagicSkillChance(int skill, int difficulty) | |
| { | |
| return GetSkillChance(skill, difficulty, 0.07f); | |
| const float factor = 0.07f; | |
| bool scalingEnabled = ServerConfig.defense_scaling_magic_enabled.Value; | |
| double aggression = ServerConfig.defense_scaling_magic_agg.Value; | |
| return ApplyScalingAndCompute(skill, difficulty, factor, scalingEnabled, aggression); | |
| } | |
| public static double GetMagicSkillChance(int skill, int difficulty) | |
| { | |
| const float factor = 0.07f; | |
| bool scalingEnabled = ServerConfig.defense_scaling_magic_enabled.Value; | |
| double aggression = ServerConfig.defense_scaling_magic_agg.Value; | |
| return ApplyScalingAndCompute(skill, difficulty, factor, scalingEnabled, aggression); | |
| } | |
| public static double GetMagicSkillChance(uint skill, uint difficulty) | |
| { | |
| return GetMagicSkillChance((int)skill, (int)difficulty); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/ACE.Server/WorldObjects/SkillCheck.cs` around lines 27 - 33, Add a
uint overload for GetMagicSkillChance to match the existing overloads for
GetMeleeCombatSkillChance and GetMissileCombatSkillChance; implement a public
static double GetMagicSkillChance(uint skill, int difficulty) that forwards to
the existing GetMagicSkillChance(int skill, int difficulty) (i.e., cast the uint
to int and call the int overload) so callers like Player_Magic.cs can pass a
uint without an explicit cast and API symmetry is preserved.
Collaborator
Author
There was a problem hiding this comment.
player_magic.cs already contains var chance = SkillCheck.GetMagicSkillChance((int)magicSkill, (int)difficulty); and this change shouldn't be required
Contributor
There was a problem hiding this comment.
@Schneebs, you're absolutely right—the explicit cast works fine and the overload isn't necessary. I'll leave it as-is. Thanks for clarifying! 👍
🧠 Learnings used
Learnt from: CrimsonMage
Repo: rkroska/ACECustom PR: 238
File: Source/ACE.Server/Command/Handlers/AdminCommands.cs:4529-4545
Timestamp: 2025-11-10T14:08:11.673Z
Learning: In ACECustom: Melee/Missile luminance aug counts (Creature.LuminanceAugmentMeleeCount/MissileCount) are applied as flat damage bonuses during DamageEvent calculation (Source/ACE.Server/Entity/DamageEvent.cs) and do not modify EffectiveAttackSkill. CombatPet.GetBaseDamage() adds only ItemAugDamageBonus; DamageEvent later adds the Melee/Missile aug damage.
Learnt from: Schneebs
Repo: rkroska/ACECustom PR: 293
File: Source/ACE.Server/WorldObjects/Monster_Awareness.cs:255-255
Timestamp: 2025-12-17T19:14:14.837Z
Learning: In code reviews, assume ThreadSafeRandom.Next(min, max) is inclusive on the upper bound (unlike System.Random which is exclusive). When using it to index into a collection, e.g., ThreadSafeRandom.Next(0, list.Count - 1), verify that all valid indices (0 through Count-1) can be selected. Look for off-by-one errors where a caller uses max as Count instead of Count-1, and adjust or add tests to ensure full index coverage.
Learnt from: Ruggan
Repo: rkroska/ACECustom PR: 325
File: Source/ACE.Server/WorldObjects/Monster_Navigation.cs:290-300
Timestamp: 2026-01-16T14:29:16.414Z
Learning: In Monster_Navigation.cs, in CheckForStuck(), do not rely solely on MoveToManager.FailProgressCount increments (which only occur when !IsInterpolating && !IsAnimating) and are reset on any distance change. Manually increment FailProgressCount to ensure independent stuck detection, since micro-movements shouldn't reset the stuck counter. This guidance can apply to other navigation-related C# files with a similar MoveToManager interaction: implement an explicit increment when stagnation is detected and only reset on deliberate progress, ensuring the stuck threshold can still be reached. Be mindful of thread-safety and existing test coverage.
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.
Summary by CodeRabbit