QOL changes to enlightenment and fellowship mechanics.#355
QOL changes to enlightenment and fellowship mechanics.#355Ruggan wants to merge 3 commits intorkroska:masterfrom
Conversation
## Changes ### Enlightenment - Players now remain in their fellowship. - Players are no longer stripped of buffs. - Players are no longer teleported away. - Players no longer lose unspent experience. - Refactored `GetEnlightenmentRatingBonus` to use O(1) math instead of a per-enl loop. - Removed dead code and cleaned up comment. ### Fellowship - Level is no longer a factor in XP sharing.
WalkthroughRemoves fellowship XP-sharing bookkeeping and stops emitting EvenShare; trims DeveloperCommands output. Large refactor of Enlightenment: recalculates luminance costs, moves many helpers to private, adjusts requirement checks and effects. Adds a zero-amount guard in Player luminance spend. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/ACE.Server/Entity/Enlightenment.cs (1)
55-88:⚠️ Potential issue | 🟡 MinorDefer the LevelUp VFX until enlightenment actually succeeds.
Right now the effect fires before the movement/luminance checks, so failed attempts still show a success cue.Suggested fix
- player.ApplyVisualEffects(PlayScript.LevelUp); var endPos = new ACE.Entity.Position(player.Location); @@ AddPerks(player); if (player.Enlightenment >= 25) { DequipAllItems(player); } + player.ApplyVisualEffects(PlayScript.LevelUp); player.SaveBiotaToDatabase();
🧹 Nitpick comments (1)
Source/ACE.Server/Entity/Enlightenment.cs (1)
410-426: Add boundary tests for the new piecewise formula.
Please cover 10/11, 20/21, and 50/51 to ensure the O(1) refactor matches previous behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/ACE.Server/Entity/Enlightenment.cs (1)
342-348:⚠️ Potential issue | 🟠 MajorPre-existing bug:
SpendLuminancefor >300 enlightenment doesn't apply the cost modifier fromVerifyRequirements.
VerifyRequirements(lines 211–225) computes the luminance cost for >300 enlightenment using a scalingcostModifier(1.0 + 0.5 * steps), butSpendLuminance(lines 342–348) just usestargetEnlightenment * baseLumCostwithout the modifier. This means the player's luminance is verified against the higher modified amount, but only the base amount is actually deducted — the player keeps the difference.Additionally,
VerifyRequirementscastsbaseLumCosttodecimalwhileSpendLuminancedoes not, introducing a second inconsistency in numeric precision.Not introduced by this PR, but since you're actively refactoring enlightenment logic, this is worth fixing now.
🐛 Proposed fix to apply the same cost modifier
else if (player.Enlightenment + 1 > 300) { - var baseLumCost = ServerConfig.enl_300_base_lum_cost.Value; + var baseLumCost = (decimal)ServerConfig.enl_300_base_lum_cost.Value; var targetEnlightenment = player.Enlightenment + 1; - long reqLum300 = targetEnlightenment * baseLumCost; + + int over = targetEnlightenment - 300; + int steps = (over - 1) / 50; + decimal costModifier = 1.0m + (0.5m * steps); + + long reqLum300 = (long)Math.Ceiling((targetEnlightenment * baseLumCost) * costModifier); return player.SpendLuminance(reqLum300); }
🧹 Nitpick comments (1)
Source/ACE.Server/Entity/Enlightenment.cs (1)
409-427: Well-done O(1) refactor — minor comment accuracy nit.The piecewise math is correct at all tier boundaries (10→10, 20→15, 50→21).
AggressiveInliningis appropriate here.Line 425: the comment says "From 50 and beyond" but
enlightenmentAmt == 50is handled by the<= 50branch above. The final tier actually starts at 51.📝 Suggested comment fix
- // From 50 and beyond: 1 Damage Rating per 10 Enlightenments + // From 51 and beyond: 1 Damage Rating per 10 Enlightenments
…lidating and spending luminance. This impacts the 350+ calculation, which was previously validating with a multiplier and then only charging the un-multiplied value.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Source/ACE.Server/Entity/Enlightenment.cs`:
- Around line 221-225: VerifyLuminance currently only checks
player.BankedLuminance but SpendLuminance will use player.AvailableLuminance
first then fallback to BankedLuminance, causing false negatives; update
VerifyLuminance in Enlightenment.cs to mirror the spend logic by returning true
when player.AvailableLuminance + player.BankedLuminance >= reqLum (or
alternatively call SpendLuminance in a non-destructive "canSpend" mode if such
exists), referencing the Player class properties AvailableLuminance,
BankedLuminance and the SpendLuminance method so the verification matches actual
spending behavior.
🧹 Nitpick comments (1)
Source/ACE.Server/Entity/Enlightenment.cs (1)
46-93: Animation chain: re-verify requirements aren't stale after 14-second delay.After the 14-second animation delay, only position and luminance are re-checked before spending. Other requirements verified upfront (level, vitae, inventory slots, tokens, etc.) could change during the delay (e.g., player gets vitae from a death in another context, or items are moved). The
IsBusyflag mitigates most of these, but consider whether a full re-verification call would be safer here.
| public static bool VerifyLuminance(Player player, long reqLum) | ||
| { | ||
| if (reqLum <= 0) return true; | ||
| return player.BankedLuminance >= reqLum; | ||
| } |
There was a problem hiding this comment.
VerifyLuminance only checks BankedLuminance, but SpendLuminance also draws from AvailableLuminance.
VerifyLuminance checks player.BankedLuminance >= reqLum, but Player.SpendLuminance (line 92-104 in Player_Luminance.cs) tries AvailableLuminance first and falls back to BankedLuminance. A player whose luminance sits entirely in AvailableLuminance would fail verification here yet succeed at the actual spend—meaning they'd be blocked with a misleading "not enough luminance" message despite having enough.
Consider aligning the check with the spend logic:
Proposed fix
public static bool VerifyLuminance(Player player, long reqLum)
{
if (reqLum <= 0) return true;
- return player.BankedLuminance >= reqLum;
+ return (player.BankedLuminance ?? 0) >= reqLum
+ || (player.AvailableLuminance ?? 0) >= reqLum;
}📝 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.
| public static bool VerifyLuminance(Player player, long reqLum) | |
| { | |
| if (reqLum <= 0) return true; | |
| return player.BankedLuminance >= reqLum; | |
| } | |
| public static bool VerifyLuminance(Player player, long reqLum) | |
| { | |
| if (reqLum <= 0) return true; | |
| return (player.BankedLuminance ?? 0) >= reqLum | |
| || (player.AvailableLuminance ?? 0) >= reqLum; | |
| } |
🤖 Prompt for AI Agents
In `@Source/ACE.Server/Entity/Enlightenment.cs` around lines 221 - 225,
VerifyLuminance currently only checks player.BankedLuminance but SpendLuminance
will use player.AvailableLuminance first then fallback to BankedLuminance,
causing false negatives; update VerifyLuminance in Enlightenment.cs to mirror
the spend logic by returning true when player.AvailableLuminance +
player.BankedLuminance >= reqLum (or alternatively call SpendLuminance in a
non-destructive "canSpend" mode if such exists), referencing the Player class
properties AvailableLuminance, BankedLuminance and the SpendLuminance method so
the verification matches actual spending behavior.
1 participant
Changes
Enlightenment
GetEnlightenmentRatingBonusto use O(1) math instead of a per-enl loop.Fellowship
Summary by CodeRabbit
Bug Fixes
Balance Changes
Quality