CustomNPC-Plus/Ability_Update_Review.md

Ability System Review - CustomNPC+

A comprehensive technical review of the Ability System identifying bugs, logic issues, incomplete features, and inconsistencies.

---

Critical Issues

1. Cooldown Uses Mixed Time Systems

Status: FIXED Files:

• Ability.java - Removed cooldown tracking (cooldownEndTime, startCooldown(), isOnCooldown(), canUse())
• DataAbilities.java - Now sole owner of cooldown management using world time

Solution: Removed all cooldown tracking from Ability.java. Cooldowns are now managed exclusively by DataAbilities using world.getTotalWorldTime() for consistency.

---

2. AbilityHeal - Heal Over Time Completely Broken

Status: FIXED File: AbilityHeal.java

Solution: Refactored to separate ally-finding from healing. The findAlliesInRadius() method is now called regardless of instantHeal flag, populating the list for HoT to use. Added heal particles during HoT ticks.

---

3. AbilityGuard - Counter Integration Missing

Status: FIXED Files:

• DataAbilities.java:onDamage() - Added integration to call AbilityGuard.onDamageTaken()
• DataAbilities.java:getGuardDamageReduction() - Added helper method for damage reduction

Solution: DataAbilities.onDamage() now checks if current ability is an AbilityGuard and calls onDamageTaken() when the NPC is hit while guarding.

---

High Priority Issues

4. knockbackUp Stored But Never Applied

Status: FIXED File: Ability.java

Solution: Updated applyNpcKnockback() to accept a knockbackUp parameter and use it instead of hardcoded 0.1D. Both applyAbilityDamage() and applyAbilityDamageWithDirection() now pass the knockbackUp value from the HitEvent.

---

5. Targeting Modes Not Implemented

Status: FIXED (Removed) File: TargetingMode.java

Solution: Removed the unused targeting modes (RANDOM_ENEMY, LOWEST_HP, HIGHEST_THREAT) from the enum. Only the implemented modes remain: AGGRO_TARGET, SELF, AOE_SELF, AOE_TARGET.

---

6. Damage Source Inconsistency

Status: FIXED File: Ability.java

Solution: Standardized both applyAbilityDamage() and applyAbilityDamageWithDirection() to use NpcDamageSource("mob", npc) for consistent damage source handling.

---

Incomplete Features

7. AbilityProjectile - No Actual Projectile

Status: CONFIRMED - INCOMPLETE File: AbilityProjectile.java:88

// TODO: Use custom EntityAbilityProjectile for actual tracking
applyAbilityDamageWithDirection(npc, target, damage, knockback, dx, dz);

Current behavior:

• speed, homing, homingStrength fields exist but are unused
• No projectile entity is spawned
• Damage is instant, just with particle cosmetics

---

8. AbilityOrb - No Visual Orb Rendering

Status: CONFIRMED - INCOMPLETE File: AbilityOrb.java:117-179

Problem: The orb tracks position (orbX, orbY, orbZ) with runtime getters but nothing renders it client-side. Players only see particle effects on hit, not the flying orb itself.

---

9. AbilityBeam - No Visual Beam Rendering

Status: CONFIRMED - INCOMPLETE File: AbilityBeam.java:213-220

Problem: Only spawns flame particles along the path. No actual beam entity or proper beam visual. The lockOnTarget telegraph during active phase isn't visually communicated.

---

Medium Priority Issues

10. Hazard/Trap Telegraph Offset Mismatch

Status: CONFIRMED Files:

• AbilityHazard.java:119-147, 201-248
• AbilityTrap.java:109-138, 166-205

Problem: Telegraph is created at target position, but actual effect applies random offset:

// createTelegraph() - No offset applied, shows at target position

// onExecute() - Random offset applied AFTER telegraph locks
double[] pos = calculateOffsetPosition(telegraph.getX(), telegraph.getY(), telegraph.getZ());
zoneX = pos[0];  // Actual effect position differs from telegraph

Impact: Players see telegraph at one location, damage happens elsewhere. Misleading visual feedback.

Fix: Either apply offset during telegraph creation, or update telegraph position in onExecute().

---

11. AbilityCharge Telegraph Direction Timing

Status: CONFIRMED - MINOR File: AbilityCharge.java:91, 254-286

Problem: Telegraph created at tick 0, direction locked at tick 1:

// Telegraph created during setup (tick 0) with current target direction
// Line 91: Direction locked at tick 1
if (tick == 1) {
    lockChargeDirection(npc, target);
}

Impact: If target moves between tick 0-1, telegraph direction won't match actual charge.

---

12. AbilitySlam - activeTicks May End Before Landing

Status: CONFIRMED - MINOR File: AbilitySlam.java:52-53, 230-250

Problem: activeTicks = 60 is fixed, but if NPC takes longer to land, ability transitions to RECOVERY without onLanding() being called.

Secondary issue: If NPC lands early (tick 15), active phase continues until tick 60 doing nothing - wasted ticks.

---

13. FOLLOW Mode Telegraph Not Updated

Status: CONFIRMED Files: AbilityHazard.java:139-146, AbilityVortex.java

Problem: For FOLLOW_TARGET mode, telegraph locks position on execute, but the actual hazard zone follows the target. Visual mismatch between warning and danger.

---

Redundancies

