DK Master Tree Finishup#766
Conversation
| { | ||
| 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; | ||
| } |
| /// 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 ?
There was a problem hiding this comment.
It's just theoretical, but it's used just in one special code path. So, just retrieve it locally.
| 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)}
|
Check the changes. Converted to draft because still need to update the UpdatePlugins after discussions. |
| var durabilityReductionFactorBoost = new PassiveSkillBoostPowerUp(skillEntry, true); | ||
| this.PassivePowerUps.Add(durabilityReductionFactorBoost); | ||
| this.PassivePowerUps.Add(new PowerUpWrapper(durabilityReductionFactorBoost, Stats.DurabilityReductionFactor, this._player.Attributes!)); | ||
| } |
| baseAttributeValues.Add(this.CreateConstValueAttribute(1, Stats.DefenseDecrement)); | ||
| baseAttributeValues.Add(this.CreateConstValueAttribute(1, Stats.SwellLifeHealthIncrease)); | ||
| baseAttributeValues.Add(this.CreateConstValueAttribute(1, Stats.SwellLifeManaIncrease)); | ||
| baseAttributeValues.Add(this.CreateConstValueAttribute(0.1f, Stats.DurabilityReductionFactor)); |
There was a problem hiding this comment.
Code Review
This pull request implements comprehensive updates to the Dark Knight master tree, refines double-wielding damage calculations, and introduces several mastery-related attributes and effects, such as Mace Mastery stun and a new Twisting Slash mastery plugin. Feedback highlights a critical logic error in attribute value caching that could zero out base stats, a potential out-of-bounds access in the twisting slash plugin, and a comparison bug in magic effect initialization that may cause duplicate configuration entries.
| if (this.Elements.All(e => e.AggregateType == AggregateType.Multiplicate)) | ||
| { | ||
| rawValues = 1; | ||
| } |
There was a problem hiding this comment.
The removal of the logic that sets multiValues to 1 when no multipliers are present is a critical issue. In the previous implementation, if no Multiplicate elements existed, multiValues was set to 1 to ensure that rawValues (the sum of base stats and item options) wasn't wiped out by the multiplication in the final formula: (rawValues * multiValues) + finalValues.
With the current change, if an attribute has only AddRaw elements (like basic Strength or Agility), multiValues will remain 0, resulting in a final value of 0. The new check only handles the case where only multipliers exist, but it fails to protect the base values when no multipliers exist.
| if (!currentMap.Terrain.WalkMap[nextTarget.X, nextTarget.Y] | ||
| || (target is NonPlayerCharacter && target.CurrentMap.Terrain.SafezoneMap[nextTarget.X, nextTarget.Y])) |
There was a problem hiding this comment.
Accessing WalkMap and SafezoneMap using nextTarget.X and nextTarget.Y without verifying that these coordinates are within the terrain's boundaries can lead to an IndexOutOfRangeException. This is particularly likely if a target is pushed near the edges of the map. Please add a bounds check before accessing the terrain arrays.
| e => e.Number == (short)effectNumber | ||
| && e.SubType == (byte)(0xFF - type) | ||
| && Equals(e.Duration?.ConstantValue.Value, durationInSeconds) | ||
| && Equals(e.Chance?.ConstantValue.Value, chance) |
There was a problem hiding this comment.
The comparison Equals(e.Chance?.ConstantValue.Value, chance) will fail to match existing effects that do not have a Chance defined (where e.Chance is null) when the method is called with the default chance = 0. This is because null is not equal to 0 in C#. This will result in the creation of duplicate MagicEffectDefinition objects in the configuration.
Consider using the null-coalescing operator to treat a missing chance as 0.
&& Equals(e.Chance?.ConstantValue.Value ?? 0, chance)
| /// <summary> | ||
| /// The version of the <see cref="FinishDarkKnightMasterTreePlugInSeason6"/>. | ||
| /// </summary> | ||
| FinishDarkKnightMasterTreeSeason6 = 83, |
There was a problem hiding this comment.
@ze-dom just a heads-up, there is a previous PR that uses versions 81-83 https://github.com/MUnique/OpenMU/pull/758/changes#diff-6889cdca23864773c06302aebda55880cf940922c4ec509ca5a8d62d7b033516
It's a small PR, for this reason I believe it will be merged soon.
| if ((SkillNumber)skill.Number is SkillNumber.ChainDrive) | ||
| { | ||
| skill.ElementalModifierTarget = Stats.IceResistance.GetPersistent(this.GameConfiguration); | ||
| skill.SkipElementalModifier = true; | ||
| skill.MagicEffectDef = this.CreateEffect(ElementalType.Ice, MagicEffectNumber.Cold, Stats.IsIced, 10, 0.4f); | ||
| return; | ||
| } |
| var magicEffect = new MagicEffect(TimeSpan.FromSeconds(2), stunEffectDefinition, [new MagicEffect.ElementWithTarget(powerUp, Stats.IsStunned)]); | ||
| await attackable.MagicEffectList.AddEffectAsync(magicEffect).ConfigureAwait(false); | ||
| } | ||
|
|
|
Integrated the changes from #770. |
There was a problem hiding this comment.
Code Review
This pull request completes the Dark Knight master tree skills and effects, fixes double wield damage calculations, and implements the Life Swell proficiency effect. It introduces several new attributes, skill plugins (such as Twisting Slash Mastery), and updates durability reduction mechanics. While the changes are comprehensive, several critical issues were identified in the review: a bug in ComposableAttribute.cs where empty attributes default to a raw value of 1; an incorrect initial value calculation for durability reduction in SkillList.cs when skills are loaded above level 1; missing copies of AggregateType and MaximumValue when duplicating power-up definitions for the Life Swell proficiency effect; and a potential crash in the Season 6 update plugin due to the unsafe use of First() instead of FirstOrDefault().
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (this.Elements.All(e => e.AggregateType == AggregateType.Multiplicate)) | ||
| { | ||
| rawValues = 1; | ||
| } |
There was a problem hiding this comment.
If this.Elements is empty, this.Elements.All(...) will evaluate to true, which incorrectly sets rawValues = 1 for an empty attribute. This causes empty attributes to have a default value of 1 instead of 0. Adding a check for this.Elements.Any() prevents this bug.
if (this.Elements.Any() && this.Elements.All(e => e.AggregateType == AggregateType.Multiplicate))
{
rawValues = 1;
}| if (isDurabilityReductionFactor) | ||
| { | ||
| this.Value = -1 / 500f; | ||
| this.AggregateType = AggregateType.AddRaw; | ||
| this._skillEntry.PropertyChanged += this.OnDurabilityReductionSkillEntryOnPropertyChanged; | ||
| } |
There was a problem hiding this comment.
The constructor hardcodes this.Value = -1 / 500f; when isDurabilityReductionFactor is true, which assumes the skill is at level 1. If the skill is loaded from the database at a higher level, the initial value will be incorrect until the level changes. It should use this._skillEntry.Level instead of 1.
if (isDurabilityReductionFactor)
{
this.Value = -this._skillEntry.Level / 500f;
this.AggregateType = AggregateType.AddRaw;
this._skillEntry.PropertyChanged += this.OnDurabilityReductionSkillEntryOnPropertyChanged;
}| powerUpCopy.Boost = this.Context.CreateNew<PowerUpDefinitionValue>(); | ||
| powerUpCopy.Boost.ConstantValue.Value = powerUp.Boost!.ConstantValue.Value; |
There was a problem hiding this comment.
When copying the power up definitions from the base GreaterFortitude effect, the AggregateType and MaximumValue of the boost are not copied. This means the proficiency effect won't inherit the MaximumValue limit (which is 2f for the health boost) or the correct AggregateType.
powerUpCopy.Boost = this.Context.CreateNew<PowerUpDefinitionValue>();
powerUpCopy.Boost.ConstantValue.Value = powerUp.Boost!.ConstantValue.Value;
powerUpCopy.Boost.ConstantValue.AggregateType = powerUp.Boost.ConstantValue.AggregateType;
powerUpCopy.Boost.MaximumValue = powerUp.Boost.MaximumValue;| powerUpCopy.Boost = context.CreateNew<PowerUpDefinitionValue>(); | ||
| powerUpCopy.Boost.ConstantValue.Value = powerUp.Boost!.ConstantValue.Value; |
There was a problem hiding this comment.
When copying the power up definitions from the base GreaterFortitude effect in the update plugin, the AggregateType and MaximumValue of the boost are not copied. This means the proficiency effect won't inherit the MaximumValue limit or the correct AggregateType.
powerUpCopy.Boost = context.CreateNew<PowerUpDefinitionValue>();
powerUpCopy.Boost.ConstantValue.Value = powerUp.Boost!.ConstantValue.Value;
powerUpCopy.Boost.ConstantValue.AggregateType = powerUp.Boost.ConstantValue.AggregateType;
powerUpCopy.Boost.MaximumValue = powerUp.Boost.MaximumValue;| } | ||
|
|
||
| // Restore iced effect (revert bug) | ||
| if (gameConfiguration.MagicEffects.First(e => e.Number == (short)MagicEffectNumber.Iced && e.Chance is { }) is { } originalIced) |
There was a problem hiding this comment.
Using First with a specific condition (e.Chance is { }) is risky in an update plugin. If the iced effect doesn't have a chance set (e.g., if the bug wasn't present or the database is in a different state), First will throw an exception and crash the update process. Using FirstOrDefault is much safer.
if (gameConfiguration.MagicEffects.FirstOrDefault(e => e.Number == (short)MagicEffectNumber.Iced && e.Chance is { }) is { } originalIced)3 participants
To-do
Developments
SkillsInitializerBase.ApplyElementalModifier()as it's shared with Chain Drive and Strike of DestructionStats.PollutionMoveTargetChanceandStats.StunChancefor the more generic and descriptiveStats.MasteryMoveTargetChanceandStats.MasteryStunChance, which can serve several classesBugfixes
SkillsInitizalizerBase.CreateEffect()(bug introduced in previous PR)