Implement Skill Book System and Riptide Guardian MythicMobs Integration#225
Draft
DiamondDagger590 wants to merge 30 commits into
Draft
Implement Skill Book System and Riptide Guardian MythicMobs Integration#225DiamondDagger590 wants to merge 30 commits into
DiamondDagger590 wants to merge 30 commits into
Conversation
Covers SkillBookFactory, SkillBookConsumeListener, SkillBookConsumeEvent, SkillBookRewardType, localization keys, and bootstrap registration. Documents McRPGSkillBookDrop refactor to delegate to the centralized factory and the migration from skill name strings to ability NamespacedKeys. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Document bake-at-creation tradeoff for skill book display text and reference the backlog issue for adopting Adventure GlobalTranslator project-wide. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
SkillBookFactory now resolves display name and lore through McRPGLocalizationManager instead of hardcoding strings. Adds player-aware overload that uses the player's locale chain, and server-default overload for contexts without a player (e.g., MythicMobs drops). New localization keys: ability.skill-book.item-name and ability.skill-book.item-lore. SkillBookRewardType.grant() and describeForDisplay() also use localization. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
- SkillBookConsumeEvent extends McRPGPlayerEvent (not PlayerEvent), passes McRPGPlayer instead of raw Bukkit Player - SkillBookConsumeEvent.getItemStack() returns a defensive clone - SkillBookConsumeListener uses EventPriority.LOWEST to claim the interaction before other plugins - Quest reward config uses keyed map format (phase_shift_book:) instead of YAML list, matching existing reward patterns - SkillBookRewardType implements withLocalizationRoute() following CommandRewardType's localization chain pattern - describeForDisplay(McRPGPlayer) tries auto-derived route first, falls back to generic SKILL_BOOK_ITEM_NAME key https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Consistent with PDC key name (skill_book_ability) and quest reward config (ability: "mcrpg:phase_shift"). Also updates McRPGSkillBookDrop config.getString() call and all documentation references. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
New files: - SkillBookFactory: centralized factory for creating skill book ItemStacks with PDC tags, localized display text via McRPGLocalizationManager - SkillBookConsumeEvent: cancellable McRPGPlayerEvent with defensive item clone, fired before ability unlock - SkillBookConsumeListener: LOWEST priority RIGHT_CLICK handler that validates ability, checks unlock state, fires events, removes item - SkillBookRewardType: QuestRewardType with keyed config format, withLocalizationRoute support, player-aware grant and display Modified files: - McRPGSkillBookDrop: delegates to SkillBookFactory, reads ability= param - LocalizationKey: adds SKILL_BOOK_* route constants - en_abilities.yml: adds ability.skill-book.* localization entries - McRPGListenerRegistrar: registers SkillBookConsumeListener unconditionally - BuiltinRewardTypes: registers SKILL_BOOK reward type https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
- SkillBookRewardType: add missing getExpansionKey() override - SkillBookConsumeListener: use asSkillHolder() for AbilityData access and updateAttribute() instead of nonexistent setContent() - McRPGSkillBookDrop: use ItemComponentBukkitItemStack (BukkitItemStack became abstract in MythicMobs 5.7.2) - MythicMobsHook: use plugin() instead of getPlugin() https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Replace CompletableFuture.runAsync() (shared ForkJoinPool) with a dedicated fixed thread pool for the 5-thread high-concurrency test. The shared pool caused CyclicBarrier timeouts under load because tasks weren't guaranteed to be scheduled within the 5-second window. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Defines the bundled RiptideGuardian.yml mob config including: - Mob stats (80 HP drowned, armor 8, ThreatTable enabled) - Four MM combat skills (Phase Shift, Whirlpool, Waterlogged Strike, Tsunami Wall) with full skill YAML - Drop table with two skill book drops: Whirlpool (12%) and Phase Shift (5%) - MM-native despawn via ~onTimer (5min) and ~onCombat (30s) - MythicMobsConfigExtractor class for first-run auto-extraction to MythicMobs Mobs/ directory - Server owner customization guide Also updates LLD-3 Section 14 with LLD-4 cross-reference. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Register a custom MythicMobs condition (mcrpg_ability_unlocked) that checks if the killing player has already unlocked an ability. Split skill book drops into paired DropTables with TriggerConditions: - Not unlocked: Whirlpool 12%, Phase Shift 5% (full rate) - Already unlocked: Whirlpool 2%, Phase Shift 1% (reduced rate) Includes McRPGAbilityUnlockedCondition class design, condition registration via MythicConditionLoadEvent, and updated full YAML. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
…k pattern - Switch from single-file extraction to MythicMobs pack structure (Packs/McRPG/ with Mobs/, Skills/, DropTables/ subdirectories) - Add McRPGAbilityMechanic design: custom MM mechanic that bridges mob ability execution to McRPG's ability system - Phase Shift and Whirlpool now use mcrpg_ability with pure-MM fallback skills for pre-LLD-6 compatibility - Split Section 7 YAML into three separate pack files - Update extraction logic for pack directory structure - Update file manifest with new Java classes and resource files - Update future notes with LLD-6 integration requirements https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Phase Shift and Whirlpool are now purely mcrpg_ability driven with no pure-MM fallback skills. If McRPG abilities aren't registered, those skills are no-ops and the mob fights with Waterlogged Strike + Tsunami Wall + melee only. The mob is an McRPG feature and is designed to require McRPG. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
All four combat abilities (Phase Shift, Whirlpool, Waterlogged Strike, Tsunami Wall) now use the mcrpg_ability mechanic. This keeps the entire combat system internal to McRPG for consistent balance, event tracking, and future extensibility. The mob requires McRPG for its full combat kit — without it, the mob is melee-only. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Flush player data to the database (async) right after unlocking the ability, before removing the item from inventory. This ensures the unlock survives a crash — worst case the world rolls back and the player still has the book but the ability is already persisted. Follows the same pattern as OnSkillLevelUpListener's immediate save via database.getDatabaseExecutorService().submit(). https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
New files: - McRPGAbilityMechanic: custom MM mechanic (mcrpg_ability) that delegates mob ability execution to McRPG's ability system - McRPGAbilityUnlockedCondition: custom MM condition (mcrpg_ability_unlocked) for unlock-aware drop rates - MythicMobsConfigExtractor: extracts bundled pack files to plugins/MythicMobs/Packs/McRPG/ on first startup - Pack YAML files: Mobs/RiptideGuardian.yml, Skills/RiptideGuardianSkills.yml, DropTables/RiptideGuardianDrops.yml Modified files: - Ability: add executeMobAbility() and supportsMobExecution() default methods for the MM-to-McRPG bridge - MythicMobsListener: register mcrpg_ability mechanic and mcrpg_ability_unlocked condition via MM load events - McRPGListenerRegistrar: call pack extraction when MM is present https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
…lity Remove executeMobAbility/supportsMobExecution from Ability interface. The mcrpg_ability mechanic now fires a MobAbilityTriggerEvent (extends AbilityActivateEvent) through the standard activateAbility() path. Abilities handle mob execution by registering an EventActivatableComponent for MobAbilityTriggerEvent — no special mob-specific API needed. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
- Create OnMobAbilityTriggerListener that handles MobAbilityTriggerEvent fired by the mcrpg_ability mechanic, calling activateAbility() directly on the event's ability (bypassing component checks since MM owns AI) - Register the listener conditionally in McRPGListenerRegistrar when MythicMobs is present - Add Javadoc to McRPGAbilityMechanic clarifying the event-driven pattern - Update LLD-4 document to reflect the event+listener architecture - Add unit tests: MobAbilityTriggerEventTest (5 tests), OnMobAbilityTriggerListenerTest (1 test), MythicMobsConfigExtractorTest (4 tests) https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
…hanic
- Track AbilityHolder in EntityManager from MythicMobSpawnEvent, remove
on MythicMobDeathEvent and MythicMobDespawnEvent for clean lifecycle
- McRPGAbilityMechanic now looks up the tracked holder (or creates one
as fallback) and lazily registers each ability with AbilityData on
first fire — abilities are inferred from MM config, no parsing needed
- Add optional tier parameter: mcrpg_ability{ability=...;tier=2}
defaults to tier 1 via AbilityTierAttribute
- Update LLD-4 document with holder lifecycle design, tier syntax, and
updated file manifest
https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Replace lazy ability registration with eager parsing at spawn time. MythicMobAbilityParser traverses the mob's skill tree across all triggers, resolving MetaSkillMechanic references and CustomMechanic wrappers to find all mcrpg_ability definitions with their tiers. Results are cached per mob type and invalidated on MythicMobs reload. The mechanic retains lazy registration as a fallback for dynamically added skills. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Addresses review feedback on the MythicMobs ability parsing layer: - Convert MythicMobAbilityParser from a static utility class to an instance-based collaborator, matching McRPG's preference for object collaborators over static domain helpers. Cache is now a per-instance field; the parser is held as a final field on MythicMobsListener. - Drop the static Logger field in favor of McRPG.getInstance().getLogger() called on demand at log sites. - Change McRPGAbilityMechanic.getAbilityKey() to return Optional <NamespacedKey> instead of a nullable return, and remove the fully-qualified @nullable annotation. - Move the McRPGAbilityMechanic constructor above its getters to follow conventional member ordering. - Rename ensureAbilityRegistered to addAbilityToHolderIfAbsent to avoid colliding with the McRPG "ability registration" domain term, which refers specifically to AbilityRegistry registration. - Update MythicMobAbilityParserTest to instantiate the parser, use instance methods, and stub getAbilityKey() with Optional returns.
Addresses follow-up review feedback on the MythicMobs ability layer:
- Make McRPGAbilityMechanic.getAbilityKey() non-null. The constructor
now throws IllegalArgumentException when 'ability' is missing or
invalid, so malformed configs fail loudly at MythicMobs mechanic-load
time instead of at cast time. Also clamp 'tier' to a minimum of 1.
- Fix unchecked cast in McRPGAbilityUnlockedCondition to use an
instanceof pattern guard instead of a raw cast on getBukkitEntity().
- Switch MythicMobAbilityParser's cache from ConcurrentHashMap to a
Caffeine cache with expireAfterAccess(1h), matching the pattern used
in QuestManager so entries drop out on long-running servers that
rarely spawn obscure mob types.
- Clamp tier to >= 1 inside ParsedAbilityInfo's compact constructor so
non-positive tiers never propagate to ability logic.
- Delegate mcrpg_ability{...} param parsing in the parser to
MythicLineConfigImpl.of(...) plus McRPGAbilityMechanic's own
constructor. The mechanic is now the single source of truth for
supported fields — adding a new field only requires editing the
mechanic, and param order is no longer brittle.
- Update MythicMobAbilityParserTest: stubs no longer wrap getAbilityKey()
in Optional, the obsolete null-key test is removed, and a new test
verifies ParsedAbilityInfo tier clamping for 0 / negative / valid values.
The parser's cache TTL was previously hardcoded to 1 hour. Expose it as configuration.mythicmobs.ability-parser.cache-ttl-minutes in the main config and wire it through the ReloadableContentManager so operators can tune it without restarts. Because Caffeine's expireAfterAccess is fixed at build time, an inner ReloadableInteger subclass overrides reloadContent() to rebuild the Caffeine cache on reload.
MythicMobsListener now pulls MAIN_CONFIG and tracks its parser's reloadable itself instead of taking a YamlDocument constructor parameter, so callers can't construct it with a foreign config. Drop the null branch from rebuildCache: ReloadableInteger always has a boxed Integer from Section::getInt, so @NotNull is accurate and the null check was dead code.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
…laborator Replaces the static utility class pattern with a constructor-injected instance that takes McRPG, preventing callers from passing arbitrary plugin instances and aligning with the project anti-pattern rule against static utility classes for domain logic. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
McRPGAbilityMechanic no longer assumes abilities are tiered. Instead, MythicMobsHook owns a registry of MechanicAttributeExtractors that third-party plugins can extend to support custom config parameters (e.g., charge_time, element). The built-in tier extractor is registered by default. ParsedAbilityInfo now carries a generic List<AbilityAttribute<?>> instead of a bare int tier, and the listener/parser flow passes these through without knowledge of specific attribute types. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Removes the MechanicAttributeExtractor system in favor of the existing AbilityAttribute infrastructure. The mechanic now iterates all registered attributes from AbilityAttributeRegistry, checks if the MythicMobs config contains a value for each attribute's database key name, and uses create(String) to construct typed instances. Third-party plugins that register custom attributes in the registry automatically get MythicMobs config support with no additional wiring. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
Converts skill book creation from a static factory to a BaseItemBuilder subclass, enabling config-driven appearance with global defaults and per-ability overrides. Server owners can now customize material, name, lore, glowing, enchantments, and custom model data for skill books. https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF
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 the complete Skill Book System (LLD-3) and MythicMobs example configuration (LLD-4) for the Riptide Guardian fishing mob. It introduces a unified item creation and consumption flow for skill books, integrates McRPG abilities with MythicMobs mechanics, and bundles the first example mob configuration.
Key Changes
Skill Book System (LLD-3)
mcrpg:skill_bookandmcrpg:skill_book_ability). Supports both player-aware (localized) and server-default item creation paths.AbilityUnlockEvent, and removes the item. Includes immediate async player data persistence to minimize crash-loss window.ItemRewardType.SkillBookFactoryinstead of building items inline, ensuring consistency across all skill book sources.MythicMobs Integration (LLD-4)
mcrpg_ability) that delegates ability execution to McRPG's ability system. MythicMobs owns AI decisions (when to fire, cooldowns, targeting); McRPG owns execution (damage, effects, events).mcrpg_abilitymechanics from a MythicMob's skill tree at spawn time with two-pass resolution (direct mechanics + nested skill references). Results are cached per mob type with configurable TTL.mcrpg_ability_unlocked) that checks if a player has unlocked a specific ability, enabling unlock-aware drop rate reduction.plugins/MythicMobs/Packs/McRPG/on first startup, preserving server-owner customizations.Bundled MythicMobs Configuration
mcrpg_abilitymechanic.Supporting Changes
ability.skill-book.item-name,ability.skill-book.item-lore, and consumption-related messages toen_abilities.yml.mythicmobs.ability-parser.cache-ttl-minutestoconfig.ymlfor parser cache TTL management.https://claude.ai/code/session_01CjRSEKEZz3bA3pV1qynfTF