Repository navigation
feat(items): use figurines and charms inside the inventory - #1470
icheat2win wants to merge 1 commit into
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 83 |
| Duplication | 18 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79006ad10a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| item.Height = height; | ||
| item.Durability = 255; |
There was a problem hiding this comment.
For a single charm created through the GM item command or a permanent shop product, these slot-less definitions are classified as stackable by DataModel.ItemExtensions.IsStackable() because their definition durability is 255. GameMasterItemFactory.CreateItem and InventoryProductDelivery.CreateItems therefore initialize the item's durability to 1, which SetItemInUseAsync rejects because it only accepts 254/255. The same classification also lets stack moves modify these state bytes as quantities without updating the bonuses. Exclude inventory-use items from quantity/stack handling and initialize their instances to the inactive state.
Useful? React with 👍 / 👎.
| if (fromItemStorage == player.Inventory && toItemStorage != player.Inventory && item.IsInUseInsideInventory()) | ||
| { | ||
| // An item which is used inside the inventory, like a charm, is only used while it stays in the inventory. | ||
| item.Durability = InventoryUsedItemExtensions.NotInUseDurability; |
There was a problem hiding this comment.
If an active charm is dropped, DropItemAction removes it through InventoryStorage.RemoveItemAsync without reaching this reset, so its durability stays 254. A player can activate one copy, drop it, activate another copy of the same kind, and pick up the first: InventoryStorage.AddItemAsync reinstates its power-ups without the duplicate check from SetItemInUseAsync. Both copies then grant their bonuses, and the sequence can be repeated. Clear the use state on real inventory exits such as drops, not only on storage moves.
Useful? React with 👍 / 👎.
| /// <inheritdoc /> | ||
| protected override ValueTask ApplyAsync(IContext context, GameConfiguration gameConfiguration) | ||
| { | ||
| new CashShopItems(context, gameConfiguration).CreateFigurinesAndCharms(); |
There was a problem hiding this comment.
On an existing Season 6 database with the client-script shop catalog, product sequences 459–462 were initialized with null ItemDefinition references because these items did not exist. This update creates the definitions but never reconnects those products, whereas fresh initialization does. CashShopProductDelivery.GetDelivery consequently continues rejecting them even if the operator enables permanent variants. Rule 4 of docs/CODING_RULES.md requires upgrades to “ensure the final state”; relink the known null catalog references idempotently and cover that pre-upgrade state in the repair test.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
| return item.Definition is { } definition | ||
| && ItemConstants.ItemsUsedInsideInventory.Any(id => id.Group == definition.Group && id.Number == definition.Number); |
There was a problem hiding this comment.
This helper decides inventory-use behavior solely from the fixed group/number list in ItemConstants, without an item-configuration or feature-configuration option. Rule 3 of docs/CODING_RULES.md forbids these configuration decisions in generic GameLogic and explicitly states that “Being hard-coded in the game client is not an exemption,” so the client-based justification on the new constants does not satisfy the rule. Represent eligibility in item configuration or a configurable feature plugin and have the shared logic read it instead of baking 13/128–134 into the behavior.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
| else if (item.IsUsedInsideInventory()) | ||
| { | ||
| // An item which is used inside the inventory loses its power-ups, until it's added again. | ||
| this._player.Attributes?.RemoveInventoryUsedItemPowerUps(item); |
There was a problem hiding this comment.
With an active Oak Charm and current health above the unbuffed maximum, rearranging the charm within the inventory calls RemoveItemAsync, which disposes its power-up before AddItemAsync reinstalls it. Player.OnAttributeValueChanged immediately clamps current health to the temporarily lowered maximum, so moving the charm permanently loses health even though its use state never changes. The corresponding mana, shield, and ability bonuses have the same problem. Preserve the power-ups during inventory-only moves, or otherwise avoid publishing the intermediate lower maximum.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27f7dfd029
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (item == this._itemMovedInsideInventory | ||
| || !item.IsUsedInsideInventory(this._gameContext)) | ||
| { | ||
| return; |
There was a problem hiding this comment.
If an operator disables the feature or removes an item from its configuration while a player has an active charm, the live plugin configuration makes IsUsedInsideInventory return false. This guard then skips cleanup when the player drops or vaults that charm, leaving its power-ups in ItemPowerUps and granting the bonus after the item has left the inventory. Remove already-applied power-ups independently of current eligibility, and reset the item's use state on actual inventory exits.
Useful? React with 👍 / 👎.
| } | ||
| else | ||
| { | ||
| this.OnItemAddedToInventory(convertedItem ?? item); |
There was a problem hiding this comment.
When a player picks up a GM-created charm, GameMasterItemFactory supplies a TemporaryItem with durability 1. This new call normalizes only its persistent copy to 255. DroppedItem.TryPickUpAsync copies only the slot back to the temporary item, and PickupItemAction.PickupItemAsync passes that original object to IItemAppearPlugIn, so the item-added packet contains durability 1 while the server's inventory contains 255. Normalize the source too, or serialize the inserted persistent item, so the pickup response carries the actual inventory-use state.
Useful? React with 👍 / 👎.
| item.Durability = requestedDurability; | ||
| attributes.UpdateInventoryUsedItemPowerUps(item, player.GameContext); |
There was a problem hiding this comment.
If an already-active Oak Charm receives another valid activation request with durability 254, this unconditionally rebuilds its power-ups even though its state has not changed. UpdateInventoryUsedItemPowerUps disposes the existing bonus first, and Player.OnAttributeValueChanged immediately clamps current health to the temporarily lowered maximum before the bonus is restored. A repeated state-setting request therefore loses health when it exceeds the unbuffed maximum; the mana, shield, and ability bonuses have the same problem. Acknowledge unchanged requests without removing and recreating their power-ups.
Useful? React with 👍 / 👎.
|
I think some of the values are not correct (https://muonlinefanz.com/guide/systems/cash-item-shop/#talisman). I will check it out more thoroughly and come back. |
|
Shots taken with S6E3 client. Hawk and Goat figurines/status add to Critical Damage and Excelent Damage chance, not bonus (tooltip incorrect/incomplete): 
Oak Charm is correct. Golden Oak Charm adds 100 HP and 500 SD. 
Maple Charm and Golden Maple Charm have no rendering. Maybe they were never released or later? MU Fanz don't have them listed either. But the values are correct. 
|
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
This PR conflicts with current master, so I can't merge it. Please rebase it onto master. These files conflict:
src/GameLogic/Properties/PlugInResources.Designer.cssrc/GameLogic/Properties/PlugInResources.resxsrc/Persistence/Initialization/Properties/ItemNames.Designer.cssrc/Persistence/Initialization/Properties/ItemNames.de.resxsrc/Persistence/Initialization/Properties/ItemNames.resxsrc/Persistence/Initialization/Properties/ItemNames.zh-CN.resxsrc/Persistence/Initialization/Properties/PlugInResources.Designer.cssrc/Persistence/Initialization/Properties/PlugInResources.de.resxsrc/Persistence/Initialization/Properties/PlugInResources.resxsrc/Persistence/Initialization/Properties/PlugInResources.zh-CN.resxtests/MUnique.OpenMU.Persistence.Initialization.Tests/ItemStatProfilesDataTest.cs
Most of them are resource files (.resx / .Designer.cs) which other merged PRs also extended. When you resolve them, keep the entries of both sides, and regenerate the designer file instead of merging it by hand. I'll review it in full once it's rebased.
Generated by Claude Code
27f7dfd to
807ef82
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 807ef8278c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public override string Name => PlugInName; | ||
|
|
||
| /// <inheritdoc /> | ||
| public override string Description => PlugInDescription; |
There was a problem hiding this comment.
In a German or Chinese admin-panel session, the configuration-updates page still displays this update in English: Updates.UpdateViewModel reads IConfigurationUpdatePlugIn.Name and Description directly, not the localized [Display] metadata. These overrides return English constants instead of the translated resource entries added by this commit. Rule 10, “No user-facing literals,” in docs/CODING_RULES.md requires localized plugin and admin-panel text; return the corresponding resource properties here as well.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
Thanks for the rebase. Using the items inside the inventory looks well thought out: the state lives in the durability (254/255), only one item of a kind is in use, and the use ends when the item leaves the inventory but not when it moves within it.
Unfortunately, #1504 was merged in the meantime and regenerated GameLogic/Properties/PlugInResources.Designer.cs, so this PR conflicts again (GameLogic/Properties/PlugInResources.*, PlayerMessage.*, GameServer/Properties/PlugInResources.*). Please rebase once more: keep the entries of both sides in the .resx files, then regenerate the designers.
While you're at it: the description says the Worn Horseshoe (13/134) is left out because there's no attribute for the item drop rate yet. #1459 is merged now and added Stats.BonusItemDropRate, so the horseshoe could be added, either here or in a follow-up.
(Your item PRs #1458, #1461, #1464, #1470 and #1471 all add to the same resource files, so each merge makes the others conflict again. I'll take whichever is ready first.)
Generated by Claude Code
Adds the Hawk and Goat Figurines and the (Golden) Oak and Maple Charms (13/128-133) of issue MUnique#1339. A right click inside the inventory (BF 20) puts them in use, which is stored in the durability like in the original game (254 in use, 255 not in use). While in use, their power-ups apply. Only one item of the same kind can be in use, and the use ends when the item leaves the inventory. The Worn Horseshoe (13/134) is left out, because there's no attribute for the item drop rate yet. Rebased onto master: this is the change of the earlier commits in one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xNc76CH3CGWUp5r4XmXWK
807ef82 to
f9cd937
Compare
Adds the Hawk and Goat Figurines and the (Golden) Oak and Maple Charms (13/128-133) of issue #1339. A right click inside the inventory (BF 20) puts them in use, which is stored in the durability like in the original game (254 in use, 255 not in use). While in use, their power-ups apply. Only one item of the same kind can be in use, and the use ends when the item leaves the inventory.
The Worn Horseshoe (13/134) is left out, because there's no attribute for the item drop rate yet.
Claude-Session: https://claude.ai/code/session_011xNc75fJTee8EPxDxQxuGi