Implement McRPG statistics integration with PAPI placeholders and admin commands#212
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review WalkthroughThis PR implements comprehensive statistics integration with McCore, adding statistic registration during bootstrap, gameplay event tracking via dedicated listeners, player statistics persistence with database operations, admin commands for stat management, PAPI placeholders for offline/online lookups with caching, and complete configuration plus localization support. Changes
Sequence Diagram(s)sequenceDiagram
participant Player as Player (Online)
participant Listener as Statistic Listener
participant Registry as Statistic Registry
participant PlayerData as Player Statistic Data
Player->>Listener: Event fires (e.g., block break)
Listener->>Listener: Resolve McRPGPlayer via registry
Listener->>Registry: Query registered statistics by key
alt Statistic registered
Listener->>PlayerData: Increment value
PlayerData->>PlayerData: Mark dirty
else Statistic not registered
Listener->>Listener: Skip update
end
Note over Listener,PlayerData: Statistic tracked in-memory
sequenceDiagram
participant Admin as Admin
participant Command as Statistic Command
participant PlayerMgr as Player Manager
participant DB as Database
participant Cache as Statistic Cache
participant Main as Main Thread
Admin->>Command: Execute /mcrpg statistic view [player] [stat]
Command->>PlayerMgr: Resolve player (online check)
alt Player Online
Command->>Command: Read stat from in-memory data
Command->>Main: Send result message
else Player Offline
Command->>Main: Send loading message
Command->>DB: Async query PlayerStatisticDAO
DB-->>Cache: Result (on cache miss)
Cache->>Cache: Store entry
Command->>Main: Schedule delivery of result
Main->>Admin: Display stat value
end
Note over DB,Cache: Offline lookups leverage cache
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/hld/statistics/statistics-integration-hld.md`:
- Around line 273-274: Search the documentation examples for the incorrect enum
identifier "McRPGMcRPGGainReason" and replace it with the correct
"McRPGGainReason"; for example change "return McRPGMcRPGGainReason.BLOCK_BREAK;"
and any similar occurrences (including the duplicate prefix at the other
example) to "return McRPGGainReason.BLOCK_BREAK;" so all examples use the
correct enum name.
- Line 58: The fenced code blocks in the HLD are missing language specifiers
(affecting syntax highlighting); update the two code fences referenced near the
`TOTAL_SKILL_EXPERIENCE` table row and the other block around line 138 by adding
appropriate language tags (for example use ```text for plain tables/CSV or
```java for Java snippets) so both fenced blocks include a language identifier;
ensure the opening fence for each block is changed to include the chosen
language (e.g., ```text or ```java) to enable proper highlighting.
In `@docs/lld/statistics/phase-2-statistics-integration-lld.md`:
- Around line 12-18: The status table in
docs/lld/statistics/phase-2-statistics-integration-lld.md incorrectly marks
Sub-Phase 2.4 as "Not Started" while the file already contains the 2.4 content
(and phase-3-papi-commands-config-lld.md covers the same scope); reconcile by
choosing a canonical location and updating the table: either move/merge the PAPI
Placeholders, Commands & Config content into a single canonical file (preferably
phase-3-papi-commands-config-lld.md) and delete the duplicate section from
phase-2-statistics-integration-lld.md, or leave the content in phase-2 and
update the status cell for Sub-Phase 2.4 to "Complete" and add a cross-reference
note to phase-3-papi-commands-config-lld.md; ensure you update the table row for
Sub-Phase 2.4 and remove or add a clarifying comment in the duplicated sections
to avoid future confusion.
- Line 128: The internal link to `#known-issue-overflow-xp-at-max-level` is
broken because that heading does not exist; either add a new section titled
"Known Issue: Overflow XP at Max Level" (or exact heading that matches the
anchor) describing the overflow XP behavior and implications for
`SkillGainExpEvent` XP tracking, or remove/replace the reference in the sentence
that mentions `SkillGainExpEvent` so it points to an existing heading; ensure
the anchor text exactly matches the heading if you add the section so the
internal link resolves.
In `@docs/lld/statistics/phase-3-papi-commands-config-lld.md`:
- Around line 1-7: The document title "Phase 3: PAPI Placeholders, Commands &
Config" (phase-3-papi-commands-config-lld.md) conflicts with its parent link to
statistics-integration-hld.md labeled "Phase 4" and the Phase 2 LLD reference
phase-2-statistics-integration-lld.md (Sub-Phase 2.4); decide which canonical
phase this content belongs to (e.g., Phase 2.4, Phase 3, or Phase 4), then
update the heading in phase-3-papi-commands-config-lld.md, the Parent HLD
label/target (statistics-integration-hld.md), and the Phase 2 LLD reference
(phase-2-statistics-integration-lld.md) so all three files consistently use the
chosen phase numbering and correct cross-reference text/links; also search for
and update any remaining references/TOC entries that mention these phase numbers
to keep documentation consistent.
- Line 210: Replace the raw e.printStackTrace() calls with structured logging:
create or reuse a java.util.logging.Logger instance (e.g., private static final
Logger LOGGER = Logger.getLogger(CurrentClassName.class.getName())) in the
classes where the stack trace appears and replace the printStackTrace()
invocation with LOGGER.log(Level.SEVERE, "Failed to populate cache" or another
contextual message, e), ensuring you import java.util.logging.Level and
java.util.logging.Logger; do this for the occurrence in the current file and the
one in StatisticViewCommand (around the previously noted line ~697).
- Line 4: The relative link in
docs/lld/statistics/phase-3-papi-commands-config-lld.md (the string
'../../../../McCore/docs/hld/statistics/statistics-framework-hld.md') assumes a
specific repo layout; replace that relative path with a stable absolute GitHub
URL to the McCore document (e.g.,
https://github.com/<org>/McCore/blob/<branch>/docs/hld/statistics/statistics-framework-hld.md)
or alternatively add a brief note in the same Markdown pointing out the required
repo layout/submodule expectation so consumers know how to resolve the link;
update the link text to match the new URL and ensure the branch (develop/main)
is correct.
In `@src/main/java/us/eunoians/mcrpg/command/redeem/RedeemExperienceCommand.java`:
- Around line 109-115: The redeem logic allows a negative `amount` to become a
negative `effectiveAmount`, causing unintended calls to
skillData.addExperience() and
playerExperienceExtras.modifyRedeemableExperience(); fix this in
RedeemExperienceCommand by clamping the input `amount` to a non-negative value
before computing `effectiveAmount` (e.g., replace usage of `amount` in the
Math.min call with a non-negative value via Math.max(0, amount) or otherwise
ensure effectiveAmount = Math.min(Math.max(0, amount), Math.max(0,
expToMaxLevel))); then proceed to call skillData.addExperience(effectiveAmount,
...) and playerExperienceExtras.modifyRedeemableExperience(effectiveAmount * -1)
as before.
In `@src/main/java/us/eunoians/mcrpg/command/statistic/StatisticListCommand.java`:
- Around line 98-107: In the SQLException catch block inside
StatisticListCommand (catch (SQLException e)) replace e.printStackTrace() with
the plugin logger: obtain the mcRPG plugin logger (mcRPG.getLogger()) and log
the error with context and the exception (e.g., logger.log(Level.SEVERE, "Failed
to lookup statistics for sender: " + sender.getName(), e)) so the error is
recorded consistently; keep the existing CoreTask that sends
LocalizationKey.STATISTIC_LOOKUP_ERROR_MESSAGE to the sender unchanged.
In
`@src/main/java/us/eunoians/mcrpg/command/statistic/StatisticResetCommand.java`:
- Around line 145-146: Replace the direct e.printStackTrace() calls in
StatisticResetCommand with the plugin's logging mechanism: catch blocks in
methods (refer to class StatisticResetCommand and the exception handlers around
the current e.printStackTrace() usages) should call the plugin or SLF4J logger
to log a clear error message and the exception (e.g.,
plugin.getLogger().log(...) or logger.error(..., e)) instead of printing the
stack trace; make the same replacement for the other occurrence later in the
class (the second printStackTrace near line 211) so all exceptions use
consistent structured logging.
In `@src/main/java/us/eunoians/mcrpg/command/statistic/StatisticSetCommand.java`:
- Around line 157-158: The code in StatisticSetCommand currently throws
NumberFormatException for the TIMESTAMP and SET_STRING branches which is
misleading; change those throws to a more appropriate exception (e.g., throw new
IllegalArgumentException("Timestamp statistics cannot be set via command") and
similarly for SET_STRING) in the switch handling of statistic types, and then
update the error handling where NumberFormatException is caught (e.g., in the
command execution method) to separately catch IllegalArgumentException (or your
custom UnsupportedStatisticException) so parse errors and unsupported-statistic
errors are handled and reported distinctly.
- Around line 136-137: In StatisticSetCommand replace the e.printStackTrace()
call in the catch block with the plugin logger: obtain the plugin's logger
(e.g., via mcrpg.getLogger() or the plugin instance used elsewhere in this
class) and call logger.log(Level.SEVERE, "Failed to set statistic for player
<playerIdentifier or variable>", e) or logger.log(Level.SEVERE, "Descriptive
message", e) to include the exception; import java.util.logging.Level if needed
to ensure consistent error logging like in StatisticResetCommand.
In `@src/main/java/us/eunoians/mcrpg/command/statistic/StatisticViewCommand.java`:
- Around line 41-44: The public static method registerCommand() in
StatisticViewCommand lacks Javadoc; add a Javadoc block immediately above the
method (public static void registerCommand()) that describes what the method
does (registers the statistic view command with the CommandManager), documents
that there are no parameters and no return value (use `@return` void or a brief
“void” statement per project style), and optionally references related classes
like McRPG and CommandManager/CommandSourceStack for context; ensure the Javadoc
follows the project’s formatting conventions and is present for all public
methods.
- Around line 112-120: In StatisticViewCommand (inside the catch block for
SQLException) remove the call to e.printStackTrace() and instead log the
exception via the plugin logger (e.g., mcRPG.getLogger()). Replace it with a
call that includes a clear context message and the exception object so the stack
trace is recorded by the logger (for example using
java.util.logging.Level.SEVERE with mcRPG.getLogger().log(..., e)); ensure the
log message references the failing operation (statistic lookup) so it’s easy to
find.
- Around line 65-66: The current use of Bukkit.getOfflinePlayer(playerName) in
StatisticViewCommand (variable target) can block the main thread; change the
lookup to first call Bukkit.getOfflinePlayerIfCached(playerName) and if that
returns null perform an asynchronous UUID resolution (e.g., using
Bukkit.getScheduler().runTaskAsynchronously or a CompletableFuture) to fetch
UUID and then obtain the OfflinePlayer on the main thread if needed; update the
code paths that use the OfflinePlayer (the target variable and any follow-up
logic in StatisticViewCommand) to handle the async callback or future so the
main thread is never blocked.
In `@src/main/java/us/eunoians/mcrpg/entity/holder/SkillHolder.java`:
- Around line 327-370: The Javadoc for SkillHolder.addExperience is incorrect:
it claims the method "Always returns 0", but the method returns the original
experience when the SkillGainExpEvent is cancelled (see addExperience and the
early return at the skillGainExpEvent.isCancelled() check). Update the method
Javadoc (the `@return` description) to state that it returns 0 when experience was
consumed/awarded, or returns the unconsumed experience (the original experience
value) if the SkillGainExpEvent was cancelled; mention the event cancellation
behavior and reference SkillGainExpEvent and addExperience in the comment.
In `@src/main/java/us/eunoians/mcrpg/expansion/content/StatisticContent.java`:
- Around line 46-50: Add a Javadoc comment to the public override
getExpansionKey() in class StatisticContent describing what the method returns
and any nullability semantics; include an `@return` tag that explains the
Optional<NamespacedKey> (e.g., present when the expansion key is set) and note
that the value may be empty, and include the `@Override` notice if desired by
style. Ensure the Javadoc sits immediately above the getExpansionKey() method
definition and references the return type Optional<NamespacedKey> and any
behavior callers should expect.
In `@src/main/java/us/eunoians/mcrpg/expansion/content/StatisticContentPack.java`:
- Around line 15-19: Add a Javadoc comment above the public constructor
StatisticContentPack(`@NotNull` ContentExpansion contentExpansion) that documents
the constructor and includes an `@param` tag for contentExpansion; update the
comment to briefly describe the purpose of the constructor (e.g., creating a
StatisticContentPack for the given ContentExpansion) and annotate the parameter
with `@param` contentExpansion the ContentExpansion instance used to initialize
the pack so it meets the project's Javadoc requirements.
In `@src/main/java/us/eunoians/mcrpg/expansion/McRPGExpansion.java`:
- Around line 113-133: The Javadoc on createAbilities() incorrectly references
{`@link` `#getStatisticContent`(List)}; update the doc to reference the actual
method signature {`@link` `#getStatisticContent`(List, List)} (or remove the
param-types from the link if you prefer a non-signature link) so the {`@link`
`#getAbilityContent`(List)}/ {`@link` `#getStatisticContent`(List, List)}
cross-reference is accurate; edit the comment above createAbilities() to replace
getStatisticContent(List) with getStatisticContent(List, List).
In
`@src/main/java/us/eunoians/mcrpg/external/papi/placeholder/McRPGPlaceHolderType.java`:
- Around line 86-107: Replace the current registration loop in
McRPGPlaceHolderType that iterates skill and ability keys with logic that
iterates the actual registered Statistic definitions from mcRPG.registryAccess()
(or wherever Statistics are exposed), and register a StatisticPlaceholder for
each Statistic using its canonical id (preserving namespace) and McRPGStatistic
keys rather than reconstructing names from getKey(); update the
mcRPGPapiExpansion.registerPlaceholder calls to use the Statistic's identifier
(e.g., statistic.getId() or equivalent) and the corresponding McRPGStatistic
lookup (e.g., McRPGStatistic.fromStatistic(statistic) or the factory that maps
Statistic -> stat key), so placeholders are created only for statistics that
actually exist and keep third‑party namespaces intact (adjust any helper methods
in McRPGStatistic or add a small adapter if needed).
In
`@src/main/java/us/eunoians/mcrpg/external/papi/placeholder/statistic/StatisticPlaceholder.java`:
- Around line 29-30: Replace the shared static NumberFormat/DecimalFormat fields
with ThreadLocal instances to avoid concurrent access; specifically change
INTEGER_FORMAT and DOUBLE_FORMAT to ThreadLocal<NumberFormat> and
ThreadLocal<DecimalFormat> (initialized to return a freshly configured
NumberFormat.getIntegerInstance() and new DecimalFormat("#,##0.00")
respectively) and update all usages in StatisticPlaceholder.formatValue (and any
other methods referencing INTEGER_FORMAT/DOUBLE_FORMAT) to call .get() on the
ThreadLocals before formatting.
In
`@src/main/java/us/eunoians/mcrpg/listener/statistic/CombatStatisticListener.java`:
- Around line 49-62: The current onEntityDeath handler records any kill where
entity.getKiller() is a Player as MOBS_KILLED, which wrongly counts PvP kills;
update the onEntityDeath(EntityDeathEvent) logic to ignore Player victims by
checking the dead entity's type (e.g., skip if event.getEntity() instanceof
Player) before calling getPlayer(killer) and incrementing
McRPGStatistic.MOBS_KILLED; keep the existing getPlayer(...) and
incrementLong(...) calls but only execute them when the victim is not a Player
(or rename the statistic if counting PvP is intended).
In `@src/main/java/us/eunoians/mcrpg/registry/manager/McRPGManagerKey.java`:
- Line 44: Add a Javadoc comment above the ManagerKey constant STATISTIC_CACHE
in McRPGManagerKey describing that it retrieves the McRPGStatisticCacheManager
instance from the manager registry and specifying which operations are safe
(e.g., read-only lookups and cached statistic retrievals) and which are not
(e.g., mutating global state, heavy I/O, or long-running tasks) and include any
thread-safety guarantees or expectations for callers when invoking methods on
McRPGStatisticCacheManager.
In
`@src/main/java/us/eunoians/mcrpg/skill/experience/context/BlockBreakContext.java`:
- Around line 18-22: Add a Javadoc block above the public override
getGainReason() in class BlockBreakContext describing the method's purpose and
return value; include an `@return` tag that states it returns the GainReason for
block-breaking, specifically McRPGGainReason.BLOCK_BREAK; ensure the doc notes
that this overrides the parent contract (since it's an `@Override`) and is public
API.
In
`@src/main/java/us/eunoians/mcrpg/skill/experience/context/EntityDamageContext.java`:
- Around line 18-22: The public override getGainReason() in class
EntityDamageContext lacks Javadoc; add a standard Javadoc block above the method
in EntityDamageContext describing what gain reason is returned, include a brief
description and an `@return` tag that specifies it returns
McRPGGainReason.ENTITY_DAMAGE (and optionally an `@see` to McRPGGainReason), so
the override has the required documentation for public methods.
In
`@src/main/java/us/eunoians/mcrpg/skill/experience/context/McRPGGainReason.java`:
- Around line 47-56: Add Javadoc comments to the public enum accessor methods in
McRPGGainReason: add a Javadoc block above getKey() that describes what
NamespacedKey is returned and include a `@return` tag, and add a Javadoc block
above getDisplayName() that describes the returned display name and include a
`@return` tag; ensure the Javadocs follow project style for public methods and
reference the method names getKey() and getDisplayName() in the comments.
- Around line 42-44: The constructor in McRPGGainReason uses
name().toLowerCase() which is locale-dependent; change the NamespacedKey
creation to use name().toLowerCase(Locale.ROOT) so key generation is
locale-stable (update the expression that assigns this.key in the
McRPGGainReason constructor), and add the java.util.Locale import if missing;
keep the rest (displayName assignment, McRPGMethods.getMcRPGNamespace())
unchanged.
In
`@src/main/java/us/eunoians/mcrpg/skill/experience/context/SkillExperienceContext.java`:
- Around line 72-81: SkillExperienceContext was changed to add a new abstract
method getGainReason(), which breaks external subclasses; change it from
abstract to a concrete method with a sensible default (e.g., return
GainReason.UNKNOWN or GainReason.OTHER) so existing subclasses won't be forced
to implement it, and keep the method non-final so subclasses can override it
when they need to; update the Javadoc for getGainReason() to mention the default
and that subclasses may override to provide specific GainReason values.
In `@src/main/java/us/eunoians/mcrpg/statistic/McRPGStatisticCacheManager.java`:
- Around line 95-97: In the catch(SQLException e) block in
McRPGStatisticCacheManager replace the e.printStackTrace() call with the plugin
logger: use getPlugin().getLogger().log(Level.WARNING, "Failed to populate
statistic cache for player " + uuid, e) (or plugin.getLogger().log(...)) so the
exception is logged consistently with the plugin's logging system and includes
the stack trace; ensure you import java.util.logging.Level if not already
present.
In `@src/main/resources/config.yml`:
- Around line 179-187: Update the statistics.cache block to document reload
behavior for each key (enabled, max-size, ttl) clarifying that these cache
settings require a full server restart rather than supporting /reload because
the Caffeine cache is constructed at startup, and also increment the global
config-version value from 1 to 2 to reflect the new structured section so config
migration tools and users are aware of the change.
In `@src/main/resources/localization/english/en_commands.yml`:
- Around line 43-55: The config file adds new localization sections (the
"statistic" keys such as statistic, statistic-view, statistic-set, etc.) which
are structural changes; update the top-level config-version value (the existing
config-version key) to the next integer (e.g., 2) so the system recognizes the
structural change and the new statistic entries are applied.
In `@src/test/java/us/eunoians/mcrpg/statistic/McRPGStatisticTest.java`:
- Around line 96-101: The test allStaticStatistics_isNotEmptyAndUnmodifiable
currently only checks emptiness and nullity but does not verify immutability;
update the method to assert that attempting to modify
McRPGStatistic.ALL_STATIC_STATISTICS (e.g., calling add(...) or clear()) throws
UnsupportedOperationException using assertThrows, and add the static import for
assertThrows (import static org.junit.jupiter.api.Assertions.assertThrows;) so
the test actually confirms the collection is unmodifiable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf8c0d8e-cce9-4ee2-b5cc-196f74d7c239
📒 Files selected for processing (58)
.cursor/rules/ability-system.mdc.cursor/rules/core.mdc.cursor/rules/skill-system.mdcCLAUDE.mdbuild.gradle.ktsdocs/hld/statistics/statistics-integration-hld.mddocs/lld/statistics/phase-2-statistics-integration-lld.mddocs/lld/statistics/phase-3-papi-commands-config-lld.mdsrc/main/java/us/eunoians/mcrpg/ability/Ability.javasrc/main/java/us/eunoians/mcrpg/ability/impl/type/ActiveAbility.javasrc/main/java/us/eunoians/mcrpg/bootstrap/McRPGBootstrap.javasrc/main/java/us/eunoians/mcrpg/bootstrap/McRPGCommandRegistrar.javasrc/main/java/us/eunoians/mcrpg/bootstrap/McRPGListenerRegistrar.javasrc/main/java/us/eunoians/mcrpg/command/CommandPlaceholders.javasrc/main/java/us/eunoians/mcrpg/command/give/GiveExperienceCommand.javasrc/main/java/us/eunoians/mcrpg/command/parser/StatisticParser.javasrc/main/java/us/eunoians/mcrpg/command/redeem/RedeemExperienceCommand.javasrc/main/java/us/eunoians/mcrpg/command/statistic/StatisticCommandBase.javasrc/main/java/us/eunoians/mcrpg/command/statistic/StatisticListCommand.javasrc/main/java/us/eunoians/mcrpg/command/statistic/StatisticResetCommand.javasrc/main/java/us/eunoians/mcrpg/command/statistic/StatisticSetCommand.javasrc/main/java/us/eunoians/mcrpg/command/statistic/StatisticViewCommand.javasrc/main/java/us/eunoians/mcrpg/configuration/file/MainConfigFile.javasrc/main/java/us/eunoians/mcrpg/configuration/file/localization/LocalizationKey.javasrc/main/java/us/eunoians/mcrpg/entity/holder/SkillHolder.javasrc/main/java/us/eunoians/mcrpg/entity/player/McRPGPlayer.javasrc/main/java/us/eunoians/mcrpg/event/skill/PostSkillGainExpEvent.javasrc/main/java/us/eunoians/mcrpg/event/skill/SkillGainExpEvent.javasrc/main/java/us/eunoians/mcrpg/expansion/McRPGExpansion.javasrc/main/java/us/eunoians/mcrpg/expansion/content/StatisticContent.javasrc/main/java/us/eunoians/mcrpg/expansion/content/StatisticContentPack.javasrc/main/java/us/eunoians/mcrpg/expansion/handler/ContentHandlerType.javasrc/main/java/us/eunoians/mcrpg/external/papi/placeholder/McRPGPlaceHolderType.javasrc/main/java/us/eunoians/mcrpg/external/papi/placeholder/statistic/StatisticPlaceholder.javasrc/main/java/us/eunoians/mcrpg/listener/skill/OnSkillLevelUpListener.javasrc/main/java/us/eunoians/mcrpg/listener/skill/SkillListener.javasrc/main/java/us/eunoians/mcrpg/listener/statistic/AbilityStatisticListener.javasrc/main/java/us/eunoians/mcrpg/listener/statistic/CombatStatisticListener.javasrc/main/java/us/eunoians/mcrpg/listener/statistic/SkillStatisticListener.javasrc/main/java/us/eunoians/mcrpg/registry/manager/McRPGManagerKey.javasrc/main/java/us/eunoians/mcrpg/skill/Skill.javasrc/main/java/us/eunoians/mcrpg/skill/experience/context/BlockBreakContext.javasrc/main/java/us/eunoians/mcrpg/skill/experience/context/EntityDamageContext.javasrc/main/java/us/eunoians/mcrpg/skill/experience/context/GainReason.javasrc/main/java/us/eunoians/mcrpg/skill/experience/context/McRPGGainReason.javasrc/main/java/us/eunoians/mcrpg/skill/experience/context/SkillExperienceContext.javasrc/main/java/us/eunoians/mcrpg/skill/impl/McRPGSkill.javasrc/main/java/us/eunoians/mcrpg/statistic/McRPGStatistic.javasrc/main/java/us/eunoians/mcrpg/statistic/McRPGStatisticCacheManager.javasrc/main/java/us/eunoians/mcrpg/task/player/McRPGPlayerLoadTask.javasrc/main/resources/config.ymlsrc/main/resources/localization/english/en_commands.ymlsrc/test/java/us/eunoians/mcrpg/entity/holder/SkillHolderDataTest.javasrc/test/java/us/eunoians/mcrpg/expansion/content/StatisticContentTest.javasrc/test/java/us/eunoians/mcrpg/external/papi/placeholder/statistic/StatisticPlaceholderTest.javasrc/test/java/us/eunoians/mcrpg/skill/experience/context/MockExperienceContext.javasrc/test/java/us/eunoians/mcrpg/skill/impl/McRPGSkillDefaultStatisticsTest.javasrc/test/java/us/eunoians/mcrpg/statistic/McRPGStatisticTest.java
| | Constant | Key | Type | Description | | ||
| |----------|-----|------|-------------| | ||
| | `TOTAL_SKILL_LEVELS_GAINED` | `mcrpg:total_skill_levels_gained` | LONG | Sum of all levels gained across all skills | | ||
| | `TOTAL_SKILL_EXPERIENCE` | `mcrpg:total_skill_experience` | LONG | Sum of all XP earned across all skills (including overflow) | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifiers to fenced code blocks.
Lines 58 and 138 have code blocks without language specifiers. Add appropriate identifiers (e.g., java, text) for proper syntax highlighting.
Also applies to: 138-138
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/hld/statistics/statistics-integration-hld.md` at line 58, The fenced
code blocks in the HLD are missing language specifiers (affecting syntax
highlighting); update the two code fences referenced near the
`TOTAL_SKILL_EXPERIENCE` table row and the other block around line 138 by adding
appropriate language tags (for example use ```text for plain tables/CSV or
```java for Java snippets) so both fenced blocks include a language identifier;
ensure the opening fence for each block is changed to include the chosen
language (e.g., ```text or ```java) to enable proper highlighting.
Comment on lines
+273
to
+274
| return McRPGMcRPGGainReason.BLOCK_BREAK; | ||
| } |
There was a problem hiding this comment.
Typo: McRPGMcRPGGainReason should be McRPGGainReason.
Lines 273 and 302 contain duplicate prefix typos in the code examples.
📝 Suggested fixes
// In BlockBreakContext
`@Override`
public GainReason getGainReason() {
- return McRPGMcRPGGainReason.BLOCK_BREAK;
+ return McRPGGainReason.BLOCK_BREAK;
}And on line 302:
-Simple listeners (like our statistic listeners) use `event.getGainReason()`. Advanced listeners that need the triggering Bukkit event can inspect `event.getExperienceContext()` and `instanceof` check for `BlockBreakContext`, `EntityDamageContext`, etc. Listeners checking for built-in reasons use `event.getGainReason() == McRPGMcRPGGainReason.BLOCK_BREAK`.
+Simple listeners (like our statistic listeners) use `event.getGainReason()`. Advanced listeners that need the triggering Bukkit event can inspect `event.getExperienceContext()` and `instanceof` check for `BlockBreakContext`, `EntityDamageContext`, etc. Listeners checking for built-in reasons use `event.getGainReason() == McRPGGainReason.BLOCK_BREAK`.Also applies to: 302-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/hld/statistics/statistics-integration-hld.md` around lines 273 - 274,
Search the documentation examples for the incorrect enum identifier
"McRPGMcRPGGainReason" and replace it with the correct "McRPGGainReason"; for
example change "return McRPGMcRPGGainReason.BLOCK_BREAK;" and any similar
occurrences (including the duplicate prefix at the other example) to "return
McRPGGainReason.BLOCK_BREAK;" so all examples use the correct enum name.
Comment on lines
+12
to
+18
| | Sub-Phase | Scope | Status | | ||
| |-----------|-------|--------| | ||
| | 2.1 | Constants & Registration | Complete | | ||
| | 2.2 | GainReason & Statistic Listeners | Complete | | ||
| | 2.3 | Player Lifecycle (Load/Save) | In Progress | | ||
| | 2.4 | PAPI Placeholders, Commands & Config | Not Started | | ||
|
|
There was a problem hiding this comment.
Status table inconsistent with documented content.
The status table marks Sub-Phase 2.4 (PAPI Placeholders, Commands & Config) as "Not Started," yet this file contains extensive documentation for 2.4 starting at line 253. Additionally, File 2 (phase-3-papi-commands-config-lld.md) covers the same scope and calls it "Phase 3."
Clarify the phase structure: either update the status to reflect completed documentation, consolidate the Phase 2.4/Phase 3 content into a single canonical document, or clearly delineate which aspects belong in each file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/lld/statistics/phase-2-statistics-integration-lld.md` around lines 12 -
18, The status table in
docs/lld/statistics/phase-2-statistics-integration-lld.md incorrectly marks
Sub-Phase 2.4 as "Not Started" while the file already contains the 2.4 content
(and phase-3-papi-commands-config-lld.md covers the same scope); reconcile by
choosing a canonical location and updating the table: either move/merge the PAPI
Placeholders, Commands & Config content into a single canonical file (preferably
phase-3-papi-commands-config-lld.md) and delete the duplicate section from
phase-2-statistics-integration-lld.md, or leave the content in phase-2 and
update the status cell for Sub-Phase 2.4 to "Complete" and add a cross-reference
note to phase-3-papi-commands-config-lld.md; ensure you update the table row for
Sub-Phase 2.4 and remove or add a clarifying comment in the duplicated sections
to avoid future confusion.
| - Updates per-skill max level via `setMaxInt()` (only increases) | ||
| - Increments `TOTAL_SKILL_LEVELS_GAINED` by `afterLevel - beforeLevel` | ||
|
|
||
| **Note:** This listener uses `SkillGainExpEvent` (the pre-event) for XP tracking. See [Known Issue: Overflow XP at Max Level](#known-issue-overflow-xp-at-max-level) for implications. |
There was a problem hiding this comment.
Broken internal link.
Line 128 references #known-issue-overflow-xp-at-max-level, but no such section heading exists in this file. Either add the missing section or remove the reference.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 128-128: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/lld/statistics/phase-2-statistics-integration-lld.md` at line 128, The
internal link to `#known-issue-overflow-xp-at-max-level` is broken because that
heading does not exist; either add a new section titled "Known Issue: Overflow
XP at Max Level" (or exact heading that matches the anchor) describing the
overflow XP behavior and implications for `SkillGainExpEvent` XP tracking, or
remove/replace the reference in the sentence that mentions `SkillGainExpEvent`
so it points to an existing heading; ensure the anchor text exactly matches the
heading if you add the section so the internal link resolves.
Comment on lines
+1
to
+7
| # Phase 3: PAPI Placeholders, Commands & Config — Low-Level Design | ||
|
|
||
| **Parent HLD:** [statistics-integration-hld.md](../../hld/statistics/statistics-integration-hld.md) (Phase 4) | ||
| **McCore Framework HLD:** [statistics-framework-hld.md](../../../../McCore/docs/hld/statistics/statistics-framework-hld.md) | ||
| **Phase 2 LLD:** [phase-2-statistics-integration-lld.md](phase-2-statistics-integration-lld.md) (Sub-Phase 2.4) | ||
|
|
||
| This LLD covers the player- and admin-facing surface for McRPG statistics: PAPI placeholders, offline stat caching, admin commands, configuration, and localization. |
There was a problem hiding this comment.
Inconsistent phase numbering across documentation.
This document is titled "Phase 3" (line 1), references the parent HLD as "Phase 4" (line 3), and cross-references Phase 2's "Sub-Phase 2.4" (line 5). Meanwhile, phase-2-statistics-integration-lld.md documents Sub-Phase 2.4 with overlapping content.
Reconcile the phase numbering: establish a consistent hierarchy (e.g., is PAPI/commands Phase 2.4, Phase 3, or Phase 4?) and update all cross-references accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/lld/statistics/phase-3-papi-commands-config-lld.md` around lines 1 - 7,
The document title "Phase 3: PAPI Placeholders, Commands & Config"
(phase-3-papi-commands-config-lld.md) conflicts with its parent link to
statistics-integration-hld.md labeled "Phase 4" and the Phase 2 LLD reference
phase-2-statistics-integration-lld.md (Sub-Phase 2.4); decide which canonical
phase this content belongs to (e.g., Phase 2.4, Phase 3, or Phase 4), then
update the heading in phase-3-papi-commands-config-lld.md, the Parent HLD
label/target (statistics-integration-hld.md), and the Phase 2 LLD reference
(phase-2-statistics-integration-lld.md) so all three files consistently use the
chosen phase numbering and correct cross-reference text/links; also search for
and update any remaining references/TOC entries that mention these phase numbers
to keep documentation consistent.
DiamondDagger590
pushed a commit
that referenced
this pull request
Apr 3, 2026
- Fix thread-safety issue with NumberFormat in StatisticPlaceholder - Skip PvP kills in CombatStatisticListener mob kill tracking - Make SkillExperienceContext.getGainReason() non-abstract with default - Use Locale.ROOT for locale-stable key generation in McRPGGainReason - Replace e.printStackTrace() with plugin logger across all stat commands - Fix negative amount clamp in RedeemExperienceCommand - Use IllegalArgumentException for unsupported stat types in StatisticSetCommand - Bump config-version for structural changes - Add missing Javadoc and fix existing doc errors - Add unmodifiable collection assertion in McRPGStatisticTest https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
Defines how McRPG integrates with McCore's statistics framework: McRPGStatistics constants, registration via StatisticRegistrar, listener strategy for incrementing stats via existing McRPG events, player lifecycle integration, data migration for existing players, and PAPI placeholders. https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
Changes: - Rename McRPGStatistics -> McRPGStatistic (singular, per naming convention) - Make global gameplay stats skill-agnostic (damage/kills not tied to Swords) - Change per-skill level stats to max-level-reached using setMaxInt - Add dynamic per-ability activation count statistics - Remove quest statistics (excluded from scope) - Fix markClean() ordering to only clean after transaction succeeds - Remove data migration section (no existing installations) - Remove QuestStatisticListener from listener strategy https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
…cturing - Clarify McRPG owns PAPI placeholder registration via McRPGPapiExpansion - Add statistics config section (cache settings in McRPG's config) - Split Phase 3 into Phase 3 (lifecycle) and Phase 4 (commands/PAPI/config) - Add unit tests to Phase 2 https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
- Experience stats now explicitly track overflow XP past max level - Block-based stats (blocks mined, trees chopped, crops harvested) can no longer rely on skill type filtering alone — redeemable XP breaks this - Added GainReason enum proposal as prerequisite for accurate block stats - Updated listener examples to filter by GainReason.BLOCK_BREAK - Phase 2 now includes GainReason implementation as first step https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
GainReason is now an abstract method on SkillExperienceContext, with each subtype (BlockBreakContext, EntityDamageContext) returning its own reason. New lightweight context subtypes (RedeemExperienceContext, CommandExperienceContext) cover non-gameplay XP sources. Events expose both the full SkillExperienceContext and a convenience getGainReason() wrapper so simple listeners check the enum while advanced listeners can inspect the triggering Bukkit event. https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
GainReason is now an interface with getKey() and getDisplayName(). McRPG ships McRPGGainReason enum (BLOCK_BREAK, ENTITY_DAMAGE, REDEEM, COMMAND, OTHER) as the built-in implementation. Third-party ContentExpansion plugins can define their own enums implementing GainReason for custom XP sources. https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
Create McRPGStatistic constants class with all statistic definitions: - Global gameplay stats (blocks mined, mobs killed, damage dealt/taken) - Per-skill XP and max level stats - Global and per-ability activation count stats - Helper methods for dynamic key generation Create McRPGStatisticRegistrar to register all static statistics and dynamically generate per-ability activation statistics from the AbilityRegistry during bootstrap. https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
GainReason system: - Create GainReason interface for extensible XP source identification - Create McRPGGainReason enum (BLOCK_BREAK, ENTITY_DAMAGE, REDEEM, COMMAND, OTHER) - Add abstract getGainReason() to SkillExperienceContext with implementations in BlockBreakContext and EntityDamageContext Event threading: - Thread GainReason through SkillHolder.addExperience() to events - Add GainReason and experience amount to SkillGainExpEvent and PostSkillGainExpEvent (backward-compatible constructors preserved) - Update SkillListener, RedeemExperienceCommand, GiveExperienceCommand to pass appropriate GainReasons Statistic listeners: - SkillStatisticListener: tracks per-skill and total XP, max levels, block-based stats filtered by BLOCK_BREAK gain reason - AbilityStatisticListener: tracks global and per-ability activations - CombatStatisticListener: tracks damage dealt/taken and mob kills - Register all three in McRPGListenerRegistrar https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
- Load player statistics from database in McRPGPlayerLoadTask - Save dirty statistics with conditional markClean() in McRPGPlayer.savePlayer() - Add Phase 2 LLD documenting all sub-phases, known issues, and implementation order https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
…perience design - Clarify onEntityDeath checks entity.getKiller() which returns Player - Replace overflow XP options A/B/C with uncapped experience approach: totalExperience grows unbounded, getCurrentLevel() clamps to maxLevel - Add unit test requirements for uncapped experience behavior https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
- Remove XP cap and early return in SkillHolder.addExperience() so totalExperience grows unbounded and events always fire for statistics - Add max-level modifier bypass in SkillListener.levelSkill() to skip boosted/rested XP consumption when skill is at max level - Add display suppression in OnSkillLevelUpListener to hide boss bar updates at max level - Cap redeem amount in RedeemExperienceCommand to prevent overflow past max level (replaces leftover-based refund logic) - Return 0 from getCurrentExperience() at max level - Fix McRPGPlayer.savePlayer() compilation error (executeTransaction returns void, not boolean) - Add SkillHolderDataTest with tests for uncapped XP, level clamping, retroactive level-up, and normal behavior - Update Phase 2 LLD with max-level silent accumulation design, PAPI async populate pattern, and async command feedback pattern https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
- Refactor McRPGStatisticRegistrar to auto-register per-ability activation statistics via AbilityRegisterEvent listener, so third-party content expansions get statistics for free without manual registration - Reorder bootstrap: McRPGStatisticRegistrar now runs before McRPGExpansionRegistrar so the event listener is active during ability registration - Fix fully qualified import in GiveExperienceCommand - Add comment explaining max-level return behavior in SkillHolder - Add class-level javadoc to SkillGainExpEvent and PostSkillGainExpEvent - Clean up long registry chain in SkillListener by extracting locals - Guard per-skill statistics in SkillStatisticListener against unregistered third-party skills; add class javadoc explaining extensibility expectations - Add javadoc to AbilityStatisticListener explaining content expansion auto-registration - Add javadoc to McRPGStatistic and McRPGGainReason explaining why displayName/description fields are not localized - Add javadoc to McRPGStatistic.key() helper explaining the @SuppressWarnings("deprecation") annotation https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
Replace the hidden McRPGStatisticRegistrar (which silently auto-registered per-ability stats via AbilityRegisterEvent) with an explicit StatisticContentPack in the content expansion system. New types: - StatisticContent: McRPGContent wrapper for McCore's Statistic (composition since SimpleStatistic is a record) - StatisticContentPack: content pack for statistic definitions - STATISTIC handler in ContentHandlerType Ability interface changes: - Ability.getDefaultStatistics() returns empty Set by default - ActiveAbility overrides to provide an activation count statistic - These are convenience helpers — the expansion must explicitly include them in its StatisticContentPack to register them McRPGExpansion now builds a StatisticContentPack containing all static stats + each ability's getDefaultStatistics(). Third-party expansions follow the same pattern for their own custom statistics (e.g., "hit three+ players in one shot with Rage Spike"). Also adds registry guards in AbilityStatisticListener for per-ability stats that may not be registered by a third-party expansion. https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
Skills now provide default statistics (experience + max level) via Skill.getDefaultStatistics(), mirroring the ability pattern. Per-skill static constants removed from McRPGStatistic in favor of dynamic generation. McRPGExpansion collects stats from both skills and abilities. Added javadoc to intStat/longStat helpers and factory methods for skill statistics. New tests cover McRPGStatistic, StatisticContent, and McRPGSkill default statistics. https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
Implements HLD Phase 4 / LLD Sub-Phase 2.4: PAPI placeholders for all statistics, offline stat caching via McRPGStatisticCacheManager, admin commands (/mcrpg statistic view/list/set/reset), config routes for cache settings, localization keys, and StatisticParser for tab completion. https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
- Fix thread-safety issue with NumberFormat in StatisticPlaceholder - Skip PvP kills in CombatStatisticListener mob kill tracking - Make SkillExperienceContext.getGainReason() non-abstract with default - Use Locale.ROOT for locale-stable key generation in McRPGGainReason - Replace e.printStackTrace() with plugin logger across all stat commands - Fix negative amount clamp in RedeemExperienceCommand - Use IllegalArgumentException for unsupported stat types in StatisticSetCommand - Bump config-version for structural changes - Add missing Javadoc and fix existing doc errors - Add unmodifiable collection assertion in McRPGStatisticTest https://claude.ai/code/session_01PYTi4S3uoiptKwqFJ3NJvs
DiamondDagger590
force-pushed
the
claude/add-statistics-feature-3GzsC
branch
from
1b1eca8 to
c208ed0
Compare
GUI/UX ReviewThe diff is entirely documentation files ( No GUI/UX concerns found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements Phase 2 and Phase 3 of the McRPG statistics integration, adding comprehensive gameplay metrics tracking, offline player stat caching, PAPI placeholders, and admin commands for managing statistics.
Key Changes
Statistics Framework Integration (Phase 2)
SkillStatisticListener: Tracks per-skill XP, level gains, and block/crop/tree harvestingAbilityStatisticListener: Tracks ability activationsCombatStatisticListener: Tracks damage dealt/taken and mob killsGainReasonthreading throughSkillGainExpEventandPostSkillGainExpEventwith backward-compatible constructorsaddExperience(int, GainReason)overload passes reason through event chainPlayer-Facing Features (Phase 3)
StatisticCachefor offline player stat lookups with database fallback%mcrpg_statistic_mcrpg:blocks_mined:0%)/mcrpg statistic:view <player> <statistic>: Display a specific stat for a playerlist <player>: List all statistics for a playerset <player> <statistic> <value>: Set a statistic value with confirmationreset <player> [statistic]: Reset one or all statistics with confirmationConfiguration & Localization
STATISTICS_CACHE_ENABLED,STATISTICS_CACHE_MAX_SIZE,STATISTICS_CACHE_TTLroutesstatistics.cachesection with sensible defaults (enabled, 1000 max entries, 300s TTL)Bootstrap & Registration
McRPGStatisticCacheManagerin PROD blockStatisticContentPackfor dynamic ability activation stat registrationTesting
addExperience()withGainReasonparameterNotable Implementation Details
populateAsync()method allows PAPI placeholders to warm cache without blockingMcRPGGainReason.OTHERfor existing codeSummary by CodeRabbit
Release Notes
New Features
/mcrpg statisticcommand suite withview,list,set, andresetsubcommands for querying and managing player statistics.Configuration
configuration.statistics.cachesection withenabled,max-size, andttloptions for cache behavior.