Repository navigation
feat(items): add the talisman of resurrection - #1461
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 | 47 |
| Duplication | 24 |
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: 5f85456a7c
ℹ️ 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".
| || player.CurrentMiniGame is not null | ||
| || player.DuelRoom is not null | ||
| || player.GuildWarContext is not null |
There was a problem hiding this comment.
During an active Castle Siege, participants use a separate CastleSiegeContext, so none of these exclusions applies. If this selector runs before CastleSiegePlugIn, for example after the siege plugin is deactivated and reactivated, it sets args.Gate first. CastleSiegePlugIn.SelectSpawnGateAsync then skips its side-specific gate because it requires args.Gate to be null. A talisman consequently lets attackers respawn behind the castle gates instead of returning to their side's respawn area. Exclude active siege participants or otherwise guarantee that siege gate selection precedes this consumptive fallback.
Useful? React with 👍 / 👎.
| public override string Name => PlugInName; | ||
|
|
||
| /// <inheritdoc /> | ||
| public override string Description => PlugInDescription; |
There was a problem hiding this comment.
For German or Chinese admin sessions, the Updates page still displays these English constants. Updates.razor.cs forwards IConfigurationUpdatePlugIn.Name and .Description directly, so the localized [Display] attribute does not make the newly added translations appear on that page. This violates the user-facing localization requirement in rule 10 of docs/CODING_RULES.md. Return the corresponding PlugInResources strings from both overrides, as resource-backed update plugins already do.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
| args.Gate = new ExitGate | ||
| { | ||
| Map = map.Definition, |
There was a problem hiding this comment.
When a lethal hit destroys a character's only flying mount on Icarus, AttackByAsync has already called OnDeathAsync before DecreasePetDurabilityAsync attempts the mandatory safezone evacuation. WarpToSafezoneAsync invokes this selector too, so the dead character's talisman is consumed and the required move to Devias is replaced with its current Icarus position, even though it no longer satisfies the map's CanFly requirement. Restrict this consumption to actual death-respawn requests and preserve mandatory evacuation when the current map's requirements are no longer met.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c06b1922a6
ℹ️ 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".
| await inventory.RemoveItemAsync(talisman).ConfigureAwait(false); | ||
| await player.PersistenceContext.DeleteAsync(talisman).ConfigureAwait(false); |
There was a problem hiding this comment.
If a player disconnects during the three-second death delay on an ordinary map, Player.DisconnectAsync first changes its state to Disconnected, then HandleMoveToNextSafezoneAsync cancels the respawn and calls WarpToSafezoneAsync. That cleanup path invokes this selector and deletes a talisman, but PlayerMapTransitions.WarpToAsync immediately returns for disconnected players. The cash-shop item is therefore permanently consumed without performing a resurrection. Reject disconnected/finished players here, or defer consumption until a death respawn is actually executed.
Useful? React with 👍 / 👎.
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
711bb9b to
7f99b43
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f99b43f8e
ℹ️ 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 (new CashShopItems(context, gameConfiguration).CreateTalismanOfResurrection() is { } talisman) | ||
| { | ||
| ItemRules.Apply([talisman]); |
There was a problem hiding this comment.
When upgrading a Season 6 database that has not yet installed AddCashShopUpdatePlugIn, that older update now creates 13/69 through CashShopItems.Initialize() before this update runs. CreateTalismanOfResurrection() consequently returns null here, so ItemRules.Apply is skipped and the item retains ItemDefinition's default permissive flags. MoveItemAction, DropItemAction, and SellItemToNpcAction therefore allow trading, dropping, and selling the paid item on this upgrade path, although fresh databases correctly prohibit those actions. Apply the restrictions when either update creates the item, and cover the chained upgrade to satisfy rule 4 of docs/CODING_RULES.md, which requires handling every upgrade path.
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. The code looks good to me: the spawn-gate plugin skips mini games, duels, guild wars, castle siege participants and maps whose requirements the player no longer fulfills, and the update plugin is localized and only touches the client-script shop.
Unfortunately, #1504 was merged in the meantime and regenerated GameLogic/Properties/PlugInResources.Designer.cs, so this PR conflicts again (GameLogic/Properties/PlugInResources.*). Please rebase once more: keep the entries of both sides in the .resx files, then regenerate the designer. Once it's clean and green, I'll merge it.
(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
The Talisman of Resurrection (13/69) was missing, so its packages of the in-game shop couldn't be sold. When a player dies while it carries one in its inventory, a spawn gate plugin respawns it at the place of its death instead of the safezone, and one talisman is used up. It doesn't apply in mini games, duels and guild wars, which have their own spawn gates. An optional update adds the item to existing Season 6 databases and puts its two packages on sale, when the shop uses the client's script. Part of MUnique#1339. 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
7f99b43 to
61ee619
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61ee6193b7
ℹ️ 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".
| await inventory.RemoveItemAsync(talisman).ConfigureAwait(false); | ||
| await player.PersistenceContext.DeleteAsync(talisman).ConfigureAwait(false); |
There was a problem hiding this comment.
When an operator enables stacking for the talisman by increasing its definition's durability, or configures another stackable resurrection item, these calls delete the entire stack for one resurrection. DataModel.ItemExtensions.IsStackable identifies such items, and InventoryProductDelivery.CreateItems stores their quantity in Item.Durability, so a single death permanently discards all remaining units instead of consuming the documented one talisman. Decrement the stack quantity and send a durability update, removing and deleting the item only when its last unit is consumed.
Useful? React with 👍 / 👎.
The Talisman of Resurrection (13/69) was missing, so its packages of the in-game shop couldn't be sold. When a player dies while it carries one in its inventory, a spawn gate plugin respawns it at the place of its death instead of the safezone, and one talisman is used up. It doesn't apply in mini games, duels and guild wars, which have their own spawn gates.
An optional update adds the item to existing Season 6 databases and puts its two packages on sale, when the shop uses the client's script.
Part of #1339.
Claude-Session: https://claude.ai/code/session_011xNc75fJTee8EPxDxQxuGi