Conversation
Adds IsTradable, IsDroppable, IsStorable, IsSellableToNpc, IsPersonalStoreSellable and IsRepairable to ItemDefinition. They are true unless set otherwise, and the migration adds them with the default true, so existing items keep allowing everything. The admin panel shows them as checkboxes on the item pages. They match the rule flags of the MuMain client item data (tradable, droppable, storable, sellable, personalShopSellable, repairable), so both sides can allow the same actions.
- MoveItemAction: an item can't be moved into a trade, the vault or the personal store when IsTradable, IsStorable or IsPersonalStoreSellable is false. - SellItemToNpcAction: IsSellableToNpc. - DropItemAction: IsDroppable. - ItemRepairAction: IsRepairable; repair all skips such items. A refused action shows a message to the player and logs a warning: the client should not have sent it, so its item data may differ from the server (or the client is modified). IsBoundToCharacter keeps working as before.
ItemRules lists the Season 6 items which don't allow every action, with the values of the MuMain client item data (src/bin/Data/Items). It is applied by the Season 6 initialization and, for existing databases, by the mandatory AddItemRuleFlagsPlugIn update. Two differences to the client data: - The Wizards Ring is bound to the character here, so it is not tradable, storable or sellable in a personal store (the client allows that at +0; its data will be adjusted). - The Dark Horse and Dark Raven stay repairable, because the server repairs trainable pets.
Mosch0512
left a comment
Mosch0512
left a comment
There was a problem hiding this comment.
Code review: 9 findings. 7 are inline comments. These 2 are in files the PR doesn't touch:
1. Offline auto-repair floods the log with unrepairable rings (src/GameLogic/Offline/RepairHandler.cs:76)
PerformRepairsAsync calls RepairItemAsync on every equipped item at or below 50% durability and doesn't check IsRepairable. Rings lose durability on hits (IsDefenseItemSlot includes both ring slots). With a worn-down Transformation Ring (13,10), Wizards Ring (13,20) or Skeleton ring equipped, the tick runs twice a second and each time logs "client item data may differ from the server" and sends ItemCannotBeRepaired, forever. BotShoppingHandler.RepairGearAsync (line 381) does the same on every merchant trip. Skip IsRepairable: false items in both loops.
2. Bots try to sell items that can't be sold to NPCs, then destroy them (src/GameLogic/Bots/BotShoppingHandler.cs:277)
GetSellableJunk doesn't filter on IsSellableToNpc. A looted Box of Kundun (14,11, any level) now fails in SellJunkAsync and again in ClearUnsoldAsync (line 313), with a misleading warning each time. When the backpack is under slot pressure it is then destroyed through DestroyInventoryItemAsync; before this PR it was sold for Zen. Filter these items out of the junk list (keep them, or treat them as dead weight on purpose).
|
|
||
| return toStorage switch | ||
| { | ||
| Storages.Trade when !definition.IsTradable => nameof(PlayerMessage.ItemCannotBeTraded), |
There was a problem hiding this comment.
The no-trade rule can be bypassed through the other temporary-storage windows. This check only fires for Storages.Trade, but ChaosMachine, PetTrainer, Refinery etc. use the same player.TemporaryStorage. CloseNpcDialogAction doesn't empty it, OpenTradeAsync doesn't clear it, and TradeButtonAction (line 81) hands over whatever TemporaryStorage.Items holds.
How it happens: a modified client moves a Demon (13,64), which is not tradable and not bound, into Storages.ChaosMachine. Only IsBoundToCharacter is checked there. It then closes the dialog, requests a trade, accepts it, and the Demon goes to the partner.
| return; | ||
| } | ||
|
|
||
| if (fromStorage != toStorage && GetRuleBlockingMove(item, toStorage) is { } blockedMessage) |
There was a problem hiding this comment.
Altitude: the rules are only checked when an item is moved in. Nothing checks them when the trade completes (TradeButtonAction) or when a personal-store sale happens (BuyRequestAction). Items already in those storages are never checked. For example, a Moonstone Pendant (13,38) put in a personal store before the AddItemRuleFlags update ran can still be bought afterwards. Checking at completion closes this gap and the temporary-storage bypass above together.
| return; | ||
| } | ||
|
|
||
| if (item.Definition is { IsRepairable: false }) |
There was a problem hiding this comment.
Altitude: the stack-repair bug is patched per item instead of fixed at the cause. The cause is repairing items that can't be worn: for them GetMaximumDurabilityOfOnePiece() returns 1, so the stack collapses to 1. The list in ItemRules only protects the listed Season 6 items. The same bug remains for 0.75/0.95d databases (left unchanged by this PR), custom items and stackables the client data doesn't flag. A !item.IsWearable() guard here and in RepairAllItemsAsync fixes it for every version without any data.
|
|
||
| await new ItemRepairAction().RepairItemAsync(player, InventorySlot).ConfigureAwait(false); | ||
|
|
||
| Assert.That(item.Durability, isRepairable ? Is.Not.EqualTo(5) : Is.EqualTo(5)); |
There was a problem hiding this comment.
The repairable case passes because of the stack-to-1 bug. CreateItem gives the definition no ItemSlot, so the item can't be worn, GetMaximumDurabilityOfOnePiece() returns 1, and the repair sets Durability from 5 to 1. Is.Not.EqualTo(5) passes on exactly the data loss the PR describes. Use a wearable item (with an ItemSlot) and assert the maximum durability.
| /// <param name="gameConfiguration">The game configuration.</param> | ||
| internal static void Apply(GameConfiguration gameConfiguration) | ||
| { | ||
| var items = gameConfiguration.Items.ToDictionary(item => (item.Group, item.Number)); |
There was a problem hiding this comment.
ToDictionary throws on duplicate (Group, Number) items. Existing databases can have two item definitions with the same group and number, for example when an admin copied one in the admin panel. The mandatory AddItemRuleFlagsPlugIn then fails with an ArgumentException. Iterate the items and look each one up in a dictionary built from Rules instead, which also sets the flags on every copy.
| (12, 17, Blocked.Repair), // Orb of Penetration | ||
| (12, 18, Blocked.Repair), // Orb of Ice Arrow | ||
| (12, 19, Blocked.Repair), // Orb of Death Stab | ||
| (13, 0, Blocked.Repair), // Guardian Angel |
There was a problem hiding this comment.
Repair is blocked on pets and rings that really wear out. Guardian Angel, Imp, Uniria, Dinorant, Fenrir and the transformation rings lose durability on hits; for them it's not a stack count. Until now the NPC repaired them (pet slot with an open NPC). Now a Dinorant keeps wearing down, and at 0 Player.DecreaseItemDurabilityAfterHitAsync destroys pets that can't be trained. If this follows the client data on purpose, please say so in the description. Right now it only explains blocking repair for stackable items.
| return gameConfiguration.Items.Single(item => item.Group == group && item.Number == number); | ||
| } | ||
|
|
||
| private static async Task<GameConfiguration> CreateSeason6ConfigurationAsync() |
There was a problem hiding this comment.
Efficiency: the full Season 6 data is built 4 times in this fixture. Build it once in [OneTimeSetUp] for the two read-only tests; only the update test needs a fresh copy it can change.
- Trade: the rules are also checked when the trade completes. Items which another window (e.g. the chaos machine) left in the temporary storage go back to the inventory when a trade opens; before, they could be traded, or were lost when the trade was cancelled. - Personal store: an item which isn't sellable there can't be bought, also when it was put into the store before the rules existed. - Repair: new Item.CanBeRepaired() (wearable and IsRepairable). Items which can't be worn are no longer repaired in any version: their durability is the stack size, and a repair reduced it to one. Offline auto repair and the bots' merchant repair skip items which can't be repaired instead of logging a warning on every attempt. - Bots keep items which can't be sold to NPCs instead of trying to sell and then destroying them. - ItemRules.Apply no longer fails on duplicate item definitions; every copy gets the rule. - The remarks of ItemRules say that the pets and transformation rings follow the client and can't be repaired. - Tests: the repair test uses a wearable item and checks the full durability; new tests for stacks, leftover items when a trade opens, the trade completion and personal store purchases. The data test builds the Season 6 data once for its read-only tests.
IsRepairable and Item.CanBeRepaired() are about the repair by an NPC or the inventory repair. Other ways to restore the durability don't use them: the Horn of Fenrir, which can't be repaired normally, is still repaired with a Jewel of Bless (the repair target items of the jewel of bless consume handler). A test covers that.
|
Superseded by MUnique#983, which contains these commits. |
1 participant
Server part of the data-driven items plan in MuMain (phase 3 / "PR A"): the client moved its item rules into data in sven-n/MuMain#635. This adds the same rules to OpenMU, so the server enforces what the client shows.
What changes
New fields on
ItemDefinition,trueunless set otherwise:IsTradableMoveItemAction(into a trade) andTradeButtonAction(when the trade completes)tradableIsStorableMoveItemAction(into the vault)storableIsPersonalStoreSellableMoveItemAction(into the personal store) andBuyRequestAction(when someone buys it)personalShopSellableIsSellableToNpcSellItemToNpcActionsellableIsDroppableDropItemActiondroppableIsRepairableItemRepairAction, via the newItem.CanBeRepaired()(repair all, offline auto repair and bot repairs skip such items)repairableIsRepairableonly covers the normal repair (NPC or inventory repair). Other ways to restore durability don't depend on it. For example, the Horn of Fenrir can't be repaired normally, but a Jewel of Bless still repairs it (configured as a repair target item of the Jewel of Bless consume handler). A test covers that.PlayerMessageentries) and logs a warning. The client should not have sent the request, so either its item data differs from the server or it is a modified client.IsBoundToCharacterworks as before (owner-only pickup, no moves between storages).AutoFields).Data
AddItemRuleFlagsadds the columns with the defaulttrue, so existing items keep allowing everything.VersionSeasonSix/Items/ItemRuleslists the 156 Season 6 items that don't allow every action. The values come from the client item data. The list is applied by the Season 6 initialization and, for existing databases, by the mandatoryAddItemRuleFlagsPlugInupdate.Two deliberate differences to the client data (and one deliberate match):
Why repair matters
For items that can't be worn, OpenMU counts durability as the stack size, and the maximum durability of one piece is 1. A repair request for a stack sets the durability to 1, and because the durability is above the maximum, the repair price is negative. Verified against master, in a unit test (a stack of 3 apples becomes 1 apple, and the player gains 1,158 Zen) and in the game: a MuMain client whose data allowed repairing apples sent the request, and the server turned the stack into one apple while adding Zen.
The normal client never sends this request, because it blocks repairing potions, so players don't run into it. A modified client could, and could farm Zen with it.
Item.CanBeRepaired()now refuses items that can't be worn, in every game version and for custom items, independent of the item data.Not included
Tests
ItemRuleFlagsTests:MoveItemActionTests: trade, vault and personal store.ItemRulesDataTest:ItemConsumptionTest: the Jewel of Bless repairs an item that can't be repaired normally.MUnique.OpenMU.Tests(1033) andMUnique.OpenMU.Persistence.Initialization.Tests(28, 6 skipped).