Remove drop level gap condition for jewels#697
Remove drop level gap condition for jewels#697ze-dom wants to merge 8 commits intoMUnique:masterfrom
Conversation
| } | ||
| else | ||
| { | ||
| filteredPossibleItems = [.. selectedGroup.PossibleItems.Where(it => it.DropLevel == 0 || (it.DropLevel <= monsterLevel && it.DropLevel > monsterLevel - 12))]; |
There was a problem hiding this comment.
That's a good question... I'm not sure if it is the correct value for all items. This would mean that Items can drop up to Level 5. If I remember correctly, this wasn't possible for all items. E.g. leather boots and gloves dropped until level 5, while other armor parts were limited to 3 or 4.
This 12 is btw at two places, it probably needs a constant, or to defined in ItemDefinition.
There was a problem hiding this comment.
Apparently the old Monster.xml database had an maxitemlevel attribute, which defined exactly that (zTeamS6.3: 1, 2; emu: 1, 2).
There was a problem hiding this comment.
That's custom stuff, the original server didn't have xml config files ;-)
Let's just keep it like it is, for the moment.
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
Good and simple solution. I had a way more complicated solution in mind, but I like yours more 😅
Please just move this one method to the base class, instead of inheriting every class from Jewels.
| /// Initializer for pets. | ||
| /// </summary> | ||
| public class Pets : InitializerBase | ||
| public class Pets : Jewels |
There was a problem hiding this comment.
This looks strange. Can you move the AddItemToJewelItemDrop to InitializerBase instead?
|
Ok. What about the remarks in the PR description, what do you want to do about those? |
| } | ||
| else | ||
| { | ||
| filteredPossibleItems = [.. selectedGroup.PossibleItems.Where(it => it.DropLevel == 0 || (it.DropLevel <= monsterLevel && it.DropLevel > monsterLevel - DropLevelMaxGap))]; |
There was a problem hiding this comment.
After adding the DropLevelMaxGap constant, the previous thread disappeared: #697 (comment)
|
All set. Please check my remarks in the original PR description. You may want to do something about those. |
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 refines the item drop mechanics by eliminating a restrictive level gap condition for jewels and other valuable consumables like Imp, Angel, Uniria, Ale, and Town Portal Scrolls. This ensures these items can consistently drop from a wider range of monster levels, improving game balance and player experience. The changes involve categorizing these items with a new 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. Changelog
Activity
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
|
![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
The pull request successfully implements the removal of the drop level gap condition for jewels and similar items by introducing a new SpecialItemType.Jewel and updating the drop generation logic. It also includes the necessary data initialization updates across different game versions. The refactoring of AddItemToJewelItemDrop into a base class is a good improvement for code reusability. However, there are several instances of hardcoded values and magic numbers that could be improved for better maintainability and readability.
| if (monsterLevel > 66) | ||
| { | ||
| // Jewel of Chaos doesn't drop after a certain monster level | ||
| filteredPossibleItems.RemoveAll(it => it.Group == 12 && it.Number == 15); |
There was a problem hiding this comment.
The logic for the 'Jewel of Chaos' not dropping after monster level 66 is hardcoded directly into the drop generation. Consider externalizing this rule into a configurable setting or a data-driven approach, especially if similar exceptions might arise for other items in the future.
| /// <param name="item">The jewel you want to register.</param> | ||
| protected void AddItemToJewelItemDrop(ItemDefinition item) | ||
| { | ||
| var id = GuidHelper.CreateGuid<DropItemGroup>(4); |
There was a problem hiding this comment.
The magic number 4 used in GuidHelper.CreateGuid<DropItemGroup>(4) to identify the jewel drop group should be replaced with a named constant for improved readability and maintainability.
| public override bool IsMandatory => true; | ||
|
|
||
| /// <inheritdoc /> | ||
| public override DateTime CreatedAt => new(2026, 3, 5, 16, 0, 0, DateTimeKind.Utc); |
There was a problem hiding this comment.
The CreatedAt property is set to a future date (2026, 3, 5). This property should reflect the actual date when this plugin was created or introduced into the codebase, not a future date.
| /// <inheritdoc /> | ||
| protected override async ValueTask ApplyAsync(IContext context, GameConfiguration gameConfiguration) | ||
| { | ||
| var jewelsDropGroupId = new Guid("00000200-0004-0000-0000-000000000000"); |
There was a problem hiding this comment.
The GUID string "00000200-0004-0000-0000-000000000000" for jewelsDropGroupId is a magic string. It would be beneficial to define this as a named constant to enhance readability and reduce the risk of errors if the GUID needs to be referenced elsewhere.
| { ItemGroups.Misc1, [0, 1, 2] }, // angel, imp, uniria | ||
| { ItemGroups.Misc2, [9, 10] }, // alcohol, town portal scroll |
There was a problem hiding this comment.
The item numbers 0, 1, 2 for ItemGroups.Misc1 and 9, 10 for ItemGroups.Misc2 are magic numbers. Consider defining these as named constants or enum members to make the code more self-documenting and easier to understand, especially for items like 'angel', 'imp', 'uniria', 'alcohol', and 'town portal scroll'.
| { | ||
| await base.ApplyAsync(context, gameConfiguration).ConfigureAwait(false); | ||
|
|
||
| var jewelOfGuardianDropGroupId = new Guid("00000200-001f-0001-0000-000000000000"); |
There was a problem hiding this comment.
The GUID string "00000200-001f-0001-0000-000000000000" for jewelOfGuardianDropGroupId is a magic string. It should be defined as a named constant for better readability and maintainability.
2 participants
Todo
This fix removes the drop level gap condition for Jewels, Imp, Angel, Uniria, Ale, and Town Portal Scroll, which was preventing these items from dropping by mobs with higher levels than that limit.
Remarks
Let me know your opinion on the following points, as to whether we should add them or not:
Apparently in zTeamS6.3, they moved the jewel item checks somewhere else and mobs didn't drop them. Maybe they sold them in NPC stores or something.