14. Duplicate findGroundLevel() Methods

Status: FIXED Files:

• Ability.java - Made findGroundLevel() public static
• TelegraphInstance.java - Now uses Ability.findGroundLevel()

---

15. Duplicate calculateOffsetPosition() Methods

Status: FIXED Files:

• Ability.java - Added shared static calculateOffsetPosition() method
• AbilityTrap.java - Removed duplicate, uses Ability.calculateOffsetPosition()
• AbilityHazard.java - Removed duplicate, uses Ability.calculateOffsetPosition()

---

16. Unused applyKnockback in AbilityOrb

Status: FIXED File: AbilityOrb.java

Removed the unused applyKnockback() method.

---

17. Duplicate Reset/Interrupt Cleanup

Status: NOT FIXED (Low Priority) Multiple ability files

Most abilities override both reset() and onInterrupt() with duplicate cleanup code. Pattern should have onInterrupt() call reset() or use shared cleanup method.

---

Minor Issues & Inconsistencies

18. Field Naming Confusion

• AbilityHazard: damagePerSecond with damageInterval - name suggests per-second but applied every N ticks
• AbilityCharge: hitWidth stored as hitRadius in older NBT (backward compat exists)

19. AbilityVortex Entity Lookup Performance

File: AbilityVortex.java:201-212

Iterates through ALL loaded entities every tick to find pulled entities by UUID. Should cache entity references or use entity IDs.

20. Telegraph Yaw Rotation

File: TelegraphRenderer.java:95

Uses GL11.glRotatef(-instance.getYaw(), 0, 1, 0) - negative yaw. Should verify this matches Minecraft's yaw conventions.

21. Missing Null Checks

Various locations where target could be null but is used without defensive checks:

• AbilityProjectile.java:73
• AbilityOrb.java:138-171

---

Missing Features (Enhancement Requests)

These are not bugs but notable missing functionality:

1. No Ability Chaining/Combo System - No way to trigger abilities from other abilities
2. No Cooldown Reduction on Hit - Can't reduce cooldowns based on successful hits
3. No Multi-Target Hit Limit - AOE abilities hit unlimited targets (unlike Vortex's maxTargets)
4. No Damage Type Configuration - All abilities use mob/melee damage, no magic/fire/etc options
5. No Target Validation During Execution - If target dies/despawns mid-ability, behavior varies

---

Priority Summary

Priority	Issue	Status
CRITICAL	Cooldown time system mismatch	FIXED
CRITICAL	AbilityHeal HoT broken	FIXED
CRITICAL	AbilityGuard counter not integrated	FIXED
HIGH	knockbackUp ignored	FIXED
HIGH	Targeting modes unimplemented	FIXED (Removed)
HIGH	Damage source inconsistency	FIXED
MEDIUM	Projectile/Orb/Beam no visuals	Not Fixed
MEDIUM	Hazard/Trap telegraph offset	Not Fixed
LOW	Charge telegraph timing	Not Fixed
LOW	Slam activeTicks timing	Not Fixed
LOW	Code redundancies	FIXED

---

Remaining Issues

The following issues were not addressed in this fix round:

1. Projectile/Orb/Beam Visual Rendering - These abilities still only use particles, no actual entity rendering
2. Hazard/Trap Telegraph Offset Mismatch - Telegraph shows at target, effect spawns with offset
3. Charge Telegraph Direction Timing - Minor visual mismatch between tick 0-1
4. Slam activeTicks Timing - Fixed at 60 ticks regardless of landing time

---

Changes Made

Ability.java

• Removed cooldownEndTime transient field
• Removed startCooldown(), isOnCooldown(), canUse() methods (cooldown managed by DataAbilities)
• Updated tick() to not call startCooldown (DataAbilities handles it)
• Updated reset() to not reset cooldownEndTime
• Made findGroundLevel() public static for shared use
• Added static calculateOffsetPosition() utility method
• Fixed applyNpcKnockback() to accept and use knockbackUp parameter
• Standardized damage source to NpcDamageSource("mob", npc) in both damage methods
• Removed deprecated getters/setters (telegraphColor, castSound, animationId)

IAbility.java (API)

• Removed isOnCooldown() method from interface

DataAbilities.java

• Added AbilityGuard import
• Added guard counter integration in onDamage()
• Added getGuardDamageReduction() helper method
• Removed deprecated isMovementBlocked() method
• Updated knockbackUp javadoc (no longer deprecated)

CombatHandler.java

• Renamed isMovementBlocked() to isAbilityControllingMovement()

TargetingMode.java

• Removed unused modes: RANDOM_ENEMY, LOWEST_HP, HIGHEST_THREAT

AbilityHeal.java

• Fixed HoT by separating findAlliesInRadius() from healing logic
• Allies are now found in onExecute() regardless of instantHeal flag
• Added heal particles during HoT ticks

AbilityHazard.java

• Removed duplicate calculateOffsetPosition(), uses Ability.calculateOffsetPosition()
• Removed deprecated getDamagePerTick()/setDamagePerTick() methods

AbilityTrap.java

• Removed duplicate calculateOffsetPosition(), uses Ability.calculateOffsetPosition()

AbilityOrb.java

• Removed unused applyKnockback() method

TelegraphInstance.java

• Removed duplicate findGroundLevel(), uses Ability.findGroundLevel()