Repository navigation
feat(items): add the gladiator's honor - #1471
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
|
![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: 06f9fc3f4e
ℹ️ 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".
| /// Gets the experience rate which is added while the player is in Vulcanus, e.g. by the Gladiator's Honor. | ||
| /// </summary> | ||
| /// <remarks>The experience is multiplied by one plus this rate by the <c>VulcanusExperiencePlugIn</c>.</remarks> | ||
| public static AttributeDefinition VulcanusExperienceRate { get; } = new(new Guid("199015AC-3139-4E6B-A7C0-5439DD4F7052"), "Vulcanus Experience Rate", "The experience rate which is added while the player is in Vulcanus."); |
There was a problem hiding this comment.
The only producer of this attribute is the new Gladiator's Honor effect in BuffItems, and its only consumer is VulcanusExperiencePlugIn. This violates rule 3 of docs/CODING_RULES.md: “Don't introduce new stats for one specific item or skill.” It turns this item's implementation detail into a permanent persisted attribute/GUID contract and requires additional attribute-creation handling during upgrades. Keep the map restriction in the feature plugin and represent the bonus through existing experience attributes or item/plugin configuration instead.
AGENTS.md reference: AGENTS.md:L16-L17
Useful? React with 👍 / 👎.
| /// </summary> | ||
| /// <param name="attribute">The attribute.</param> | ||
| /// <returns>The persistent attribute definition.</returns> | ||
| private AttributeDefinition GetOrCreateAttribute(AttributeDefinition attribute) |
There was a problem hiding this comment.
Replace by extension method StatsExtensions.GetPersistent
sven-n
left a comment
sven-n
left a comment
There was a problem hiding this comment.
Review: Prefer a configuration-driven implementation
I'd like to change the approach for Gladiator's Honor. The current implementation contains too much feature-specific game logic. OpenMU's attribute system should already provide most of the required functionality through configuration.
Proposed approach
-
Add
Stats.IsInVulcanusas a flag attribute. Set it to1through Vulcanus'sGameMapDefinition.CharacterPowerUpDefinitions. The existingMapPowerUpsinfrastructure automatically applies and removes it when entering or leaving the map. -
Keep
Stats.VulcanusExperienceRateas the attribute contributed by Gladiator's Honor (0.2). -
Add a conditional attribute relationship to the relevant Season 6 character classes, so the Vulcanus bonus only contributes while
IsInVulcanusis active. -
The experience bonus must be multiplicative, not additive. For example, an existing experience rate of
1.5should become1.8, not1.7.One possible solution is a generic
Stats.ExperienceGainMultiplier, with a base value of1.0and this relationship:CreateConditionalRelationship( Stats.ExperienceGainMultiplier, Stats.IsInVulcanus, Stats.VulcanusExperienceRate);
Apply this multiplier after the existing experience-rate calculation in
PlayerExperience.csand the corresponding calculation inParty.cs. This would provide a reusable mechanism for future experience modifiers. -
Remove
VulcanusExperiencePlugInand the duplicate map configuration. Neither should be necessary when the bonus is calculated through attributes.
The map-use restriction may still require a small consume-handler plugin if the item must only be usable inside Vulcanus. However, the experience calculation itself should not require a dedicated plugin.
Initialization and testing
Please ensure that both fresh initialization and the configuration update for existing databases contain the new attributes, map power-up and conditional relationships. The update should be idempotent.
Tests should cover entering and leaving Vulcanus, buff expiration, normal and master experience, party experience, and interaction with existing experience bonuses.
The goal is to keep the feature almost entirely configuration-driven, with only minimal, reusable changes to generic game logic. This is more consistent with CODING_RULES.md, especially rule 3.
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.resxsrc/Persistence/Initialization/VersionSeasonSix/Items/BuffItems.cstests/MUnique.OpenMU.Persistence.Initialization.Tests/BuffItemsDataTest.cstests/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
06f9fc3 to
a336f06
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a336f062d1
ℹ️ 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".
| /// Tests that the item can't be used outside of Vulcanus, so it isn't used up. | ||
| /// </summary> | ||
| [Test] | ||
| public async Task CanNotBeUsedOutsideOfVulcanusAsync() |
There was a problem hiding this comment.
The four new tests here, plus the new data test and its configuration factory, return Task or Task<T>. Rule 11 of docs/CODING_RULES.md, adopted as the review baseline by the root AGENTS.md, explicitly requires: “Asynchronous methods return ValueTask / ValueTask<T>.” Change these newly introduced signatures to the required return types; the nearby BuffItemConsumptionTest already demonstrates ValueTask-returning NUnit tests.
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 Gladiator's Honor looks good to me: it can only be used on the configured map, and the experience bonus comes from its own stat, which only applies while the player is in Vulcanus.
Unfortunately, #1504 was merged in the meantime and regenerated GameLogic/Properties/PlugInResources.Designer.cs, so this PR conflicts again (GameLogic/Properties/PlugInResources.*, PlayerMessage.*). 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
Adds the Gladiator's Honor (13/95) of issue MUnique#1339. Like the game client describes it, it can only be used in Vulcanus and increases the experience there by 20 %. Its effect adds the new Stats.VulcanusExperienceRate, which the new VulcanusExperiencePlugIn applies while the player is in Vulcanus. The map is configurable. The client doesn't define its duration, so it lasts like a seal (30 minutes). The higher drop rate of rare items is left out. 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
a336f06 to
ca30a97
Compare
Adds the Gladiator's Honor (13/95) of issue #1339. Like the game client describes it, it can only be used in Vulcanus and increases the experience there by 20 %. Its effect adds the new
Stats.VulcanusExperienceRate, which the new VulcanusExperiencePlugIn applies while the player is in Vulcanus. The map is configurable.
The client doesn't define its duration, so it lasts like a seal (30 minutes). The higher drop rate of rare items is left out.
Claude-Session: https://claude.ai/code/session_011xNc75fJTee8EPxDxQxuGi