-
Notifications
You must be signed in to change notification settings - Fork 457
Rage Fighter Killing Blow, Beast Uppercut, Chain Drive, Dragon Roar, Phoenix Shot skills completion #683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @ze-dom, 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 significantly enhances the combat system by introducing multi-hit capabilities for several Rage Fighter skills. It involves fundamental changes to skill definitions, attack processing logic, and client-server communication to accurately represent and display these multi-hit sequences and their associated new magic effects. The changes aim to complete the implementation of these specific Rage Fighter skills, making them fully functional within the game. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces multi-hit skill mechanics for the Rage Fighter class, which is a significant feature addition. The changes span across the data model, game logic, networking, and persistence layers. While the implementation is largely on the right track, there are a few areas that need attention. The logic for handling multi-hit attacks is duplicated in several places, which should be refactored for better maintainability. I've also found a critical bug in the new DragonRoarSkillPlugIn that incorrectly handles single-hit skills. Additionally, there are some minor code quality issues like an inconsistent parameter name and other duplicated logic that should be addressed. Addressing these points will improve the correctness and quality of the code.
| if (i <= 3 || Rand.NextRandomBool()) | ||
| { | ||
| // first three 100% chance, others 50% chance | ||
| await extraTarget.AttackByAsync(attacker, skillEntry, false, 1, false).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug in how the first hit is handled. The isFinalStreakHit parameter is hardcoded to false. If a skill has only one hit (skill.NumberOfHitsPerAttack == 1), this parameter should be null. This will cause incorrect behavior for single-hit skills that might use this logic. Please use the same pattern as in AreaSkillAttackAction to correctly handle both single and multi-hit skills.
await extraTarget.AttackByAsync(attacker, skillEntry, false, 1, skill.NumberOfHitsPerAttack > 1 ? false : null).ConfigureAwait(false);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not possible in this context
|
|
||
| /// <inheritdoc /> | ||
| public async ValueTask<HitInfo?> AttackByAsync(IAttacker attacker, SkillEntry? skill, bool isCombo, double damageFactor = 1.0) | ||
| public async ValueTask<HitInfo?> AttackByAsync(IAttacker attacker, SkillEntry? skill, bool isCombo, double damageFactor = 1.0, bool? isStreakFinalHit = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter isStreakFinalHit should be renamed to isFinalStreakHit to match the method signature in the IAttackable interface. Maintaining consistency with the interface definition is important for readability and prevents confusion.
public async ValueTask<HitInfo?> AttackByAsync(IAttacker attacker, SkillEntry? skill, bool isCombo, double damageFactor = 1.0, bool? isFinalStreakHit = null)| if (isFinalStreakHit.HasValue) | ||
| { | ||
| hitInfo.Attributes |= DamageAttributes.RageFighterStreakHit; | ||
|
|
||
| if (isFinalStreakHit.Value || this.Attributes[Stats.CurrentHealth] < 1) | ||
| { | ||
| hitInfo.Attributes |= DamageAttributes.RageFighterStreakFinalHit; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic for setting the RageFighterStreakHit and RageFighterStreakFinalHit attributes is duplicated from AttackableNpcBase.HitAsync. To improve maintainability and prevent future inconsistencies, this logic should be extracted into a shared helper method. An extension method on HitInfo, such as hitInfo.SetRageFighterStreakAttributes(isFinalStreakHit, isKilled), could be a good approach.
| var hitInfo = await target.AttackByAsync(player, skillEntry, isCombo, 1, skill.NumberOfHitsPerAttack > 1 ? false : null).ConfigureAwait(false); | ||
| await target.TryApplyElementalEffectsAsync(player, skillEntry).ConfigureAwait(false); | ||
|
|
||
| for (int hit = 2; hit <= skill.NumberOfHitsPerAttack; hit++) | ||
| { | ||
| await target.AttackByAsync(player, skillEntry, isCombo, 1, hit == skill.NumberOfHitsPerAttack).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for performing a multi-hit attack is duplicated in TargetedSkillDefaultPlugin.cs and DragonRoarSkillPlugIn.cs. To improve maintainability and ensure consistent behavior, this logic should be extracted into a single, reusable helper method. For instance, an extension method like IAttackable.AttackWithMultiHitsAsync(...) would be a suitable solution.
| if (isFinalStreakHit.HasValue) | ||
| { | ||
| hitInfo.Attributes |= DamageAttributes.RageFighterStreakHit; | ||
|
|
||
| if (isFinalStreakHit.Value || killed) | ||
| { | ||
| hitInfo.Attributes |= DamageAttributes.RageFighterStreakFinalHit; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zTeamS6.3, MuMain 5.2
Apparently the original didn't check if the target has died, which would cause the damage values not to appear if the target didn't die on the final hit: https://www.youtube.com/watch?v=Vo6aI34Y3FE
But this way we can always show the damage values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zTeamS6.3 -> ObjUseSkill.cpp: UseSkill(), SkillAreaMonsterAttack()
| /// <summary> | ||
| /// Gets the range around the target, in which additional targets are searched. | ||
| /// </summary> | ||
| protected virtual short Range => 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zTeamS6.3. This is the range from the target.
| public override short Key => 270; | ||
|
|
||
| /// <inheritdoc /> | ||
| protected override short Range => 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zTeamS6.3. This is the range from the target.
| return false; | ||
| } | ||
|
|
||
| var extraTargets = target.CurrentMap?.GetAttackablesInRange(target.Position, this.Range).Where(FilterTarget).Take(7); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zTeamS6.3. We take only 7 since we have already hit 1 earlier and 8 is the maximum for the skill.
| { | ||
| if (i <= 3 || Rand.NextRandomBool()) | ||
| { | ||
| // first three 100% chance, others 50% chance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zTeamS6.3 -> ObjUseSkill.cpp: UseSkill(), SkillAreaMonsterAttack()
| packet.HealthDamage = @healthDamage; | ||
| packet.Kind = @kind; | ||
| packet.IsRageFighterStreakHit = @isRageFighterStreakHit; | ||
| packet.IsRageFighterStreakFinalHit = @isRageFighterStreakFinalHit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was IsRageFighterStreakHit & IsRageFighterStreakFinalHit added as a custom addition or is it because these new fields hadn't been added to the main version (version 5.2) yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They aren't custom. They were simply missing in the server. As you can see, the client has them.
| /// Light pink color, usually used by reflected damage. | ||
| /// </summary> | ||
| LightPink = 4, | ||
| ReflectedLightPink = 4, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| powerUpDefinition.Boost = this.Context.CreateNew<PowerUpDefinitionValue>(); | ||
| powerUpDefinition.Boost.ConstantValue.Value = 16f; | ||
| powerUpDefinition.Boost.ConstantValue.AggregateType = AggregateType.AddFinal; | ||
| powerUpDefinition.Boost.MaximumValue = 200f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zTeamS6.3 (no data), emu, MuMain 5.2, PR#652
| { SkillNumber.Innovation, MagicEffectNumber.Innovation }, | ||
| { SkillNumber.DamageReflection, MagicEffectNumber.Reflection }, | ||
| { SkillNumber.BeastUppercut, MagicEffectNumber.DefenseReductionBeastUppercut }, | ||
| { SkillNumber.PhoenixShot, MagicEffectNumber.DecreaseBlock }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| { SkillNumber.Weakness, MagicEffectNumber.WeaknessSummoner }, | ||
| { SkillNumber.Innovation, MagicEffectNumber.Innovation }, | ||
| { SkillNumber.DamageReflection, MagicEffectNumber.Reflection }, | ||
| { SkillNumber.BeastUppercut, MagicEffectNumber.DefenseReductionBeastUppercut }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| skill.MagicEffectDef.Chance = this.Context.CreateNew<PowerUpDefinitionValue>(); | ||
| skill.MagicEffectDef.Chance.ConstantValue.Value = 0.4f; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// The decrease block effect of the rage fighter. | ||
| /// </summary> | ||
| DecreaseBlock = 132, | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emu: BuffEffect_.h, ObjUseSkill.cpp
| this.CreateSkill(SkillNumber.KillingBlow, "Killing Blow", CharacterClasses.AllFighters, DamageType.Physical, distance: 2, manaConsumption: 9, elementalModifier: ElementalType.Earth); | ||
| this.CreateSkill(SkillNumber.BeastUppercut, "Beast Uppercut", CharacterClasses.AllFighters, DamageType.Physical, distance: 2, manaConsumption: 9, elementalModifier: ElementalType.Fire); | ||
| this.CreateSkill(SkillNumber.ChainDrive, "Chain Drive", CharacterClasses.AllFighters, DamageType.Physical, distance: 4, abilityConsumption: 20, manaConsumption: 15, levelRequirement: 150, elementalModifier: ElementalType.Ice); | ||
| this.CreateSkill(SkillNumber.KillingBlow, "Killing Blow", CharacterClasses.AllFighters, DamageType.Physical, distance: 2, manaConsumption: 9, elementalModifier: ElementalType.Earth, hitsPerAttack: 4); // 1 packet => 1*4 hits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| this.CreateSkill(SkillNumber.BeastUppercut, "Beast Uppercut", CharacterClasses.AllFighters, DamageType.Physical, distance: 2, manaConsumption: 9, elementalModifier: ElementalType.Fire); | ||
| this.CreateSkill(SkillNumber.ChainDrive, "Chain Drive", CharacterClasses.AllFighters, DamageType.Physical, distance: 4, abilityConsumption: 20, manaConsumption: 15, levelRequirement: 150, elementalModifier: ElementalType.Ice); | ||
| this.CreateSkill(SkillNumber.KillingBlow, "Killing Blow", CharacterClasses.AllFighters, DamageType.Physical, distance: 2, manaConsumption: 9, elementalModifier: ElementalType.Earth, hitsPerAttack: 4); // 1 packet => 1*4 hits | ||
| this.CreateSkill(SkillNumber.BeastUppercut, "Beast Uppercut", CharacterClasses.AllFighters, DamageType.Physical, distance: 2, manaConsumption: 9, elementalModifier: ElementalType.Fire, hitsPerAttack: 2); // 2 packets => 2*2 hits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| this.CreateSkill(SkillNumber.ChainDrive, "Chain Drive", CharacterClasses.AllFighters, DamageType.Physical, distance: 4, abilityConsumption: 20, manaConsumption: 15, levelRequirement: 150, elementalModifier: ElementalType.Ice); | ||
| this.CreateSkill(SkillNumber.KillingBlow, "Killing Blow", CharacterClasses.AllFighters, DamageType.Physical, distance: 2, manaConsumption: 9, elementalModifier: ElementalType.Earth, hitsPerAttack: 4); // 1 packet => 1*4 hits | ||
| this.CreateSkill(SkillNumber.BeastUppercut, "Beast Uppercut", CharacterClasses.AllFighters, DamageType.Physical, distance: 2, manaConsumption: 9, elementalModifier: ElementalType.Fire, hitsPerAttack: 2); // 2 packets => 2*2 hits | ||
| this.CreateSkill(SkillNumber.ChainDrive, "Chain Drive", CharacterClasses.AllFighters, DamageType.Physical, distance: 4, abilityConsumption: 20, manaConsumption: 15, levelRequirement: 150, elementalModifier: ElementalType.Ice, hitsPerAttack: 4); // 2 packets => 2*4 hits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| this.CreateSkill(SkillNumber.ChainDrive, "Chain Drive", CharacterClasses.AllFighters, DamageType.Physical, distance: 4, abilityConsumption: 20, manaConsumption: 15, levelRequirement: 150, elementalModifier: ElementalType.Ice, hitsPerAttack: 4); // 2 packets => 2*4 hits | ||
| this.CreateSkill(SkillNumber.DarkSide, "Dark Side", CharacterClasses.AllFighters, DamageType.Physical, distance: 4, manaConsumption: 70, levelRequirement: 180, elementalModifier: ElementalType.Wind); | ||
| this.CreateSkill(SkillNumber.DragonRoar, "Dragon Roar", CharacterClasses.AllFighters, DamageType.Physical, distance: 3, abilityConsumption: 30, manaConsumption: 50, levelRequirement: 150, elementalModifier: ElementalType.Earth, skillType: SkillType.AreaSkillAutomaticHits); | ||
| this.CreateSkill(SkillNumber.DragonRoar, "Dragon Roar", CharacterClasses.AllFighters, DamageType.Physical, distance: 3, abilityConsumption: 30, manaConsumption: 50, levelRequirement: 150, elementalModifier: ElementalType.Earth, skillType: SkillType.AreaSkillExplicitTarget, hitsPerAttack: 4); // 1 packet => 1*4 hits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| this.CreateSkill(SkillNumber.IncreaseBlock, "Increase Block", CharacterClasses.AllFighters, DamageType.Physical, distance: 7, abilityConsumption: 10, manaConsumption: 50, levelRequirement: 50, energyRequirement: 80, skillType: SkillType.Buff, skillTarget: SkillTarget.ImplicitParty); | ||
| this.CreateSkill(SkillNumber.Charge, "Charge", CharacterClasses.AllFighters, DamageType.Physical, 90, 4, 15, 20); | ||
| this.CreateSkill(SkillNumber.PhoenixShot, "Phoenix Shot", CharacterClasses.AllFighters, DamageType.Physical, distance: 4, manaConsumption: 30, elementalModifier: ElementalType.Earth, skillType: SkillType.AreaSkillExplicitTarget); | ||
| this.CreateSkill(SkillNumber.PhoenixShot, "Phoenix Shot", CharacterClasses.AllFighters, DamageType.Physical, distance: 4, manaConsumption: 30, elementalModifier: ElementalType.Earth, skillType: SkillType.AreaSkillExplicitTarget, hitsPerAttack: 4); // 1 packet => 1*4 hits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| { | ||
| result += (int)masterSkillEntry.CalculateValue(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the TargetAttribute is null we assume the skill adds damage. We need to go all the way back to the ancestor master skills because some may add damage.
| attribute.AddElement(new SimpleElement { Value = attrValue }); | ||
| } | ||
|
|
||
| attributeDictionary.Add(targetAttribute, attribute); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need this to make Stats.DefenseDecrement work. Do you see any issues?
| result.AttributeCombinations.Add(this.CreateAttributeRelationship(Stats.MaximumHealth, 1, finalBerserkerHealthDecrement, InputOperator.Add, AggregateType.Multiplicate)); | ||
| result.AttributeCombinations.Add(this.CreateConditionalRelationship(Stats.DefensePvm, statsDefense, finalBerserkerHealthDecrement, AggregateType.AddFinal)); | ||
| result.AttributeCombinations.Add(this.CreateConditionalRelationship(Stats.DefensePvp, statsDefense, finalBerserkerHealthDecrement, AggregateType.AddFinal)); | ||
| result.AttributeCombinations.Add(this.CreateConditionalRelationship(Stats.DefenseFinal, statsDefense, finalBerserkerHealthDecrement, AggregateType.AddFinal)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Fire Slash's reduction (now pooled into Stats.DefenseDecrement) is no longer applied before the berserker penalty, we can simplify this.
| this.AddMasterSkillDefinition(SkillNumber.KillingBlowMastery, SkillNumber.KillingBlowStrengthener, SkillNumber.Undefined, 2, 3, SkillNumber.KillingBlowStrengthener, 20, $"{Formula120} / 100", Formula120, Stats.WeaknessPhysDmgDecrement, AggregateType.AddRaw); | ||
| this.AddMasterSkillDefinition(SkillNumber.BeastUppercutMastery, SkillNumber.BeastUppercutStrengthener, SkillNumber.Undefined, 2, 3, SkillNumber.BeastUppercutStrengthener, 20, $"{Formula120} / 100", Formula120, Stats.DefenseDecrement, AggregateType.Multiplicate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.



🚧 WIP 🚧
To-do:
Developments:
Bugfixes: