DK Master Tree Finishup#766
Conversation
| var baseSkill = skillEntry.GetBaseSkill(); | ||
|
|
||
| if (player.GameContext.PlugInManager.GetStrategy<short, IAreaSkillPlugIn>(baseSkill.Number) is { } strategy) | ||
| if (player.GameContext.PlugInManager.GetStrategy<short, IAreaSkillPlugIn>(skillEntry.Skill.Number) is { } strategy) | ||
| { | ||
| await strategy.AfterTargetGotAttackedAsync(player, target, skillEntry, targetAreaCenter, hitInfo).ConfigureAwait(false); | ||
| return; | ||
| } | ||
|
|
||
| var baseSkill = skillEntry.GetBaseSkill(); | ||
| if (player.GameContext.PlugInManager.GetStrategy<short, IAreaSkillPlugIn>(baseSkill.Number) is { } baseSkillStrategy) | ||
| { | ||
| await baseSkillStrategy.AfterTargetGotAttackedAsync(player, target, skillEntry, targetAreaCenter, hitInfo).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
Added for TwistingSlashMasterySkillPlugIn.cs.
Another option is to change the Key there to the base Twisting Slash skill and keep this as before.
| var magicEffect = new MagicEffect(TimeSpan.FromSeconds(2), stunEffectDefinition, [new MagicEffect.ElementWithTarget(powerUp, Stats.IsStunned)]); | ||
| await attackable.MagicEffectList.AddEffectAsync(magicEffect).ConfigureAwait(false); | ||
| } | ||
|
|
| { | ||
| return item.ItemSlot >= InventoryConstants.HelmSlot && item.ItemSlot <= InventoryConstants.BootsSlot; | ||
| } | ||
|
|
| result.AttributeCombinations.Add(this.CreateAttributeRelationship(Stats.ComboBonus, 0.5f, Stats.TotalEnergy)); | ||
|
|
||
| result.AttributeCombinations.Add(this.CreateConditionalRelationship(Stats.AttackSpeedAny, Stats.IsOneHandedSwordEquipped, Stats.WeaponMasteryAttackSpeed)); | ||
| result.AttributeCombinations.Add(this.CreateConditionalRelationship(Stats.DoubleDamageChance, Stats.IsSpearEquipped, Stats.SpearMasteryDoubleDamageChance)); |
| magicEffect.StopByDeath = true; | ||
| magicEffect.Duration = this.Context.CreateNew<PowerUpDefinitionValue>(); | ||
| magicEffect.Duration.ConstantValue.Value = 60f; | ||
| magicEffect.Duration.MaximumValue = 180f; |
| var boostPerPartyMember = this.Context.CreateNew<AttributeRelationship>(); | ||
| boostPerPartyMember.InputAttribute = Stats.NearbyPartyMemberCount.GetPersistent(this.GameConfiguration); | ||
| boostPerPartyMember.InputOperator = InputOperator.Multiply; | ||
| boostPerPartyMember.InputOperand = 1f / 100; |
|
|
||
| healthPowerUpDefinition.Boost = this.Context.CreateNew<PowerUpDefinitionValue>(); | ||
| healthPowerUpDefinition.Boost.ConstantValue.Value = 0.12f; | ||
| healthPowerUpDefinition.Boost.MaximumValue = 2f; |
| // one percent per party member in view | ||
| var boostPerPartyMember = this.Context.CreateNew<AttributeRelationship>(); | ||
| boostPerPartyMember.InputAttribute = Stats.NearbyPartyMemberCount.GetPersistent(this.GameConfiguration); | ||
| boostPerPartyMember.InputOperator = InputOperator.Multiply; | ||
| boostPerPartyMember.InputOperand = 1f / 100; | ||
|
|
||
| var manaPowerUpDefinition = this.Context.CreateNew<PowerUpDefinition>(); | ||
| magicEffect.PowerUpDefinitions.Add(manaPowerUpDefinition); | ||
| manaPowerUpDefinition.TargetAttribute = Stats.SwellLifeManaIncrease.GetPersistent(this.GameConfiguration); | ||
| manaPowerUpDefinition.Boost = this.Context.CreateNew<PowerUpDefinitionValue>(); | ||
| manaPowerUpDefinition.Boost.RelatedValues.Add(boostPerPartyMember); |
| skill.SkipElementalModifier = true; | ||
| skill.MagicEffectDef = this.CreateEffect(ElementalType.Ice, MagicEffectNumber.Cold, Stats.IsIced, 10); | ||
| return; | ||
| } |
| { | ||
| skill.ElementalModifierTarget = Stats.IceResistance.GetPersistent(this.GameConfiguration); | ||
| skill.SkipElementalModifier = true; | ||
| skill.MagicEffectDef = this.CreateEffect(ElementalType.Ice, MagicEffectNumber.Cold, Stats.IsIced, 10); |
| /// The cold effect caused by chain drive and strike of destruction skills. Like <see cref="Iced"/>, it also slows down movement. | ||
| /// </summary> | ||
| Cold = 0x56, | ||
|
|
| if (multiValues == 0 && this.Elements.All(e => e.AggregateType != AggregateType.Multiplicate)) | ||
| { | ||
| multiValues = 1; | ||
| } |
There was a problem hiding this comment.
This is an impossible situation since in line 66 an enumerable with "1" is concatenated.
| { | ||
| await this.InvokeViewPlugInAsync<IItemDurabilityChangedPlugIn>(p => p.ItemDurabilityChangedAsync(randomArmorItem, false)).ConfigureAwait(false); | ||
| } | ||
| } |
|
All set! |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes the Dark Knight master tree implementation, introducing new skill effects and proficiency bonuses. It also refactors the attribute system to use more generic mastery-based stats and addresses critical bugs in double-wield damage calculations and magic effect handling. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request completes the Dark Knight master tree implementation and refines double-wield damage calculations. Key changes include the introduction of new attributes for mastery skills, logic for mace-based stuns and durability reduction, and the ability to override attribute aggregation types for specific item options. Additionally, it includes several data initialization updates and a new plugin for Twisting Slash Mastery. The review feedback identifies several critical issues, including potential NullReferenceExceptions when accessing entity attributes, a logic regression in attribute defaulting, thread-safety concerns regarding shared static fields, and idempotency risks in database update scripts.
| if (this.Elements.All(e => e.AggregateType == AggregateType.Multiplicate)) | ||
| { | ||
| rawValues = 1; | ||
| } |
There was a problem hiding this comment.
The new logic for defaulting rawValues to 1 is more restrictive than the previous implementation. By checking if all elements are of type Multiplicate, it fails to handle scenarios where the attribute contains other non-raw elements (like AddFinal or Maximum) but no AddRaw elements. In Mu Online's attribute system, multipliers should generally apply to a base of 1 if no explicit raw additions are present. The previous check this.Elements.All(e => e.AggregateType != AggregateType.AddRaw) correctly identified the absence of a base value regardless of other non-raw elements.
if (this.Elements.All(e => e.AggregateType != AggregateType.AddRaw))
{
rawValues = 1;
}There was a problem hiding this comment.
That would fail if we have only AggregateType.AddFinal values. Suppose we only have a +3 AggregateType.AddFinal. Then, we get: 1*1 + 3 = 4, instead of 3.
In case the attribute has different values, like AggregatyeType.AddFinal and AggregateType.Multiplicate values, I think it makes more sense to explicitly add a AggregateType.AddRaw value of 1 (this seems to be the standard anyway), otherwise it becomes too messy with assumptions.
|
|
||
| await this.HitAsync(hitInfo, attacker, skill?.Skill, isFinalStreakHit).ConfigureAwait(false); | ||
| await this.DecreaseItemDurabilityAfterHitAsync(hitInfo).ConfigureAwait(false); | ||
| await this.DecreaseItemDurabilityAfterHitAsync(hitInfo, attacker.Attributes[Stats.RagefulBlowMasteryDurabilityDecChance]).ConfigureAwait(false); |
There was a problem hiding this comment.
Directly accessing attacker.Attributes is unsafe here. If the attacker is an entity without initialized attributes (which can happen with certain NPCs or in specific edge cases), this will throw a NullReferenceException. It's safer to use a null-conditional access with a default value.
await this.DecreaseItemDurabilityAfterHitAsync(hitInfo, attacker.Attributes?[Stats.RagefulBlowMasteryDurabilityDecChance] ?? 0).ConfigureAwait(false);| e => e.Number == (short)effectNumber | ||
| && e.SubType == (byte)(0xFF - type) | ||
| && Equals(e.Duration?.ConstantValue.Value, durationInSeconds) | ||
| && e.Chance?.ConstantValue.Value == chance |
There was a problem hiding this comment.
This comparison will throw a NullReferenceException if e.Chance is null, which is the case for many magic effects. Since the chance parameter defaults to 0, the check should safely handle null Chance properties by treating them as 0.
&& (e.Chance?.ConstantValue.Value ?? 0) == chance| { Stats.CurrentAbility, Stats.AbilityUsageReduction }, | ||
| }; | ||
|
|
||
| private static MagicEffectDefinition? stunEffectDefinition; |
There was a problem hiding this comment.
The static field stunEffectDefinition is shared across all instances and is not initialized in a thread-safe manner. In environments with multiple GameContext instances (e.g., multi-tenant servers or unit tests), this could lead to cross-context contamination if different configurations are used. Additionally, the lazy initialization ??= is subject to race conditions. Consider retrieving the definition from the attacker.GameContext directly within the method or using a thread-safe caching mechanism if performance is a concern.
There was a problem hiding this comment.
That seems to make sense. Remove static, @sven-n ?
| this._stunEffectDefinition ??= ((Player)attacker).GameContext.Configuration.MagicEffects.First(m => m.Number == StunnedMagicEffectNumber); | ||
|
|
||
| if (!target.IsAlive || !Rand.NextRandomBool(Convert.ToDouble(attacker.Attributes[Stats.StunChance]))) | ||
| if (!target.IsAlive || !Rand.NextRandomBool(attacker.Attributes[Stats.MasteryStunChance])) |
There was a problem hiding this comment.
Unsafe access to attacker.Attributes. If the attacker is not a player or has no attributes, this will throw a NullReferenceException. Use null-conditional access.
if (!target.IsAlive || !Rand.NextRandomBool(attacker.Attributes?[Stats.MasteryStunChance] ?? 0))| if (!target.IsAlive | ||
| || target is not IMovable movableTarget | ||
| || target.CurrentMap is not { } currentMap | ||
| || !Rand.NextRandomBool(attacker.Attributes[Stats.MasteryMoveTargetChance])) |
There was a problem hiding this comment.
Unsafe access to attacker.Attributes. If the attacker is not a player or has no attributes, this will throw a NullReferenceException. Use null-conditional access.
|| !Rand.NextRandomBool(attacker.Attributes?[Stats.MasteryMoveTargetChance] ?? 0))| twistingSlashMastery.ReplacedSkill = gameConfiguration.Skills.First(s => s.Number == (short)SkillNumber.TwistingSlashStreng); | ||
| twistingSlashMastery.TargetAttribute = Stats.MasteryMoveTargetChance.GetPersistent(gameConfiguration); | ||
| twistingSlashMastery.Aggregation = AggregateType.AddRaw; | ||
| twistingSlashMastery.ValueFormula = $"{twistingSlashMastery.ValueFormula} / 100"; |
There was a problem hiding this comment.
The update to ValueFormula is not idempotent. If this update plugin is executed multiple times (e.g., during a failed migration retry), the formula will be appended with / 100 repeatedly (e.g., ... / 100 / 100), leading to incorrect skill values. You should check if the formula already contains the division or set the formula to a fixed string.
| if (option.OptionType == ItemOptionTypes.Excellent) | ||
| { | ||
| if (item.ItemSlot == InventoryConstants.PendantSlot | ||
| && option.PowerUpDefinition?.TargetAttribute == Stats.PhysicalBaseDmg) | ||
| { | ||
| // Pendant options are not subject to double wield averaging | ||
| aggregateType = AggregateType.AddFinal; | ||
| } | ||
| else if (option.PowerUpDefinition?.TargetAttribute == Stats.PhysicalBaseDmgIncrease | ||
| && item.Definition!.BasePowerUpAttributes.Any(pu => pu.TargetAttribute == Stats.DoubleWieldWeaponCount)) | ||
| { | ||
| // This needs special treatment, since this option is averaged when double wielding | ||
| aggregateType = AggregateType.AddRaw; | ||
| } | ||
| else | ||
| { | ||
| // the normal aggregate type should be used | ||
| } |
| attributeRelationships.Add(this.CreateAttributeRelationship(Stats.MaximumPhysBaseDmgByWeapon, 0.5f, Stats.HasDoubleWield, InputOperator.ExponentiateByAttribute, AggregateType.Multiplicate)); | ||
| attributeRelationships.Add(this.CreateAttributeRelationship(Stats.PhysicalBaseDmg, 0.5f, Stats.HasDoubleWield, InputOperator.ExponentiateByAttribute, AggregateType.Multiplicate)); | ||
| attributeRelationships.Add(this.CreateAttributeRelationship(Stats.PhysicalBaseDmgIncrease, 0.5f, Stats.HasDoubleWield, InputOperator.ExponentiateByAttribute, AggregateType.Multiplicate)); | ||
| attributeRelationships.Add(this.CreateAttributeRelationship(Stats.PhysicalBaseDmgIncrease, 1, Stats.HasDoubleWield)); |
There was a problem hiding this comment.
Refer to this comment.
For example, if you double wield with weapons w/ item option (or exc lvl/20 option) (Stats.PhysicalBaseDmg):
- 1 weapon with +12 dmg => +6 dmg
- 2 weapons with +12 dmg => +12 dmg
- 1 weapon with +8 dmg, 1 weapon with +4 dmg => +6 dmg
if you double wield with exc +2% dmg option (Stats.PhysicalBaseDmgIncrease):
- 1 weapon with +2% dmg => +1% dmg
- 2 weapons with +2% dmg => +2% dmg
This checks out with the client calculations in 'C' menu.
| public override string? ToString() | ||
| { | ||
| return $"{this._parentAttribute?.Definition.Designation}: {this._element}"; | ||
| return $"{this._parentAttribute?.Definition.Designation}: {this._element}{(this._aggregateType is { } aggreg ? $"->({aggreg})" : string.Empty)}"; |
There was a problem hiding this comment.
So that one can see that this aggregate type has been overriden.
E.g.: {Physical Base Damage: 20 (AddRaw)->(AddFinal)}
1 participant
To-do
Developments
Stats.PollutionMoveTargetChanceandStats.StunChancefor the more generic and descriptiveStats.MasteryMoveTargetChanceandStats.MasteryStunChance, which can serve several classesBugfixes
SkillsInitizalizerBase.CreateEffect()(bug introduced in previous PR)