Skip to content

Conversation

@Haberno
Copy link
Collaborator

@Haberno Haberno commented Jun 23, 2025

PLEASE MERGE THIS

Haberno added 3 commits June 22, 2025 21:35
- b3147 is the final working version that is compatible with 1.8.8 upgrading will break it as the newer plugin ymls use a syntax that doesn't work with the old bukkit parser.
@Haberno Haberno requested a review from estebangarcia21 June 23, 2025 01:42
Copilot AI review requested due to automatic review settings December 26, 2025 04:24
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors and extends the Minecraft Pit server plugin with significant architectural improvements and new features. The changes span across multiple systems including items, enchantments, GUIs, NPCs, and build configurations.

Key Changes

  • Item System Refactoring: Introduced a new Item interface with separated default, shop, and mystic items, centralizing item configuration and build logic
  • GUI System Enhancement: Added dynamic row sizing, close handlers, locked slots, and click handler builders for more flexible inventory management
  • NPC System Redesign: Refactored NPCs to use a centralized click handler and definition-based configuration with level requirements
  • New Enchantments: Added 5 new enchantments (Volley, PushComesToShove, PinDown, WhatDoesntKillYou, Explosive)
  • Build System Updates: Upgraded Kotlin to 1.9.22, optimized Gradle configuration, added development tasks, and improved dependency management

Reviewed changes

Copilot reviewed 100 out of 106 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
plugin.yml Added new commands (set, freshpants), fixed description formatting
build.gradle.kts Upgraded Kotlin, optimized dependencies, added dev tasks, minimization config
Timer.kt Added tickInterval parameter for throttled callbacks
Player.kt Added utility functions for inventory management and item checking
NPC.kt Refactored to centralized handler with level-gated access
ItemStack.kt Added color support, data parameter, leather armor coloring
GUI.kt Enhanced with dynamic sizing, close handlers, locked slots
Shop.kt Complete shop GUI implementation with dynamic pricing and multi-items
Items (various) New shop/default/mystic item implementations
Enchants (various) New enchantments and description refactoring
Commands Added SetStats and FreshPantsCommand, refactored to PluginCommand interface
DamageIndicator.kt New damage indicator display system
Main.kt Updated registration for items, commands, and NPC handler
Files not reviewed (5)
  • .idea/copilot.data.migration.agent.xml: Language not supported
  • .idea/gradle.xml: Language not supported
  • .idea/kotlinc.xml: Language not supported
  • .idea/misc.xml: Language not supported
  • .idea/vcs.xml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +114 to +117
if (player.data.level < level) {
player.sendMessage(replaceChatColorTags("<red>You must be level $level to access this!</red>"))
player.playSound(player.location, Sound.VILLAGER_NO, 1.0f, 1.0f)
// return // TODO: UNCOMMENT FOR PRODUCTION
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TODO comment references a production check that is currently commented out. This allows any player regardless of level to access NPCs. Either remove the TODO and commented code if this is intentional for development, or uncomment line 117 for production builds.

Copilot uses AI. Check for mistakes.
return rootNBT.getString(pitItemIdKey(signature))
}

// Will nee to overhaul
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in comment: "Will nee to overhaul" should be "Will need to overhaul".

Copilot uses AI. Check for mistakes.
val enchantTier = it.enchantTier
val range = range[enchantTier] ?: undefPropErr("range", enchantTier)
val explosionPitch = explosionPitch[enchantTier] ?: undefPropErr("explosionPitch", enchantTier)
val explosionParticle = explosionParticle[enchantTier] ?: undefPropErr("explosion)Particle", enchantTier)
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in variable name: "explosion)Particle" should be "explosionParticle" (closing parenthesis should be removed).

Copilot uses AI. Check for mistakes.
Comment on lines +108 to +112
for (item in contents) {
if (item.type == Material.LEATHER_HELMET) {
freshPants.add(item)
}
}
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete implementation: The check for Material.LEATHER_HELMET on line 109 should check for Material.LEATHER_LEGGINGS since this is a pants bundle. Using helmet material will check for the wrong item type.

Copilot uses AI. Check for mistakes.
)

private val description: EnchantDescription = {
"Your hits <green>heal</green> you for <red>${damagedHearts[it]?.toInt()}${Text.HEART}</red><br/>and them for <red>${damagerHearts[it]?.toInt()}${Text.HEART}</red> ($cooldown}s cooldown)"
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string interpolation syntax $cooldown}s is missing the opening brace. It should be ${cooldown.seconds()}s to properly display the cooldown value.

Copilot uses AI. Check for mistakes.
Comment on lines +62 to +80
fun playerHasItem(player: Player, material: Material): Boolean {
val inventory = player.inventory

// Check armor slots
val hasEquipped = when {
isHelmet(material) -> inventory.helmet?.type == material
isChestplate(material) -> inventory.chestplate?.type == material
isLeggings(material) -> inventory.leggings?.type == material
isBoots(material) -> inventory.boots?.type == material
else -> false
}

if (!hasEquipped) {
return false
}

// Check main inventory
return inventory.contents.any { it?.type == material }
}
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic check if (!hasEquipped) on line 74 returns false, but the function then unconditionally returns true on line 79, meaning this function will always return true regardless of whether the armor piece is equipped. The logic should be: return true if the player HAS the item equipped OR in inventory, not check if they don't have it equipped then always return true.

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +48
if (isLeather(material) && isLeggings(material)) {
itemMeta = itemMeta as LeatherArmorMeta
itemMeta.color = pantsColors(itemColor ?: "white")
}
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition checks if the item is both leather AND leggings, but it should check if it's leather leggings specifically. The current logic will never execute because isLeather(material) && isLeggings(material) can only be true for Material.LEATHER_LEGGINGS. Consider simplifying to material == Material.LEATHER_LEGGINGS.

Copilot uses AI. Check for mistakes.
Comment on lines +94 to +95
// Prevent self damage
if (entity !== shooter) return@forEach
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition if (entity !== shooter) on line 95 inverts the logic - it returns early if the entity is NOT the shooter, meaning only the shooter will receive damage. This should be if (entity == shooter) to prevent self-damage as the comment indicates.

Copilot uses AI. Check for mistakes.
Comment on lines +39 to +44
if (respawnLocations.isEmpty()) {
player.teleport(randomSpawnLocation)
} else {
val location = respawnLocations.remove(player.uniqueId)
player.teleport(location)
}
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The respawnLocations check on line 39 uses isEmpty() but should check if the map contains the player's UUID. An empty map would prevent all players from using tactical insertion. Change to if (!respawnLocations.containsKey(player.uniqueId)) or use respawnLocations[player.uniqueId] with null-safe handling.

Copilot uses AI. Check for mistakes.
Comment on lines +32 to +39
var tickCount = 0
val throttledOnTick = onTick?.let {
Runnable {
if ((tickCount % tickInterval).toInt() == 0) {
it.run()
}
tickCount++
}
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tickCount variable is never incremented in the throttled onTick callback. This means the modulo check will always evaluate to true on the first call, and the counter will never advance. Move the tickCount++ statement before the if condition, or redesign the logic to properly track tick intervals.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants