Fix Decay and Ice Storm area skill settings#749
Fix Decay and Ice Storm area skill settings#749itayalroy wants to merge 1 commit intoMUnique:masterfrom
Conversation
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 adjusts the area skill settings for Decay and Ice Storm to ensure they match the expected Season 6 game mechanics. By modifying both the initial configuration and providing a migration path for existing environments, the changes ensure consistent behavior regarding target area detection and hit timing across all server instances. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new plug-in to align the area skill settings for Decay and Ice Storm with Season 6 server behavior, including updates to the initialization logic and versioning. Regarding the implementation, the DefaultAreaSkillSettings defines ProjectileCount as 1, which may create inconsistencies with other initialization paths where this property is not explicitly set. Additionally, the Where clause in the plug-in could be simplified for better readability.
| foreach (var skill in gameConfiguration.Skills.Where(skill => skill.Number is (short)SkillNumber.Decay or (short)SkillNumber.IceStorm | ||
| || skill.MasterDefinition?.ReplacedSkill?.Number is (short)SkillNumber.Decay or (short)SkillNumber.IceStorm)) |
There was a problem hiding this comment.
The Where clause is slightly complex and could be simplified for better readability. Additionally, consider that if gameConfiguration.Skills is a large collection, this filter runs for every skill. While not a performance bottleneck here, it's a good practice to keep filters concise.
foreach (var skill in gameConfiguration.Skills.Where(s => s.Number is (short)SkillNumber.Decay or (short)SkillNumber.IceStorm
|| s.MasterDefinition?.ReplacedSkill?.Number is (short)SkillNumber.Decay or (short)SkillNumber.IceStorm))| MinimumNumberOfHitsPerTarget = 1, | ||
| MaximumNumberOfHitsPerTarget = 1, | ||
| HitChancePerDistanceMultiplier = 1.0f, | ||
| ProjectileCount = 1, |
There was a problem hiding this comment.
In DefaultAreaSkillSettings, ProjectileCount is explicitly set to 1. However, in the corresponding changes in AddAreaSkillSettingsUpdatePlugIn.cs and SkillsInitializer.cs, this property is not explicitly set, which might lead to an inconsistency where fresh installations have a default value (likely 0 for an int) while updated installations have 1. If 1 is the intended value for these skills, it should be consistently applied across all initialization paths.
fc351a3 to
0652da7
Compare
|
Thanks, it looks fine. I'll need to test it when I have time |
50998c1 to
402a137
Compare
|
@sven-n I've added a fix to hellfire (target area was too big) and hellfire strengthener (target area was 0, skill did not hit any monster) |
402a137 to
9bf30ba
Compare
|
@sven-n Actually, I've been comparing skill effect area implementation in OpenMU vs MuAlfa open source (https://github.com/creadormu/GameServer), and they use euclidean distance to check which monsters are in range, while OpenMU is using manhattan distance (which creates a square effect area)- is it intentional? In any case, it seems that on legacy MU implementations, the client chooses which monsters are hit, and the server only validates it against some sanity value. I think it's worth to thoroughly align OpenMU skill effect areas with the legacy MU client, because it is very likely that we have additional skills with similar issues |
Fix AreaSkillSettings of a few mage skills: