vibe coded - #1677
Open
LotusRPG wants to merge 2 commits into
Open
Conversation
This was referenced Jul 21, 2026
Travja
reviewed
Jul 21, 2026
Travja
left a comment
Travja
left a comment
Contributor
There was a problem hiding this comment.
This PR bundles 8 unrelated concerns across 27 files (item-stack/icon refactor, Divinity plugin integration, bow/crossbow attack indicator, flag-system reentrancy, dynamic flag placeholders, several unrelated mechanic fixes, a config-save refactor, and a new event), which makes it hard to review and test as a single unit. I've split it into 8 focused PRs against dev, each independently reviewable/testable:
- #1774 — refactor: replace lazy ItemStack suppliers with direct fields, preserve icon metadata
- #1775 — feat: integrate with Divinity plugin stats, healing, CC, and durability
- #1776 — feat: draw-force based bow/crossbow attack indicator condition
- #1777 — fix: prevent infinite recursion in FlagManager.clearFlags
- #1778 — feat: resolve dynamic placeholders in flag names
- #1779 — fix: small independent mechanic/condition/registry fixes
- #1780 — feat: add PlayerMaxManaChangeEvent
- #1781 — refactor: save dynamic skills/classes immediately instead of a deferred queue
A couple of notes on what didn't carry over 1:1:
- The
Data.javaicon-parsing simplification (removing Codex item-type lookups) was left out of #1774 —devhas since added custom item ID preservation for unresolved icons (1bca114), and this PR's change would have silently regressed that feature. That needs a deliberate call, not a blind carry-over. - The
pom.xmlversion downgrades (project version andpacketevents-spigot) were dropped —devhas moved past even the pre-downgrade versions, so these looked like the contributor's fork being stale rather than intentional.
Thanks for the contribution — closing this out in favor of the split PRs above, but happy to loop back here if any of the reasoning above needs correcting.
Generated by Claude Code
Travja
reviewed
Jul 21, 2026
Travja
left a comment
Travja
left a comment
Contributor
There was a problem hiding this comment.
Correction to my comment above: this PR is not being closed right now — leaving it open for now. It'll stay open until there's an explicit decision to close it in favor of the split PRs.
Generated by Claude Code
Travja
added a commit
that referenced
this pull request
Jul 23, 2026
* fix: prevent infinite recursion in FlagManager.clearFlags Split out of #1677 (piece 4 of 8). If a FlagExpireTrigger-driven skill adds a new flag to an entity while FlagManager.clearFlags() is clearing that same entity's flags, the new flag re-creates a FlagData entry, and removing it during the same clear pass re-enters clearFlags() for that entity, recursing indefinitely (observed on player death, where clearing flags can trigger skills that re-flag the dying entity). Guard clearFlags() with a per-entity "currently clearing" set so a reentrant call for the same entity is a no-op instead of recursing. * test: cover FlagManager.clearFlags reentrancy guard Reproduces the infinite-recursion scenario the guard fixes: a FlagExpireTrigger-driven skill reacting to FlagExpireEvent by adding a new flag to the same entity while clearFlags() is still unwinding for that entity. Without the clearingEntities guard, this recurses through clear() -> removeFlag() -> (synchronous event) -> addFlag() -> clearFlags() until the stack overflows; with it, the reentrant call is a no-op and the newly-added flag survives. --------- Co-authored-by: Claude <noreply@anthropic.com>
Travja
added a commit
that referenced
this pull request
Jul 23, 2026
* feat: resolve dynamic placeholders in flag names Split out of #1677 (piece 5 of 8). Flag names/keys are matched literally today, so a skill can't use a flag name built from cast data (e.g. per-target or per-cast flags like "stun-{targetUUID}"). This resolves {player}, {target}, {targetUUID}, and any custom cast-data keys in flag names consistently across the producing and consuming sides: - FlagMechanic, FlagToggleMechanic, FlagClearMechanic: apply EffectComponent#filter() to the configured flag key/name before use. - FlagTrigger, FlagExpireTrigger: resolve the same placeholders when matching a fired/expired flag against the configured "flags" list, mirroring EffectComponent#filter with the trigger's caster entity acting as both caster and target. - FlagCondition: apply filter() to the configured flag name. - FlagClearMechanic: on regex mode, a pattern that fails to compile for one target's resolved key now skips just that target (and the mechanic reports failure) instead of aborting the whole mechanic. Also cleans up minor issues in FlagTrigger/FlagExpireTrigger noticed along the way (min-duration tick math, a stray semicolon). * test: cover dynamic placeholder resolution in flag names Adds/extends tests for the flag-placeholder feature: - FlagMechanicTest, FlagToggleMechanicTest (new): per-target and cast-data placeholders resolve before the flag is applied/toggled. - FlagClearMechanicTest: adds placeholder resolution cases, including the regex-mode change where one target's unresolved/invalid pattern now only skips that target instead of aborting the whole mechanic. - FlagConditionTest (new): static and dynamic ({target}) flag names, plus the "not set" type. - FlagTriggerTest, FlagExpireTriggerTest (new): matching a fired/expired flag against a configured flags list containing {player}/cast-data placeholders, including the existing min-duration/inverted behavior. --------- Co-authored-by: Claude <noreply@anthropic.com>
Travja
added a commit
that referenced
this pull request
Jul 23, 2026
* feat: add PlayerMaxManaChangeEvent Split out of #1677 (piece 7 of 8). Fires a new PlayerMaxManaChangeEvent after Fabled computes a player's max mana (including class bonuses and attribute scaling), letting other plugins apply additional flat bonuses via setMaxMana() before it's applied. The result is clamped to a minimum of 0. * test: cover PlayerMaxManaChangeEvent firing and mana clamping Covers that updatePlayerStat() fires PlayerMaxManaChangeEvent, that a listener's setMaxMana() bonus is reflected in the player's final max mana, and that a listener setting a negative value is clamped to 0. --------- Co-authored-by: Claude <noreply@anthropic.com>
Travja
added a commit
that referenced
this pull request
Jul 23, 2026
* fix: small independent mechanic/condition/registry fixes Split out of #1677 (piece 6 of 8) — four small, unrelated fixes bundled together since each is a one-off, self-contained change: - RepeatMechanic: new single-instance setting cancels a caster's previous repeat tasks before starting a new one, instead of letting them stack. - TargetComponent: allow self-targeting when IncludeCaster is anything other than FALSE (previously self-targeting was always excluded regardless of the IncludeCaster setting). - Skill#damage(): wrap damage application in try-finally so the static skillDamage flag is always reset, even if dealing damage throws. - ComponentRegistry: register WorldTarget, which existed but was never registered as a usable target component. * test: cover misc mechanic/condition/registry fixes - RepeatMechanic: widen the tasks field to package-visible (matching DelayMechanic's existing convention) so tests can assert on it directly, then cover single-instance cancelling the caster's previous repeat task before starting a new one, vs. stacking without it. - TargetComponent: cover the self-targeting clause across all three IncludeCaster values, isolated from the separate ally/enemy grouping check by using group=both. - Skill#damage(): cover that the static skillDamage flag resets even when applying the damage throws (mocks DamageRegistry.dealDamage to throw), not just on the happy path. - ComponentRegistry: cover that WorldTarget is now registered as a selectable target component. * fix: implement missing getKey() override in TargetComponentSelfTargetTest stub CI caught this - StubTargetComponent didn't implement EffectComponent's abstract getKey(), failing test compilation. --------- Co-authored-by: Claude <noreply@anthropic.com>
Travja
added a commit
that referenced
this pull request
Aug 2, 2026
* feat: draw-force based bow/crossbow attack indicator condition Split out of #1677 (piece 3 of 8). AttackIndicatorCondition previously only checked Player#getAttackCooldown(), which doesn't reflect bow/crossbow draw state. This adds a weapon setting ("auto", "bow", "crossbow") and computes a 0.0-1.0 draw-force value: - Bow: fraction of full draw based on item-in-use ticks. - Crossbow: 1.0 once charged (accounting for Quick Charge), otherwise partial draw progress. - A new ShootListener (registered in Fabled#onEnable) snapshots the force from EntityShootBowEvent as short-lived caster metadata, since the Launch trigger fires after the bow/crossbow state has already reset. Paper-only APIs (getActiveItem, CrossbowMeta#isCharged) are accessed via reflection with a Spigot-compatible fallback, since the project compiles against the Spigot API but runs on Paper. ## Testing Could not build locally (private Maven repo unreachable in this sandbox). Needs manual testing on a Paper server: bow/crossbow draw indicator values while charging, at full draw, and immediately after firing. * test: cover weapon-mode range check in AttackIndicatorCondition Covers the crossbow weapon mode (charged -> full value, within a high min/max range) and that a non-player caster always fails the condition. Deliberately not covered: the ShootListener metadata-capture path (would need to construct a real EntityShootBowEvent, which nothing else in this codebase does, so its constructor isn't modeled anywhere to copy) and the Paper-only reflection fallbacks for getActiveItem/CrossbowMeta#isCharged - those need manual/in-game verification since MockBukkit's support for them isn't established here. * style: replace inline fully-qualified class names with imports java.lang.reflect.Method and AttackIndicatorCondition were referenced by fully-qualified name inline instead of via a normal import. --------- Co-authored-by: Claude <noreply@anthropic.com>
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.
Coped from copilot:
1. ItemStack Refactoring (FabledClass, Skill, FabledAttribute, Data.java)
Supplier<ItemStack>lazy loading with directItemStackfieldsData.parseIcon()/serializeIcon()by removing Codex item type lookups2. Divinity Plugin Integration (DivinityHook.java, StatMechanic, HealMechanic, StatusMechanic, DurabilityMechanic)
DivinityHookmethods:clampStatAmount(),applyStatToMob(),removeStatFromMob()StatMechanicnow respects Divinity's stat caps via clampingHealMechanicapplies Divinity's healing-cast and healing-received multipliersStatusMechanicapplies CC duration multiplier and CC resistance reductionDurabilityMechanicintegrates with Divinity's durability systemPluginChecker.isDivinityActive()tracking3. Bow/Crossbow Attack Indicator Condition (AttackIndicatorCondition.java)
ShootListenercaptures bow/crossbow draw force viaEntityShootBowEventmetadata4. Flag System & Reentrance Protection (FlagManager.java, FlagTrigger.java, FlagExpireTrigger.java)
FlagManagerguards against reentrantclearFlags()calls withclearingEntitiessetFlagTriggerandFlagExpireTriggernow resolve dynamic placeholders ({player},{target},{targetUUID}, custom cast data)FlagExpireTriggerskills add flags during clearing5. Other Mechanics & Conditions Fixes
FlagCondition: Applyfilter()to flag names for dynamic variable resolutionFlagMechanic,FlagToggleMechanic,FlagClearMechanic: Applyfilter()to flag keysDamageMechanic: Added damage flags metadata (crit ignore, dodge ignore, block ignore, bleed/vamp disable, skill crit ignore)RepeatMechanic: Newsingle-instancesetting to cancel previous repeats before starting new onesTargetComponent: Allow self-targeting whenIncludeCasteris notFALSESkill.damage(): Wrapped damage application in try-finally for proper skill damage flag cleanup6. Config Save Refactoring (RegistrationManager.java)
7. New Event (PlayerMaxManaChangeEvent.java)
8. UI & Display Improvements
FabledAttribute.getToolIcon(): Now preserves icon metadata (lore, display name)Skill.getIndicator(): Fixed to preserve item metadata including damage and custom model data