Enhance Building with Packetmine and more finegrained material control #107
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces extensive changes across multiple modules. Mixins, configurations, and utility classes have been updated to streamline block breaking, placement, and inventory management. New DSLs and event hooks have been added to simplify container selection and request handling. Additionally, several modules now integrate hotbar and rotation events, and various packet handling methods have been refactored or newly introduced. The overall control flow for break, placement, and request operations has been enhanced with clearer abstractions and improved event pre/post hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant P as Player
participant BT as BuildTask
participant BM as BreakManager
participant S as Server
P->>BT: Initiate block break action
BT->>BM: Dispatch break request (preEvent)
BM->>S: Send break packet with hotbarIndex & config
S-->>BM: Confirm block state update
BM->>BT: Post break event (postEvent)
BT->>P: Update game state with break completion
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (21)
common/src/main/kotlin/com/lambda/interaction/material/container/containers/MainHandContainer.kt (1)
46-46: LGTM! Consider adding documentation for clarity.The change to use
matchingStacksdirectly maintains the same functionality while reducing verbosity. However, consider adding a brief comment to document where this method is inherited from, to help future maintainers understand the code better.+ // Inherited from MaterialContainer val moveStack = matchingStacks(selection).firstOrNull() ?: run {common/src/main/kotlin/com/lambda/util/player/SlotUtils.kt (2)
30-30: LGTM! Consider adding KDoc documentation.The implementation correctly combines both hand stacks into an immutable list. Consider adding KDoc to document the list order (main hand first, off hand second) for better maintainability.
+ /** Returns a list containing the main hand stack (index 0) and off hand stack (index 1). */ val ClientPlayerEntity.handStacks: List<ItemStack> get() = listOf(mainHandStack, offHandStack)
30-30: Consider caching the list for better performance.Since the list is recreated on every property access, consider caching it if the property is accessed frequently in performance-critical code paths.
- val ClientPlayerEntity.handStacks: List<ItemStack> get() = listOf(mainHandStack, offHandStack) + val ClientPlayerEntity.handStacks: List<ItemStack> by lazy { listOf(mainHandStack, offHandStack) }Note: Only apply this optimization if profiling shows that the property access is a performance bottleneck. The current implementation might be sufficient for most use cases.
common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarManager.kt (2)
26-27: Consider reducing coupling with mixin implementation details.The direct imports of mixin classes suggest tight coupling with implementation details. Consider introducing an abstraction layer or interface to maintain better separation of concerns.
29-34: Enhance documentation with class-level details.While the mixin references are helpful, consider adding:
- A description of the HotbarManager's purpose and responsibilities
- How it interacts with the referenced mixin methods
- Key usage examples or scenarios
common/src/main/kotlin/com/lambda/interaction/construction/result/BuildResult.kt (1)
208-209: Consider enhancing the error message.The error message could be more descriptive by including context about where the transfer failed.
- .transfer(MainHandContainer, inventory) ?: MaterialContainer.FailureTask("Couldn't find $neededSelection anywhere.") + .transfer(MainHandContainer, inventory) ?: MaterialContainer.FailureTask("Failed to transfer $neededSelection to main hand. Item not found in available containers.")common/src/main/kotlin/com/lambda/util/item/ItemUtils.kt (1)
124-135: Add documentation and input validation.The function logic is correct, but could benefit from:
- KDoc documentation explaining the purpose, parameters, and return value.
- Input validation for null/empty parameters.
Apply this diff to improve the function:
+ /** + * Finds the best tools for breaking a specific block state. + * + * @param blockState The block state to find tools for + * @param availableTools Set of tools to consider, defaults to all tools + * @return List of tools sorted by mining speed (best tools first) + * @throws IllegalArgumentException if blockState is null or availableTools is empty + */ fun findBestToolsForBreaking( blockState: BlockState, availableTools: Set<Item> = tools, - ) = availableTools.map { + ): List<Item> { + require(availableTools.isNotEmpty()) { "availableTools cannot be empty" } + return availableTools.map { it to it.getMiningSpeedMultiplier(it.defaultStack, blockState) }.filter { (item, speed) -> speed > 1.0 && item.isSuitableFor(blockState) }.sortedByDescending { it.second }.map { it.first } + }common/src/main/kotlin/com/lambda/interaction/construction/context/PlaceContext.kt (1)
47-47: Consider adding documentation for the primeDirection property.The nullable
primeDirectionproperty is a good addition for enhanced building control. However, its purpose and usage could be clearer with KDoc documentation.Add KDoc documentation above the property:
+ /** + * The primary direction for block placement. + * When specified, this direction takes precedence during placement calculations. + */ val primeDirection: Direction?common/src/main/kotlin/com/lambda/config/groups/InventorySettings.kt (3)
29-29: Fix typographical error.
"droped" should be "dropped".- "Items that will be included when checking for a free slot / are allowed to be droped when inventory is full" + "Items that will be included when checking for a free slot / are allowed to be dropped when inventory is full"
39-46: Review tool naming and usage semantics.
The properties (useWoodenTools,useStoneTools, etc.) correctly toggle tool usage. However, theuseFlintAndSteeldescription is ambiguous ("Use flint and steel to mine blocks?")—consider clarifying if flint and steel are truly intended for mining or just for block updates (ignition).
48-50: Page enumeration clarity.
While thePageenum effectively segregates config sections, consider naming or documentation that reinforces its usage context—e.g.,InventoryModeorInventoryPage.common/src/main/kotlin/com/lambda/config/groups/InventoryConfig.kt (2)
39-42: Consolidate container access flags if they scale up further.
Multiple boolean access flags (e.g.,accessShulkerBoxes,accessChests) can become cumbersome. A future approach could use a single bitmask or an enum set for better scalability.
64-74: Optimize building the allowed tools set.
Building a mutable set and then removing disallowed items works, but you could build the final set by selectively adding allowed items for clarity and efficiency.common/src/main/kotlin/com/lambda/util/BlockUtils.kt (1)
248-262: Expedited block-breaking for bamboo
Returning1.0ffor swords on bamboo blocks is presumably a desired special case. Consider adding a clarifying comment to explain the rationale for bypassing normal hardness calculations.common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt (2)
473-488: Fallback when no tools are found
IfbestToolsis empty, the code tries verifying the current item’s ability to mine. Consider scanning offhand or specialized containers, if relevant, to avoid missed mining opportunities.
490-521: Flexible tool selection
Handling Silk Touch, Fortune, and default paths is a comprehensive approach. Logging each path could improve debugging if the final selection fails.common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (1)
289-305: Central placeBlock method
Centralizing block placement logic is a clean approach. Consider improved error reporting ifactionResultis neitherSUCCESSnorCONSUME.common/src/main/kotlin/com/lambda/config/groups/BuildConfig.kt (1)
46-46: Consider groupingbreakWeakBlockswith other breaking properties.The
breakWeakBlocksproperty seems out of place between the breaking and placing sections.// Breaking val breakMode: BreakMode val breakThreshold: Float val doubleBreak: Boolean val breakDelay: Int val sounds: Boolean val particles: Boolean val breakingTexture: Boolean val rotateForBreak: Boolean val breakConfirmation: BreakConfirmationMode val breaksPerTick: Int val forceSilkTouch: Boolean val forceFortunePickaxe: Boolean val minFortuneLevel: Int +val breakWeakBlocks: Boolean val ignoredBlocks: Set<Block> -val breakWeakBlocks: Boolean // Placingcommon/src/main/java/com/lambda/mixin/render/InGameHudMixin.java (1)
40-43: LGTM! Consider adding error logging for invalid hotbar indices.The implementation correctly handles invalid hotbar indices by returning an empty ItemStack. However, logging invalid indices could help with debugging.
@Redirect(method = "tick()V", at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/player/PlayerInventory;getMainHandStack()Lnet/minecraft/item/ItemStack;")) private ItemStack onTick(PlayerInventory inventory) { - return isValidHotbarIndex(inventory.selectedSlot) ? inventory.main.get(inventory.selectedSlot) : ItemStack.EMPTY; + if (!isValidHotbarIndex(inventory.selectedSlot)) { + LOGGER.warn("Invalid hotbar index: {}", inventory.selectedSlot); + return ItemStack.EMPTY; + } + return inventory.main.get(inventory.selectedSlot); }common/src/main/kotlin/com/lambda/interaction/material/ContainerSelection.kt (1)
22-25: Enhance class documentation with usage examples.While the documentation explains the purpose, adding examples would make it more user-friendly.
/** * ContainerSelection is a class that holds a predicate for matching MaterialContainers. * It can be combined using "and", "or", etc. + * + * Example usage: + * ``` + * val selection = selectContainer { + * ofAnyType(MaterialContainer.Rank.HOTBAR) and + * matches(stackSelection) + * } + * ``` */common/src/main/kotlin/com/lambda/interaction/material/container/ContainerManager.kt (1)
42-42: Consider implementing the TODO for container caching.The TODO suggests implementing a cached region-based storage system for containers, which could improve performance.
Would you like me to help design and implement the caching system for containers?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
common/src/main/java/com/lambda/mixin/entity/PlayerInventoryMixin.java(3 hunks)common/src/main/java/com/lambda/mixin/render/InGameHudMixin.java(1 hunks)common/src/main/kotlin/com/lambda/command/commands/TransferCommand.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/BuildConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/BuildSettings.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/InventoryConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/InventorySettings.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt(3 hunks)common/src/main/kotlin/com/lambda/interaction/construction/context/BuildContext.kt(2 hunks)common/src/main/kotlin/com/lambda/interaction/construction/context/PlaceContext.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/construction/result/BreakResult.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/construction/result/BuildResult.kt(4 hunks)common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt(8 hunks)common/src/main/kotlin/com/lambda/interaction/material/ContainerSelection.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/material/StackSelection.kt(11 hunks)common/src/main/kotlin/com/lambda/interaction/material/container/ContainerManager.kt(3 hunks)common/src/main/kotlin/com/lambda/interaction/material/container/MaterialContainer.kt(2 hunks)common/src/main/kotlin/com/lambda/interaction/material/container/containers/MainHandContainer.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarRequest.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/client/TaskFlowModule.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/debug/SilentSwap.kt(1 hunks)common/src/main/kotlin/com/lambda/task/tasks/AcquireMaterial.kt(1 hunks)common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt(6 hunks)common/src/main/kotlin/com/lambda/util/BlockUtils.kt(4 hunks)common/src/main/kotlin/com/lambda/util/item/ItemUtils.kt(3 hunks)common/src/main/kotlin/com/lambda/util/player/SlotUtils.kt(1 hunks)common/src/main/resources/lambda.accesswidener(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (62)
common/src/main/resources/lambda.accesswidener (2)
69-69: LGTM! Widening access to enable sequenced packet handling.The addition of
sendSequencedPacketmethod access aligns with the PR's objective of enhancing building functionality by enabling more granular control over block interactions.
70-70: LGTM! Widening access to enable world update management.The addition of
pendingUpdateManagerfield access complements the building enhancements by providing control over world updates during block interactions.common/src/main/kotlin/com/lambda/interaction/material/StackSelection.kt (4)
227-229: LGTM! Well-structured DSL marker implementation.The
@DslMarkermeta-annotation is correctly used to create a custom DSL marker, which helps prevent implicit receiver confusion in nested DSL blocks.
112-129: LGTM! Well-implemented collection predicates.The new functions
isOneOfItemsandisOneOfStacksprovide efficient collection-based predicates with clear documentation and idiomatic Kotlin implementation.
240-249: LGTM! Well-designed extension functions.The new extension functions provide a fluent API for item selection with proper Java interoperability through
@JvmNameannotations.
1-271: LGTM! Clean documentation and consistent code style.The file maintains high-quality documentation and follows Kotlin coding conventions consistently throughout.
common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarManager.kt (1)
36-38: Verify the impact of changing the fallback slot from -1 to 0.The change from -1 to 0 means that when no slot is selected, the first hotbar slot (index 0) will be used instead of indicating an invalid state. Please verify:
- Whether this change is intentional
- If any code relies on detecting invalid states using -1
- The impact on item selection and interaction behavior when no slot is explicitly selected
common/src/main/kotlin/com/lambda/interaction/construction/result/BuildResult.kt (3)
25-25: LGTM!The import addition aligns with the refactoring from
ItemtoStackSelection.
193-199: LGTM! Improved type safety with StackSelection.The refactoring from
ItemtoStackSelectionenhances type safety and provides more granular control over item selection. The class rename better reflects its purpose.Also applies to: 202-203, 208-209
221-221: LGTM!The comparison logic has been correctly updated to maintain consistency with the class rename.
Also applies to: 258-258
common/src/main/kotlin/com/lambda/util/item/ItemUtils.kt (1)
149-150: LGTM!The simplified conditional statements improve readability while maintaining the same functionality.
Also applies to: 155-156, 161-161
common/src/main/kotlin/com/lambda/interaction/material/container/MaterialContainer.kt (2)
48-48: LGTM!The use of
toItemCountimproves the readability of item counts while maintaining the infinity check.
54-54: LGTM!The use of
toItemCountimproves the readability of item counts while maintaining the infinity check.common/src/main/kotlin/com/lambda/interaction/construction/context/BuildContext.kt (3)
23-23: LGTM! Necessary imports added for new functionality.The new imports support the LocalizedStack data class and are properly scoped.
Also applies to: 26-26
44-44:Details
✅ Verification successful
Consider moving LocalizedStack to a separate file if used broadly.
The data class is well-structured and its purpose is clear. However, if this class is used beyond the BuildContext, consider moving it to a separate file for better modularity and reusability.
🏁 Script executed:
#!/bin/bash # Check if LocalizedStack is used in other files echo "Checking LocalizedStack usage across the codebase..." rg "LocalizedStack" --type kotlinLength of output: 317
LocalizedStack Placement Verified
The data class is well-structured, and its usage is currently limited to this file. Since there are no other references found across the codebase, its placement within BuildContext appears appropriate for now. If future requirements show that it is used in multiple contexts, then consider moving it to a separate file to enhance modularity and reusability.
39-39:Details
✅ Verification successful
Verify consistent usage of hotbarIndex across the codebase.
The change from
handtohotbarIndexlooks good and provides more direct control. However, let's verify that:
- All related contexts have been updated consistently
- The hotbar index is properly validated where used
🏁 Script executed:
#!/bin/bash # Search for any remaining references to Hand in context files echo "Searching for remaining Hand references in context files..." rg -l "Hand" --type kotlin "context" # Search for hotbarIndex usage to verify consistent implementation echo "Checking hotbarIndex usage..." rg "hotbarIndex" --type kotlinLength of output: 963
Consistent implementation of
hotbarIndexverified across contextsI confirmed that all occurrences of
hotbarIndexare now used consistently in the following files:
common/src/main/kotlin/com/lambda/interaction/construction/context/PlaceContext.ktcommon/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.ktcommon/src/main/kotlin/com/lambda/interaction/construction/context/BuildContext.ktcommon/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.ktcommon/src/main/kotlin/com/lambda/task/tasks/BuildTask.ktThere are no lingering references to the old
Handproperty, and the refactoring to use anIntforhotbarIndexis appropriately applied for managing hotbar indices.Please ensure that any necessary bounds validation (e.g., confirming the index is within the expected range) is handled in the consuming logic.
common/src/main/kotlin/com/lambda/interaction/construction/context/PlaceContext.kt (1)
42-42:Details
❓ Verification inconclusive
LGTM! The change from
handtohotbarIndeximproves material control granularity.The switch to using hotbar indices instead of hand states allows for more precise control over block placement materials.
Let's verify the interface implementation:
🏁 Script executed:
#!/bin/bash # Description: Verify that BuildContext interface defines hotbarIndex property ast-grep --pattern 'interface BuildContext { $$$ val hotbarIndex: Int $$$ }'Length of output: 84
I'll run an additional check to verify if the
BuildContextinterface is defined with thehotbarIndexproperty. Please see the new shell script below:Once you review the output, please verify manually that the interface (likely
BuildContext) includes thehotbarIndexproperty.LGTM! The change from
handtohotbarIndeximproves material control granularity.
However, please ensure that the interface requiring this property is updated accordingly.
🏁 Script executed:
#!/bin/bash # Search for the BuildContext interface definition in Kotlin files rg -n "interface BuildContext" . # Search for occurrences of "override val hotbarIndex" to verify its usage rg -n "override val hotbarIndex" .Length of output: 344
LGTM! The shift from using
handtohotbarIndexsignificantly improves control granularity for block placement.
- Observation: The
PlaceContext.ktfile properly overrideshotbarIndex(line 42), and theBuildContextinterface is present in the codebase.- Action Required: Please manually verify that the
BuildContextinterface (located atcommon/src/main/kotlin/com/lambda/interaction/construction/context/BuildContext.kt) explicitly declares the propertyval hotbarIndex: Intto ensure the override is fully compliant with the interface contract.common/src/main/kotlin/com/lambda/config/groups/InventorySettings.kt (1)
27-27: Consider verifying the default page logic.
While defaultingpagetoPage.Containermakes sense in many scenarios, ensure it's always in line with the user's intention.common/src/main/kotlin/com/lambda/config/groups/InventoryConfig.kt (1)
44-53: Validate container prioritization logic.
The current approach withcontainerSelectionis clear but consider edge cases: if all booleans are false, the default set is empty. Review how the rest of the code handles an empty container set.common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt (3)
48-48: Enforce hotbarIndex range checks.
hotbarIndexshould remain within valid hotbar bounds (typically 0–8). Consider validating or clamping its value to avoid future indexing errors.
87-95: Prevent potential arithmetic errors.
buildConfig.breakThresholdmight be zero or negative. Ensure it's strictly positive to avoid division by zero or unexpected behavior.
97-115: Ensure robust packet sending.
The new break packet methods (startBreakPacket,stopBreakPacket,abortBreakPacket) rely on the network handler being valid. Consider handling exceptions or queuing if the connection is unavailable.common/src/main/kotlin/com/lambda/util/BlockUtils.kt (8)
26-27: Use consistent import grouping
These new imports forBambooBlockandBambooShootBlockseem fine. Ensure they are meaningfully utilized and checked for potential duplication in other files.
85-89: Validate new enchantment & effect imports
These imports (EnchantmentHelper,Enchantments,StatusEffectUtil,StatusEffects,PlayerEntity) align with the advanced mining speed logic below. No immediate concerns spotted.
93-97: Check item imports
Pulling inItemStack,SwordItem,FluidTags, andBlockViewis consistent with your new item-centric calculations.
100-105: Verify shadowing of offset
The extension properties and functions (likeblockPos,flooredPos, etc.) appear helpful. However, confirm thatoffsetwithEightWayDirectiondoes not overshadow existing vanillaoffset.
165-165: Renamed property
ChanginginteractionClassestointeractionBlocksmore accurately describes its contents.
243-246: Potential division by zero
calcItemBlockBreakingDeltacan return0.0f(indicating unbreakable). In such cases,1 / 0.0fproducesInfinity. Verify this behavior is intentional and won't cause unexpected logic results.
264-266: Basic harvest logic
canHarvestis straightforward and aligns well with vanilla checks (isToolRequiredandisSuitableFor).
267-301: Layered mining speed modifiers
The logic for Efficiency, Haste, and Mining Fatigue increments is thorough. Check that cumulative multipliers stay in sync with evolving versions or modded enchantments.common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt (6)
26-26: New BuildContext import
Used for bridging building interactions with the simulator’s logic.
33-38: Imports for container and stack selections
AddingselectContainer,select,selectStack, and related container classes suggests refined and dynamic item selection.
52-52: Importing calcItemBlockBreakingDelta
Ties the simulator’s break checking to the newly introduced block-breaking speed logic inBlockUtils.
57-57: Import findBestToolsForBreaking
Facilitates more precise tool matching for block breaking.
59-59: Hotbar extension usage
Ensure that referencingplayer.hotbarfor direct slot management doesn’t conflict with existing inventory handling.
64-64: Enchantment usage
Check whether newly referencedEnchantmentslogic aligns with the existing item-breaking requirements.common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (14)
47-48: Hotbar configuration imports
HotbarConfigandHotbarRequestfacilitate direct management of hotbar items. Looks consistent with the updated building logic.
53-54: Extended BlockUtils usage
Adds usage forcalcItemBlockBreakingDeltaandfluidState, giving the task advanced break mechanics and fluid context checks.
62-62: Hotbar extension
ImportinghotbarfromSlotUtils; be sure the hotbar switching logic stays consistent with your item usage flow.
64-64: Sound effect import
PositionedSoundInstancecan enrich feedback when blocks are broken or placed. No immediate issues.
67-72: Extended packet and item handling
Imports forItemEntity,ItemStack,PlayerActionC2SPacket,SoundCategory, andHandsupport the new break/place approach.
93-95: New tracking variables
breaking,breakingTicks, andsoundsCooldownneatly centralize break progress and audio cooldown management.
110-133: Refined interaction flow
This tick-based logic elegantly branches into place or break contexts. Thehotbar.requestcall ensures correct slot usage. Confirm you handle stalling or partial success from the hotbar request.
257-259: BlockUpdate event
Removing the matching context frompendingInteractionsis correct. Validate that out-of-order or delayed updates don’t cause partial orphan contexts.
264-268: Await-then-break approach
AwaitThenBreakhelps confirm state changes server-side. Watch out for blocks with multiple break stages or partial progress.
307-368: updateBlockBreakingProgress
Coordinates break progression, sounds, and particles. Keep an eye on concurrency if multiple blocks are chipped at once, especially with partial states.
370-385: onBlockBreak confirmations
HandlingBreakConfirmationModevariations is tidy. Ensure none of these modes lead to desync if the server delays block removal.
387-404: destroyBlock logic
Checks for permissions, fluid states, and calls block lifecycle methods. This is properly robust.
406-415: Managing block-breaking texture
A negative value like-1resets the crack overlay. Confirm clients quickly reflect it in all scenarios (including re-log or chunk reload).
417-464: Attack block routine
InPacketmode, multiplePlayerActionC2SPackets are dispatched quickly. Confirm no anti-cheat triggers or extraneous server load from repeated spam.common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarRequest.kt (1)
25-25: LGTM! Good improvement to the API.Making the priority parameter optional with a default value of 0 improves the API's usability while maintaining backward compatibility.
common/src/main/kotlin/com/lambda/config/groups/BuildConfig.kt (2)
31-38: LGTM! Comprehensive breaking mechanics configuration.The new properties provide granular control over block breaking behavior:
- Visual feedback (
sounds,particles,breakingTexture)- Performance tuning (
breakThreshold,breakDelay)- Tool selection (
forceFortunePickaxe,minFortuneLevel)Also applies to: 42-43
53-61: LGTM! Well-structured enums for breaking mechanics.The new enums provide clear and type-safe options:
BreakMode: Distinguishes between vanilla and packet-based breakingBreakConfirmationMode: Provides fine-grained control over break confirmation flowcommon/src/main/kotlin/com/lambda/task/tasks/AcquireMaterial.kt (1)
36-36: LGTM! Good use of Kotlin extension functions.The change from a static method to an instance method improves encapsulation and follows Kotlin's idiomatic approach.
common/src/main/kotlin/com/lambda/module/modules/debug/SilentSwap.kt (1)
35-40: LGTM! Well-structured debug module.The module is properly configured with appropriate metadata and tags. Good separation of concerns with dedicated hotbar settings.
common/src/main/kotlin/com/lambda/module/modules/client/TaskFlowModule.kt (1)
34-34: LGTM! Clean integration of hotbar settings.The addition of the Hotbar page and its corresponding settings follows the established pattern and maintains consistency with the module's architecture.
Also applies to: 42-42
common/src/main/kotlin/com/lambda/config/groups/BuildSettings.kt (3)
40-47: LGTM! Well-structured break settings with appropriate defaults.The new break settings are well-organized with clear descriptions, appropriate validation ranges, and logical default values.
48-49: LGTM! Enhanced break confirmation with more granular control.The change from boolean to enum for break confirmation provides better control over the breaking behavior.
53-61: LGTM! Well-structured fortune pickaxe settings with proper dependencies.The fortune pickaxe settings are well-organized with proper visibility conditions, and the interaction timeout logic correctly handles both placement and break confirmation modes.
common/src/main/kotlin/com/lambda/command/commands/TransferCommand.kt (1)
51-51: LGTM! Improved encapsulation with extension function.The change to use
selection.containerWithMaterial()aligns with the refactoring in ContainerManager and improves code organization.common/src/main/kotlin/com/lambda/interaction/material/container/ContainerManager.kt (2)
93-104: LGTM! Improved method organization with extension functions.The refactoring to use extension functions for transfer and container selection improves code organization and readability.
110-126: LGTM! Enhanced container filtering with flexible selection.The addition of containerSelection parameter and updated filtering logic provides more flexibility while maintaining backward compatibility.
| override fun resolve() = | ||
| findBestAvailableTool(blockState, inventory = inventory) | ||
| ?.select() | ||
| ?.transfer(MainHandContainer, inventory) | ||
| ?: selectStack { | ||
| isItem(badItem).not() | ||
| }.transfer(MainHandContainer, inventory) | ||
| selectStack { | ||
| isItem(badItem).not() | ||
| }.transfer(MainHandContainer, inventory) | ||
| ?: MaterialContainer.FailureTask("Couldn't find a tool for ${blockState.block.name.string} with $badItem in main hand.") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Verify the intentional simplification of tool selection logic.
The removal of findBestAvailableTool in favor of selecting any non-bad item could lead to suboptimal tool selection. This might result in:
- Slower block breaking times due to using non-optimal tools
- Increased tool durability consumption
- Less efficient resource usage
Consider maintaining the previous behavior of attempting to find the best tool first before falling back to any non-bad item.
override fun resolve() =
- selectStack {
- isItem(badItem).not()
- }.transfer(MainHandContainer, inventory)
+ selectStack {
+ // First try to find the best tool for the block
+ isBestToolFor(blockState) and isItem(badItem).not()
+ }.transfer(MainHandContainer, inventory)
+ ?: selectStack {
+ // Fall back to any non-bad item if no optimal tool is found
+ isItem(badItem).not()
+ }.transfer(MainHandContainer, inventory)
?: MaterialContainer.FailureTask("Couldn't find a tool for ${blockState.block.name.string} with $badItem in main hand.")Let's verify if this change was intentional and if there are any specific reasons for removing the optimal tool selection:
🏁 Script executed:
#!/bin/bash
# Search for related changes in tool selection logic
rg -l "findBestAvailableTool|findBestToolsForBreaking|isBestToolFor" --type kotlinLength of output: 229
Action Required: Confirm Tool Selection Behavior
The changes in BreakResult.kt (lines 99-103) remove the logic for selecting the best available tool—instead, it now picks any tool that isn’t flagged as badItem. Note that alternative optimal tool selection logic (using constructs like isBestToolFor) still exists in other areas of the codebase (e.g., in BuildSimulator.kt and ItemUtils.kt). This discrepancy might be intentional, but it raises potential concerns regarding:
- Suboptimal tool performance (e.g., slower block breaking)
- Increased wear on tools due to non-optimal selection
- Overall user experience implications
If this change was intentional for simplification, please confirm that its trade-offs are acceptable. Otherwise, consider reintroducing a two-step selection approach. For example:
override fun resolve() =
- selectStack {
- isItem(badItem).not()
- }.transfer(MainHandContainer, inventory)
+ selectStack {
+ // Prefer the best available tool for the block
+ isBestToolFor(blockState) and isItem(badItem).not()
+ }.transfer(MainHandContainer, inventory)
+ ?: selectStack {
+ // Fall back to any suitable tool if an optimal one isn't found
+ isItem(badItem).not()
+ }.transfer(MainHandContainer, inventory)Please verify whether this design decision was intentional and that it aligns with the overall tool selection strategy across the project.
common/src/main/java/com/lambda/mixin/entity/PlayerInventoryMixin.java
Outdated
Show resolved
Hide resolved
| class ContainerSelection { | ||
| private var selector: (MaterialContainer) -> Boolean = { true } | ||
|
|
||
| /** | ||
| * Tests whether the provided container matches this selection. | ||
| */ | ||
| @ContainerSelectionDsl | ||
| fun matches(container: MaterialContainer): Boolean = selector(container) | ||
|
|
||
| /** | ||
| * Returns a function that matches containers having at least one stack | ||
| * which matches the given StackSelection. | ||
| */ | ||
| @ContainerSelectionDsl | ||
| fun matches(stackSelection: StackSelection): (MaterialContainer) -> Boolean = | ||
| { container -> container.matchingStacks(stackSelection).isNotEmpty() } | ||
|
|
||
| /** | ||
| * Returns a function that checks whether a given MaterialContainer matches the criteria | ||
| * defined in the provided ContainerSelection. | ||
| */ | ||
| @ContainerSelectionDsl | ||
| fun matches(containerSelection: ContainerSelection): (MaterialContainer) -> Boolean = | ||
| { container -> containerSelection.matches(container) } | ||
|
|
||
| /** | ||
| * Returns a function that matches containers whose rank is any of the types provided. | ||
| */ | ||
| @ContainerSelectionDsl | ||
| fun ofAnyType(vararg types: MaterialContainer.Rank): (MaterialContainer) -> Boolean = | ||
| { container -> types.contains(container.rank) } | ||
|
|
||
| /** | ||
| * Returns a function that matches containers whose rank is not any of the types provided. | ||
| */ | ||
| @ContainerSelectionDsl | ||
| fun noneOfType(vararg types: MaterialContainer.Rank): (MaterialContainer) -> Boolean = | ||
| { container -> !types.contains(container.rank) } | ||
|
|
||
| /** | ||
| * Returns a function that combines two container predicates using logical AND. | ||
| */ | ||
| @ContainerSelectionDsl | ||
| infix fun ((MaterialContainer) -> Boolean).and(other: (MaterialContainer) -> Boolean): (MaterialContainer) -> Boolean = | ||
| { container -> this(container) && other(container) } | ||
|
|
||
| /** | ||
| * Returns a function that combines two container predicates using logical OR. | ||
| */ | ||
| @ContainerSelectionDsl | ||
| infix fun ((MaterialContainer) -> Boolean).or(other: (MaterialContainer) -> Boolean): (MaterialContainer) -> Boolean = | ||
| { container -> this(container) || other(container) } | ||
|
|
||
| /** | ||
| * Returns a function that negates the current selection predicate. | ||
| */ | ||
| @ContainerSelectionDsl | ||
| fun ((MaterialContainer) -> Boolean).negate(): (MaterialContainer) -> Boolean = | ||
| { container -> !this(container) } | ||
|
|
||
| companion object { | ||
| @DslMarker | ||
| annotation class ContainerSelectionDsl | ||
|
|
||
| @ContainerSelectionDsl | ||
| fun selectContainer( | ||
| block: ContainerSelection.() -> (MaterialContainer) -> Boolean | ||
| ): ContainerSelection = | ||
| ContainerSelection().apply { selector = block() } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add unit tests for predicate combinations.
The class implements complex predicate combinations. Consider adding unit tests to verify the behavior of:
- Combined predicates using
and/or - Negated predicates
- Stack selection matching
🏁 Script executed:
#!/bin/bash
# Check if tests exist for ContainerSelection
rg -l "ContainerSelectionTest" || echo "No test file found for ContainerSelection"Length of output: 123
Action Required: Implement Comprehensive Unit Tests for ContainerSelection Predicate Logic
The current implementation includes several complex predicate operations (combined predicates using and/or, predicate negation, and stack selection matching), but no tests exist to verify these behaviors. Please add unit tests that cover:
- Combined predicates using the
and/orinfix functions. - Negated predicates via the
negate()extension. - Stack selection matching functionality through
matches(stackSelection).
This will ensure the correctness and stability of the predicate operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt (1)
30-33: Consider filtering out null fromcontextsRight now, the list may contain a
nullentry ifsecondaryContextisnull. If that is unintentional, you could filter it out to keep the list strictly non-null, for example:val contexts = listOfNotNull(primaryContext, secondaryContext)common/src/main/kotlin/com/lambda/module/modules/player/PacketMineRewrite.kt (1)
42-48: Enhance blueprint configurationThe block positions to break are currently hardcoded. Consider supporting dynamic position choices or user configuration for more flexible automation workflows.
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakConfig.kt (1)
24-42: Consider makingignoredBlocksimmutable for thread safety.
If this set is ever updated outside this class, concurrent reads may cause unexpected behavior. Converting it to an immutable set or clearly documenting concurrency expectations can help avoid subtle thread-safety issues.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (4)
46-59: Potential concurrency considerations for shared state.
BreakManagerholds globalbreakingInfosandpendingInteractions. If multiple threads operate on these, consider appropriate synchronization or avoid shared mutable state.
60-116: Avoid overshadowing theitidentifier.
In Kotlin, nested lambdas usingitin both outer and inner scopes (for (it in breakingInfos.reversed())) can be confusing for readability. Consider refactoring variable names.- for (it in breakingInfos.reversed()) { + for (breakInfo in breakingInfos.reversed()) { ... }
131-202: Consider caching repeated block-breaking calculations.
calcItemBlockBreakingDeltais called each tick; for large or quick block updates, performance might degrade. A simple cache keyed by(blockState, item, position)could improve efficiency.
278-297: Handle potential failures fromworld.setBlockState.
IfsetBlockStateunexpectedly fails (e.g., chunk unload mid-action), consider logging or handling that scenario to avoid leaving inconsistent game states.common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (2)
97-104: Commented cleanup logic for breaking textures.The commented-out
onCancelmethod appears to be intended for cleaning up breaking textures when the task is canceled. Consider completing this implementation or removing the commented code.It's best to either implement the breaking texture cleanup functionality or remove the commented code to maintain code cleanliness.
249-254: Commented event listener.The commented-out
WorldEvent.BlockUpdate.Clientlistener appears to be superseded by other functionality. Consider removing this commented code once you're confident it's no longer needed.Remove commented code that's no longer needed to improve code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
common/src/main/kotlin/com/lambda/config/groups/BreakSettings.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/BuildConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/BuildSettings.kt(2 hunks)common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt(3 hunks)common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt(10 hunks)common/src/main/kotlin/com/lambda/interaction/request/RequestHandler.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/player/PacketMineRewrite.kt(1 hunks)common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/kotlin/com/lambda/config/groups/BuildConfig.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (33)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt (1)
24-28: Well-structured data classThe constructor parameters are straightforward, and referencing them in
onBreakis also intuitive. Matches well with the usage inRequest.common/src/main/kotlin/com/lambda/interaction/request/RequestHandler.kt (1)
54-68: Allowing override forupdateRequestMaking
updateRequestopen provides greater flexibility, letting subclasses tailor the request selection process. However, consider verifying thread safety for concurrent requests, especially sincerequestMapis cleared afterwards.common/src/main/kotlin/com/lambda/module/modules/player/PacketMineRewrite.kt (1)
32-36: Module definition is clearDeclaring
PacketMineRewritewith a descriptive name and relevant tags helps keep it distinct as a specialized player-oriented feature.common/src/main/kotlin/com/lambda/config/groups/BuildSettings.kt (3)
21-21: No usage issues identifiedThe
BreakConfirmationModeimport appears valid and is used bybreakSettings.
40-40: Nice consolidation of break behaviorEncapsulating block-breaking properties under
breakSettingskeeps the class organized and eases future maintenance.
47-47: Double-check combined confirmations
interactionTimeoutis now gated by eitherplaceConfirmationorbreakSettings.breakConfirmation != BreakConfirmationMode.None. Confirm that this covers all intended scenarios without unintended skip or delay conditions.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakConfig.kt (3)
1-16: License header looks good.
No issues here; the GPL license is correctly referenced and included.
43-45: Request delegation is straightforward.
Forwarding the request toBreakManageris clean and keeps configuration concerns separate.
47-56: Enums are concise and self-explanatory.
TheBreakModeandBreakConfirmationModeenumerations are clear.common/src/main/kotlin/com/lambda/config/groups/BreakSettings.kt (2)
1-16: License header looks good.
No issues; the GPL license is included properly.
25-45: Verify parameter ranges and dependencies.
Properties likeminFortuneLevelare only valid ifforceFortunePickaxeis true. Ensure edge cases (e.g., disablingforceFortunePickaxemid-run) are handled gracefully in any surrounding logic.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (4)
1-16: License header looks good.
No issues found in this section.
204-259: Check repeatedpendingUpdateManagerincrements.
Incrementing the sequence multiple times in a row (lines 247–249) might create unexpected sequence gaps. Ensure these increments align with your packet-handling logic.
261-276: Break confirmation logic is clear.
The three modes (None,BreakThenAwait,AwaitThenBreak) are handled cleanly and are easy to follow.
310-354: Subclass separation for primary vs. secondary breaks is well-organized.
The override ofgetBreakThreshold()inSecondaryBreakInfoand usage ofnullify()methods keep the code straightforward.common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt (4)
44-44: Transition from hand to hotbarIndex enhances flexibility.Replacing
handwithhotbarIndexprovides more precise control over which item is used for breaking, allowing direct access to specific hotbar slots rather than just main or off hand.
46-47: Good design: Adding configuration dependencies.Adding
buildConfigandinventoryConfigproperties to theBreakContextclass allows for better configuration injection, ensuring the context has all the information needed for making decisions about breaking blocks.
76-76: More granular rotation control.Updated to use the more specific
breakSettings.rotateForBreakconfiguration property, which provides better control over when rotation should occur for breaking blocks.
83-100: Well-structured packet handling implementation.The new methods for packet handling (
startBreakPacket,stopBreakPacket,abortBreakPacket, and the privatebreakPacket) provide a clean API for sending the various types of block breaking packets. This encapsulation improves modularity and maintainability.common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt (5)
290-290: Type-safe block interaction check.Using
hitBlock::class in BlockUtils.interactionBlocksis a more type-safe approach for checking if a block requires sneaking for interaction.
471-486: Enhanced tool selection logic.The implementation now uses
findBestToolsForBreakingfor a more sophisticated tool selection process, considering block type and tool efficiency. The error handling for when no appropriate tools are available is comprehensive.
488-494: Advanced enchantment-based tool selection.The tool selection now considers enchantments like Silk Touch and Fortune based on configuration preferences, offering much more granular control over which tools are selected for specific breaking scenarios.
495-500: Efficient container selection for tool swapping.Implemented two container selection strategies: one for silent swapping within the hotbar and one that respects the broader inventory configuration, providing flexibility in how tools are obtained.
502-519: Comprehensive tool management workflow.The implementation creates a complete workflow for:
- Finding containers with the right tools
- Identifying the most efficient tool for the specific block
- Handling cases where suitable tools aren't available
- Setting the correct hotbar index in the context
This is a significant improvement that will make block breaking more efficient and reliable.
common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (9)
74-74: Improved constructor with HotbarConfig parameter.Adding the
hotbarparameter to the constructor allows for better control over hotbar management, making the class more configurable.
84-86: Breaking state tracking.New fields for tracking breaking state provide the foundation for more sophisticated breaking mechanics, including progressive breaking and sound management.
113-113: Hotbar validation before interaction.Checking that the hotbar request is completed before proceeding with the interaction ensures that the correct tool is selected, improving reliability.
115-118: Encapsulated block placement logic.The code now properly checks for sneaking state and delegates to a dedicated
placeBlockmethod, which improves code organization and readability.
122-122: Modern break request registration.Using
BreakManager.registerRequestcentralizes break request handling, which is a better approach for managing breaking operations across the application.
175-180: Configurable multi-breaking capability.The system now supports breaking multiple blocks per tick based on configuration, which can significantly improve efficiency for certain scenarios.
186-190: Improved result filtering.The new filtering mechanism ensures that results aren't blocked by pending interactions and are properly sorted, which will lead to more predictable and efficient building behavior.
222-227: Comprehensive break request registration.The break request now includes more context, such as the potential next break context, which can help with optimizing sequences of break operations.
283-299: Well-structured placeBlock method.The
placeBlockmethod properly handles:
- Block interaction through the interaction service
- Hand swinging based on result and configuration
- Equipment progress reset for creative mode
- Error reporting
This encapsulation is cleaner and more maintainable than inline code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
138-145: Consolidate repeated creative-mode checks.
Creative-mode checks appear both here and inattackBlock(). A separate utility method or early return for creative mode could unify the logic, reducing duplication and potential drift.
156-158: Remove block-breaking texture if the block becomes air.
When the block turns to air, the code nullifies theBreakInfobut does not reset the breaking texture (e.g., by callingsetBreakingTextureStage(info, -1)). This can leave clients displaying a cracked block texture until the next frame update.
281-299: Consider handling drops or custom block-breaking side effects.
onBreak(...)andonBroken(...)handle some logic, but item drops or special side effects may need explicit handling. If this is managed elsewhere, confirm that it’s invoked reliably.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (4)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (4)
65-68: Verify potential skipping of secondary breaking info.
Wheninteraction.blockBreakingCooldown > 0, the loopbreaks immediately. This means other slots inbreakingInfoswon’t be processed in the same tick. Double-check if this behavior is intentional or if eachBreakInfoshould decrement or be updated regardless.
125-132: Implementation aligns well with superclass override.
TheupdateRequestoverride properly callssuperand updates the localbuildConfig, ensuring your break settings remain in sync.
245-259: Confirm packet sequencing and server rate limits.
The repeated start/stop break packet sequence (incrementing thependingUpdateManagermultiple times) may risk sending rapid-fire packets. Ensure that these packet bursts comply with server rate limits and do not cause synchronization issues.
313-356: Structuring ofBreakInfosubclasses is well-organized.
Your use ofBreakInfohierarchy andnullify()overrides for primary vs. secondary references is a neat approach to manage different breaking scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (5)
90-111: Ensure correct behavior for block update rejection.
In lines 108-110, the code logs a warning when the block update is rejected becausenewStatedoesn't match thetargetState. Verify whether gracefully ignoring this mismatch (by returning early) fully covers edge cases. If the mismatch suggests a partial or out-of-sync break, consider whether further handling or rollback is warranted.
132-143: Optimize block-breaking checks for creative mode.
Lines 136-143 handle creative mode separately and send a packet to start destroying the block. If the underlying logic is similar for creative vs. survival, consider unifying some of the code paths to reduce duplication. Otherwise, clarify in code comments how creative mode should differ from survival or adventure mode to avoid confusion.
167-182: Throttle audio playback more precisely.
Right now,info.soundsCooldownincrements by 1.0f each tick, and a sound is played every timeinfo.soundsCooldown % 4.0f == 0.0f. On slower or inconsistent tick rates, this might result in staggered or spammy audio. Consider using a time-based check (e.g., a fixed duration in milliseconds) and updating it with delta-time for more consistent pacing across frames.
279-298: Check if frequent block state updates may impact performance.
Line 292 sets a new block state and triggers block break logic. If you’re calling this frequently (especially with repeated breaks), it may cause performance bottlenecks. Ensure that repeated block updates are necessary and that the default flags11(neighbor notifications & block updates) are appropriate.
311-354: Improve clarity in nested classesPrimaryBreakInfoandSecondaryBreakInfo.
These nested classes overridegetBreakThreshold()andnullify(), which is helpful, but the difference in behaviors is subtle (e.g.,SecondaryBreakInfohas a fixed threshold of 1.0f). Consider documenting, in code, the use-cases for each class to improve maintainability and better convey their differences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (3)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
46-54: Consider concurrency concerns for shared mutable state.
BreakManageris defined as a Singleton (object) with mutable properties likeprimaryBreakingInfo,secondaryBreakingInfo, andbreakingInfos. If these are accessed or modified across multiple threads, there is a potential for data races or inconsistent states. In many game contexts, the logic often runs in a single thread, but if there's any asynchronous usage, consider adding synchronization or clarifying the single-threaded assumptions.
61-88: Guard the reversed iteration and block breaking cooldown logic more robustly.
When iterating overbreakingInfos.reversed(), the code checks ifinteraction.blockBreakingCooldown > 0, then decrements the cooldown and breaks out of the loop. If there's a scenario where bothbreakingInfosindices are non-null, you'll only handle the first reversed entry. Confirm that this earlybreakis intentional, and consider whether bothprimaryBreakingInfoandsecondaryBreakingInfoshould still be processed in the same tick if possible.
243-257: Evaluate duplication in packet sending for BreakMode.Packet.
Lines 243-251 send multiple start/stop packets repeatedly, whereas lines 252-257 handle a simplified approach for normal (non-packet) flow. Carefully confirm each packet sequence is required to ensure correct block-breaking synchronization, as repeated STOP/START calls could flood the network if used excessively or incorrectly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
common/src/main/kotlin/com/lambda/module/modules/player/PacketMineRewrite.kt (1)
39-40: Consider restricting member visibility.
Exposingblueprintandtaskasvarproperties could lead to unintended external modifications. Make them private or provide controlled access if they must be read or manipulated externally.- var blueprint: StaticBlueprint? = null - var task: BuildTask? = null + private var blueprint: StaticBlueprint? = null + private var task: BuildTask? = nullcommon/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
47-47: Consider concurrency or reactivity when updatingbuildConfig.
BecausebuildConfigcan be reassigned at runtime (var buildConfig: BuildConfig), verify that other components depending on it react properly to changes without causing race conditions or stale reads.
62-75: Clarify the flow for multiple request contexts.
Inside the loop, oncecanAccept(requestCtx)fails for a single context, the remainder is simply skipped (return@forEach). If the intention is to reject only that context but allow others, consider continuing the loop instead of returning early.- if (!canAccept(requestCtx)) return@forEach + if (!canAccept(requestCtx)) { + // Skip only this request context + return@forEach + }
146-150: Consolidate block-attack failure handling.
WhenattackBlock(info)returns false,info.nullify()is called. If the code is expanded or refactored in the future, consider centralizing this failure path to avoid repeating nullification logic in multiple places.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/player/PacketMineRewrite.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (8)
common/src/main/kotlin/com/lambda/module/modules/player/PacketMineRewrite.kt (3)
1-16: No concerns with the license header.
18-36: Module declaration looks good.
The class signature and module tag usage are straightforward and clear.
42-57:Details
❓ Verification inconclusive
Verify event cancellation and concurrency behavior.
Cancelling thePlayerEvent.Attack.Blockevent outright could interfere with other modules or concurrent processes. Ensure that this is the desired behavior and that multiple calls to the listener don't lead to race conditions withtaskorblueprint.
🏁 Script executed:
#!/bin/bash # Description: Search for references to PacketMineRewrite to see if there's concurrency or other modules interfering. # The goal is to identify any potential race condition or conflicting event cancellation strategy. rg -A5 'PacketMineRewrite' rg 'blueprint' rg 'task'Length of output: 28975
Action Required: Verify Concurrency Safety and Event Cancellation Strategy
Based on the code search results:
- The cancellation pattern (using
it.cancel()andtask?.cancel()) is consistent with similar modules (e.g., inPacketMine.kt,Nuker.kt) where task management is performed inline.- No conflicting modules or alternate management of
blueprintandtaskwere found in the repository search.However, please verify that:
- The event system reliably dispatches
PlayerEvent.Attack.Blockon a single thread or includes its own synchronization.- Repeated invocations of the listener (and thus rapid reassignments of
blueprintandtask) do not introduce race conditions, particularly if other parts of the system manipulate similar state.It’s advisable to double-check these assumptions to ensure that the cancellation behavior won’t inadvertently affect concurrent processes.
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (5)
56-58: Double-check thread-safety forLimitedDecayQueue.
If this queue is accessed across multiple threads (e.g., during events vs. game ticks), ensure that it’s either confined to a single thread or adequately synchronized.
67-73: Ensure secondary break logic always aligns with user intent.
You allow a secondary break ifdoubleBreakis enabled and the primary break wasn't started with a secondary. Confirm that this meets all desired multi-break scenarios, especially if multiple contexts arrive at once.
91-117: Potential race in handling block updates and pending interactions.
WhenWorldEvent.BlockUpdate.Servertriggers, the code removes theBreakInfofrompendingInteractionsand proceeds to break the block. If the queue is modified elsewhere in parallel (e.g., from ticks), there might be edge cases. Check that listeners are single-threaded or lock-protected.
277-277: Revisit theAwaitThenBreakflow.
ForBreakConfirmationMode.AwaitThenBreak, the block isn’t actually destroyed until a matchingWorldEvent.BlockUpdate.Serverarrives. If the server never updates (e.g., no state change), the block might remain in pending forever. Consider a fallback or timeout logic for this mode.
311-354: Overall class structure is well organized.
The nestedPrimaryBreakInfoandSecondaryBreakInfoclasses neatly differentiate standard vs. short threshold break flows, improving clarity and reducing duplication.
| block.onBreak(world, ctx.expectedPos, ctx.checkedState, player) | ||
| val fluidState = fluidState(ctx.expectedPos) | ||
| val setState = world.setBlockState(ctx.expectedPos, fluidState.blockState, 11) | ||
| if (setState) block.onBroken(world, ctx.expectedPos, ctx.checkedState) |
There was a problem hiding this comment.
Validate return value of world.setBlockState(...).
When setBlockState returns false, the old block remains intact. If this occurs, the code still proceeds with block.onBroken(...). Confirm this mismatch doesn’t lead to unintended states or partial break.
+ if (!setState) {
+ this@BreakManager.warn("Failed to set new block state for ${ctx.expectedPos}")
+ return false
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (setState) block.onBroken(world, ctx.expectedPos, ctx.checkedState) | |
| if (!setState) { | |
| this@BreakManager.warn("Failed to set new block state for ${ctx.expectedPos}") | |
| return false | |
| } | |
| block.onBroken(world, ctx.expectedPos, ctx.checkedState) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (2)
246-253: Multiple start/stop packets
This approach sends multiplestartBreakPacket/stopBreakPacketcalls in rapid succession with incremented sequence values. Although it can work, it looks somewhat redundant and may lead to confusion or extraneous network traffic. If the intention is to force multiple block state updates, consider adding a clarifying comment or consolidating the logic.
313-359: Refine threshold logic inBreakInfosubclasses
PrimaryBreakInfousesbuildConfig.breakSettings.breakThreshold, whereasSecondaryBreakInfooverrides it with1.0f. This is functional but might be error-prone if more break info types emerge. You could unify or centralize threshold logic in a single configurable source and add param(s) for specialized overrides to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (3)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
79-83: Potential unintended behavior in cooldown check
Currently, ifinteraction.blockBreakingCooldown > 0, the loop breaks immediately, potentially skipping the update for other break info items inbreakingInfos. Verify whether it’s intentional that only the first item in the reversed list is evaluated in each tick.
139-146: Confirm survival-mode parity
When game mode is creative (lines 139-146), the code bypasses much of the normal logic by sending only one packet for block destruction. Ensure that a similar final block removal check is handled properly in survival mode, especially if the server or mods rely on additional block break updates.
294-295: Validate return value ofworld.setBlockState(...)
WhensetStateis false, old block data remains, but the code only logs success states. Confirm this won’t result in partial breaks or desync.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (3)
84-87: Unused or partially used fields.
breaking,breakingTicks, andsoundsCooldownare introduced here, but they appear underutilized or not referenced later. If future logic is pending, consider marking them with a TODO or removing them to keep the codebase clean.
97-104: Commented-out method content.
onCancel()has relevant logic entirely commented out. Either remove the commented code or implement a final version to avoid confusion.
219-225: Creating & registering a dual break request.
The code safely usesgetOrNull(1)for the second context. If planning to break multiple blocks in sequence, consider clarifying the logic to avoid confusion regarding concurrency or partial completion of the second break.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (8)
common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (8)
47-50: No issues with these newly added imports.
They appear to be correctly referenced by subsequent changes for breaking and hotbar logic.
63-63: Import introduction is valid.
ImportingHandis necessary for usage withinplaceBlockand related logic.
74-74: Constructor enhancement for hotbar configuration.
Addinghotbar: HotbarConfigextends control over hotbar usage. Ensure all references to the new parameter are updated in any calling code.
111-117: Conditional hotbar request and place logic.
The early return if the hotbar request is not done is correct for preventing race conditions. However, ensure any errors or rejections fromhotbar.request(...)are logged or handled for better diagnostics.
120-120: Break request registration.
Registers the break request correctly usingBreakManager. Consider confirming thread-safety if used with asynchronous tasks.
173-177: Multiple break actions per tick.
The logic for handling multiple breaks per tick using.take(build.breakSettings.breaksPerTick)is clear. Confirm whether partial breaks might occur if there's less than the configured break limit.
184-187: Filtering out blocked results.
Excluding positions inpendingInteractionsbefore sorting prevents duplicate interactions. This approach seems reliable and efficient.
281-297: Block placement flow.
TheplaceBlockfunction elegantly handles success vs. failure cases:
- Calls
interactBlockand checks acceptance.- Swings the hand and resets equip progress if needed.
- Logs a warning if the operation fails.
Implementation looks solid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (2)
46-53: Clarify the purpose of storing multiple BreakInfo entries.
Storing two break info objects (primary and secondary) in an array suggests multi-break support, but it's not immediately clear how these two entries are orchestrated. Consider adding doc comments or clarifying variable names (e.g.breakingInfos[PRIMARY],breakingInfos[SECONDARY]) to improve maintainability.
320-366: Consider unifying logic in BreakInfo sub-classes.
Primary and secondary break info classes differ only slightly, largely by break threshold and how they nullify themselves. Consider extracting shared logic or switching to parameters indicating whether it’s the primary or secondary info, to reduce code duplication.common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (4)
47-50: Document new break management dependencies.
IntroducingBreakManagerandBreakRequestdependencies here can affect how tasks handle block breaking. Add doc comments on how these new references integrate into the building workflow.
96-102: Remove or clarify commented-out cancellation code.
The commented code inonCancelhints at potential unsetting of block-breaking textures. If not needed, remove it for cleanliness; otherwise, consider implementing the functionality with proper logic.
175-180: Re-evaluate breaksPerTick usage.
The code extracts a subset of break results for instant breaks, limited bybreakSettings.breaksPerTick. Consider how the number of potential break results is handled across multiple ticks, especially when large structures require mass block breaking.
239-245: Rotate only when necessary.
This code explicitly checksshouldRotate(build)before requesting rotation. Consider short-circuiting any repeated calls to rotation if the rotation is already requested or completed.common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt (3)
21-21: Add doc comment for InventoryConfig usage.
Explain in doc comments howinventoryConfiginfluences the breaking logic (e.g., whether it applies any special constraints or item usage for block-breaking).
26-26: Document rotationConfig’s role.
rotationConfigpresumably configures how rotation is calculated for block breaking. Providing clarifying doc comments or usage notes would help maintainers.
78-78: Align condition with build config usage.
shouldRotate(config: BuildConfig) = config.breakSettings.rotateForBreakis consistent, but ifBreakContextcan also have its own rotation policy, ensure they remain in sync, or unify them to reduce confusion.common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt (1)
428-428: Consider implementing the TODO comment.The TODO comment suggests a need to rewrite the rotation request system to better support all simulator features.
Since this PR is focused on enhancing building with more granular material control, improving the rotation system in a follow-up PR would make sense to maintain code quality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt(3 hunks)common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt(10 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt(10 hunks)
🔇 Additional comments (22)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
67-76: Ensure correct handling of doubleBreak logic.
When double-breaking is enabled, the code sets a secondary break info if primary break info is already in progress. Confirm that simultaneous block-breaking operations do not interfere with one another or lead to race conditions if user interactions happen rapidly.
80-94: Check blockBreakingCooldown decrement logic when multiple breaks are queued.
The loop stops processing further break infos once a break cooldown is found, which may delay secondary breaks. Verify that this is intentional and won't cause starved updates of one break info while the other is paused.
301-303: Past comment about partial states still applies.
This line checkssetStatebefore callingblock.onBroken, but does nothing ifsetStateisfalse. A partial break might occur if setting the block state fails yet the block-breaking flow is partially completed.common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (5)
84-84: Verify null checks on breakRequest usage.
breakRequestis a mutable var. Be sure any references to it confirm non-null status or handle the null case gracefully to prevent NPE.
107-112: Confirm concurrency or rate-limiting for instantBreaks.
Adding items toinstantBreaksmid-tick callsBreakManager.registerRequest(...). For repeated calls, ensure no undesired concurrency effect or that the queue won't overflow in short bursts.
117-118: Check hotbar request ordering before block placement.
The code waits forhotbar.request(...)to finish before placing blocks. Confirm that partial handoff or repeated requests do not cause unexpected states if usage overlaps.
222-229: Ensure break requests don't overshadow other tasks.
When registering a newBreakRequest, you override the localbreakRequestfield. Confirm that multiple break tasks won't conflict if the user toggles them repeatedly (e.g., only one break request is tracked at once).
261-267: Validate block update acceptance logic.
OnWorldEvent.BlockUpdate.Server, removing interactions frompendingInteractionsis straightforward, but ensure no race conditions occur when the block changes state in quick succession.common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt (1)
85-103: Consider shared error handling for break packets.
ThestartBreakPacket,stopBreakPacket, andabortBreakPacketmethods all rely onbreakPacket(...). If a network failure or server rejection occurs, ensure upper layers handle partial breaks or unexpected states gracefully.common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt (13)
32-36: Improved inventory and material handling with new imports.These new imports introduce more sophisticated material selection mechanisms, leveraging container and stack selection capabilities to better manage tool and material access.
119-119: Updated reference to use breakSettings instead of direct property.The code now references settings through the more organized
build.breakSettings.ignoredBlocksstructure rather than accessing the property directly, which aligns with the enhanced configuration structure introduced in this PR.
290-290: Improved detection of interactive blocks.Using
hitBlock::class in BlockUtils.interactionBlocksprovides better type-safe handling of blocks that require sneaking for interaction.
302-302: Changed from hand to hotbar index in context.The PlaceContext now uses the player's selected slot index instead of hardcoding a hand, providing a more flexible approach for tool and item selection.
317-317: Enhanced item selection with dedicated selector method.Using
optimalStack.item.select()leverages the new selection system to create a proper item selector, improving code clarity and consistency with the material selection system.
340-340: Improved breakSettings reference for weak blocks.Similar to the ignoredBlocks change, this properly uses the reorganized configuration structure through
build.breakSettings.breakWeakBlocks.
407-417: Enhanced BreakContext with more comprehensive parameters.The BreakContext now includes additional parameters like hotbar index, build configuration, rotation and inventory settings, providing more context for break operations and better supporting the simulation needs.
457-466: Improved hit selection and context creation.This section now properly extracts hit information and creates a more complete BreakContext with all necessary parameters for break operations.
467-470: Clean handling of creative mode breaking.The code now cleanly handles creative mode as a special case that doesn't require tool selection, improving readability.
472-487: Comprehensive tool selection and fallback logic.This section intelligently handles cases where appropriate tools aren't available:
- Checks if any tools are suitable
- Verifies if current items can mine the block
- Provides a fallback for cases where no ideal tool exists
This significantly improves the robustness of the breaking simulation.
489-502: Enhanced tool selection with enchantment considerations.This implementation adds sophisticated tool selection based on configuration preferences:
- Can force Silk Touch tools when specified
- Can require Fortune enchantment with configurable minimum level
- Falls back to best available tools when enchantment requirements aren't specified
- Creates appropriate container selectors for hotbar and inventory
This provides much more granular control over material usage.
503-507: Improved tool availability verification.The code now properly checks if suitable tools are available in the player's inventory and hotbar, returning an appropriate error result when needed.
509-520: Optimized tool selection based on breaking speed.This sophisticated algorithm:
- Maps containers to their matching stacks
- Calculates breaking speed for each potential tool
- Selects the optimal tool based on maximum breaking speed
- Updates the hotbar index in the break context
This ensures the simulator always selects the most efficient tool for the job.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt (1)
26-35: Consider documenting theonBreakcallback and clarifying howdoneis always false.
TheonBreakcallback indicates an event-like mechanism, but it lacks explicit concurrency or usage documentation. Future maintainers might appreciate inline KDoc to explain when and by whomonBreakis invoked. Additionally,donealways returningfalsecould be confusing unless there's a specific design requirement. A brief comment or doc note can reduce any misunderstandings.common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt (2)
73-73: Conditional rotation logic is straightforward but might need more nuance.
override fun shouldRotate(config: BuildConfig) = config.breakSettings.rotateForBreaksimply delegates rotation decisions to a single config flag. If future conditions (like block type or distance) also matter, consider a more detailed check or additional configuration hooks.
80-97: Ensure robust error handling for block-breaking packets.
ThestartBreakPacket,stopBreakPacket,abortBreakPacket, and privatebreakPacketmethods systematically send block-destroying actions. Consider checking theconnectionstate (e.g., is it still open?) or catching any exceptions to prevent unhandled errors if the network state is invalid.common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (5)
91-98:onCancelis defined but currently commented out.
The placeholder code references a remarked-out block-breaking texture reset. If this logic is still needed, consider properly implementing it. If not, removing the commented lines avoids confusion.
163-172: Support for batching multiple breaks per tick.
AllowingbreakSettings.breaksPerTickto exceed 1 is a powerful feature. Be cautious with potential server anti-cheat triggers or performance spikes if values become too large.
223-229: Conditional skip forBreakContextincurrentInteraction.
The early return forBreakContextmight interrupt the normal item placement flow. Ensure you intentionally want to bypass the rest of this block for break interactions.
245-255: Pending interactions logic upon server block updates.
You remove contexts only if the new state matches the expected state. Any mismatch logs a warning. This approach is solid. Just confirm that legitimate partial updates don’t get stuck in pending.
271-287:placeBlockencapsulation is clear.
Extracting block placement logic into a dedicated function is good for readability. If a more detailed result or error handling is ever required, consider returning a status from this method instead of just logging.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
50-50: Consider concurrency implications of using a global object
BreakManageris declared as a top-level object, which can be accessed from anywhere and at any time. If there's any chance your environment is multi-threaded, ensure internal state (e.g.,breakingInfos) remains consistent. Otherwise, clarify that this code runs only in a single-threaded context.
63-99: Potential performance concerns in bulk operations
The logic processes all breaking contexts on every tick, sorting byinstantBreak. For large-scale usage with many blocks, consider a more selective or batched approach to avoid potential spikes.
358-413: Optional fallback when removing breaking texture
Currently,setBreakingTextureStageis called with stage-1to clear the texture during block destruction. If the call fails or if the environment doesn’t reset the texture, consider adding a fallback or forced refresh of the affected area.common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt (4)
32-36: Promote consistent naming for imports
The series ofselectContainer,select,selectStackimports reflect similar functionalities. Consider aligning their naming patterns or adding clarifying doc comments to improve maintainability.
170-220: Address TODO for sneaking and multiple hands
A “ToDo” comment indicates future work for handling each hand or sneaking. If that requirement is important, consider implementing it soon or tracking it via an issue to avoid forgetting.Do you want help drafting a design for multi-hand and sneaking usage?
316-319: Consider partial matching for different item variants
The code checksif (optimalStack.item != currentHandStack.item), which may fail when items share an ID but differ by minor data (e.g., NBT tags). If partial matching is acceptable, consider comparing item IDs or broader metadata for more flexibility.
469-484: Handle scenario when no valid tool is found
The code addsBreakResult.ItemCantMinefor each non-mining item in the loop, but eventually proceeds with a break. This might lead to item loss or partial breaks in some game rules. Revisit if you want to prevent or confirm the break action in such cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt(3 hunks)common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt(10 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt(1 hunks)common/src/main/kotlin/com/lambda/module/modules/player/PacketMineRewrite.kt(1 hunks)common/src/main/kotlin/com/lambda/task/Task.kt(1 hunks)common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/kotlin/com/lambda/module/modules/player/PacketMineRewrite.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (14)
common/src/main/kotlin/com/lambda/task/Task.kt (1)
169-169: Double-check the unconditional invocation ofonCancel()before state checks.
The newly introducedrunSafe { onCancel() }callsonCancel()unconditionally, even ifstatemight already beCOMPLETEDorCANCELLED. If youronCancel()logic assumes the task is still active, consider wrapping it in a condition or reordering the call to avoid potential inconsistent states.common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt (2)
43-43: Verify consistency ofhotbarIndexusage.
Previously, ahandproperty was used. Now,hotbarIndexreplaces it. Ensure all references and calling sites have been updated accordingly to avoid mismatched usage of a hand-based API in place of the new hotbar-based API.
75-78: Renderer usage appears consistent, but confirm side-exclusion in large-scale builds.
The call toexclude(result.side)helps highlight the relevant face. Confirm that partial side rendering is desired in all break scenarios (e.g., large or diagonally oriented structures).common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (6)
47-49: Imports for new request classes look good.
The added imports forBreakRequest,HotbarConfig, andHotbarRequestmatch the usage below.
63-63: Confirm single-hand usage.
Importingnet.minecraft.util.Handimplies the code only usesHand.MAIN_HAND. Verify if off-hand actions or multi-hand usage are needed.
73-73: Hotbar configuration injection is beneficial for portability.
Introducingprivate val hotbar: HotbarConfigin the constructor is a clean solution for managing hotbar interactions. Ensure other tasks requiring similar logic also adopt this pattern for consistency.
103-111: Requesting the hotbar slot before placing blocks is a good approach.
Blocking placement untilhotbar.request(HotbarRequest(context.hotbarIndex))completes ensures the correct slot is active. Double-check any race conditions if multiple tasks try to manage hotbar concurrently.
175-179: Filtering out blocked results is important.
By excluding positions inpendingInteractions, you avoid redundant interactions. Just ensure stale entries are cleaned up promptly so valid results are not filtered mistakenly.
210-215: Group break request creation uses up to two contexts.
Taking two break contexts for a singleBreakRequestcan be highly efficient. Confirm that partial completions (e.g., if one break fails) don’t leave the second context in an inconsistent state.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (4)
110-130: Verify correctness of block update handling
When aBlockUpdate.Serverevent is received, the code checks for exact target-state matches before proceeding with destruction or ignoring. If partial or intermediate states can occur in your environment, verify that skipping them doesn't cause unintended block states or missed updates.
185-260: Validate multiplayer synchronization
updateBlockBreakingProgresssends block-breaking packets to the server while also updating local progress. Ensure that the server’s authoritative state remains consistent if the client’s progression differs due to lag or packet loss. Additional server-side checks could prevent desync.
262-318: Ensure packet timings do not cause server confusion
Rapidly sending multipleSTART_DESTROY_BLOCKandSTOP_DESTROY_BLOCKpackets inattackBlockcan lead to race conditions on the server. Confirm that the server is designed to handle these repeated start/stop events without unintended partial breaks.
350-351: Validate return value usage ofworld.setBlockState(...)
WhensetBlockStatereturns false, the block remains in its old state. This logic only proceeds toblock.onBroken(...)ifsetStateis true, but consider how partial breaks or unexpected states are handled ifsetStateis false.common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt (1)
487-490: Check block droprates for Silk Touch / Fortune logic
forceSilkTouchandforceFortunePickaxeare enforced, but some blocks do not benefit from either enchant, potentially wasting time or resources. Consider verifying if the block truly benefits from these enchants before forcing tool selection.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
common/src/main/kotlin/com/lambda/util/player/PlayerUtils.kt (1)
43-50: New client-side hand animation function looks good but could use documentationThe implementation correctly handles the player's hand swinging animation by managing the swing ticks, state, and preferred hand. This function is a valuable addition for providing visual feedback during gameplay actions.
Consider adding KDoc documentation to explain the function's purpose and emphasize that it's a client-side only operation.
+/** + * Makes the player swing their hand on the client-side only. + * This is useful for providing visual feedback during interactions without sending packets to the server. + * + * @param hand The hand to swing (MAIN_HAND or OFF_HAND) + */ fun SafeContext.swingHandClient(hand: Hand) { if (!player.handSwinging || player.handSwingTicks >= player.handSwingDuration / 2 || player.handSwingTicks < 0) { player.handSwingTicks = -1 player.handSwinging = true player.preferredHand = hand } }common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakConfig.kt (1)
24-44: Abstract properties look appropriate for config, but consider future-proofing with sealed interfaces or data objects.
Currently, theseabstract valproperties suffice for straightforward usage and extension. However, if your use cases expand, sealed interfaces or specialized config objects might provide clearer versioning, validation, and forward compatibility.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (2)
141-185: Double-break logic is intricate.
Managing primary/secondary assignment and reassigning theBreakInfocan be confusing. Consider extracting this block into smaller functions or adding clarifying comments. This reduces complexity and eases future modifications.
191-266:updateBlockBreakingProgressis large and could benefit from modularization.
Its responsibilities (creative vs. survival handling, sound/particle effects, texture updates) could be decomposed into smaller helpers, improving readability and maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/main/kotlin/com/lambda/config/groups/BreakSettings.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/util/player/PlayerUtils.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/kotlin/com/lambda/config/groups/BreakSettings.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (7)
common/src/main/kotlin/com/lambda/util/player/PlayerUtils.kt (2)
9-9: Import addition looks goodThe import for
Handfromnet.minecraft.utilis correctly added to support the new functionality.
44-49:Details
✅ Verification successful
Verify if server notification is needed for this hand swing action
The function name indicates this is a client-side operation, but if server synchronization is required for certain game mechanics, you may need to add a server notification.
Consider verifying if any game features that depend on hand swinging (like certain attack mechanics) would need server awareness of this action.
🏁 Script executed:
#!/bin/bash # Look for other similar hand swing functions that might notify the server rg -A 2 -B 2 "swingHand|swing.*Hand|Hand.*swing" --type kotlin --type javaLength of output: 12041
Design Confirmation: Client-Only Swing Does Not Require Server Notification
After reviewing similar hand swing functions across the codebase, it’s clear that the design deliberately separates client and server responsibilities. The
swingHandClientfunction only updates local swing state—server notifications are sent by other functions (e.g., viaconnection.sendPacket(HandSwingC2SPacket(...))in the server swing variant and in the switch-case ofBreakManager.kt). Please ensure that if a game mechanic does require server awareness of a swing action, the calling code uses the appropriate variant. Otherwise, the current implementation is correct.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakConfig.kt (2)
45-47: Consider validating registration outcomes.
requestInternaldelegates toBreakManager.registerRequest(this, request)without checking success or failure. If this method can fail or return a status, adding a check or logging could help diagnose request registration issues.
49-72: Enums are straightforward and self-documenting.
They capture well-distinguished modes for each configurable option.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
76-76: Confirm sorting behavior for instant breaks.
Currently,request.contextsis sorted ascending byinstantBreak, so non-instant contexts run first. If you want to prioritize instant breaks, consider reversing the sort to handle them first.
309-320: Ensure server/client synchronization when issuing multiple start/stop break packets.
WhenBreakMode.Packetis active, sending repeated block-breaking packets can lead to inconsistent partial break states if not carefully tracked on the server side.
357-357: ** Validate return value ofworld.setBlockState(...).**
WhensetBlockStatereturns false, the old block remains intact. Ensure skippingblock.onBroken(...)doesn’t yield partial or unintended states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (4)
227-242: Consider making the 4-tick sound interval configurable.Hardcoding
% 4.0fcan make future adjustments more difficult. Extracting it into a configuration property or constant would improve maintainability.
317-320: Replace magic number 0.7 with a named constant or config setting.Hardcoding 0.7 can reduce clarity. Converting it into a named field or a configurable property helps document its significance and facilitates fine-tuning later.
354-362: Consider logging a warning whensetBlockStatefails.If
setStateis false, the old block remains and you silently return. Logging a warning or handling partial failures can aid in debugging and ensure consistent game states.if (!setState) { + this@BreakManager.warn("Failed to set new block state for ${ctx.expectedPos}") + return false }
1-429: Add dedicated tests or integration scenarios for this file.This class orchestrates several complex interactions (tick processing, block state transitions, and event handling). Implementing thorough unit or integration tests will help ensure correctness over time.
Would you like me to generate test scaffolding to give you a head start on coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/resources/lambda.accesswidener(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/resources/lambda.accesswidener
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (2)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (2)
66-70: No issues found with the cooldown logic.This effectively prevents processing of new interactions during the cooldown period. If this aligns with your design, you’re good to proceed.
75-88: Ensure the sorting approach matches the intended behavior.Currently,
.sortedBy { it.instantBreak }placesfalsebeforetrue. If you intend to prioritize instant-break contexts first, consider.sortedByDescending { it.instantBreak }. Otherwise, verify that you prefer to handle non-instant breaks initially.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
1-16: Add test coverage to ensure reliability.
This file introduces sizable, complex block-breaking logic that could be prone to regressions if not properly tested. Consider adding unit or integration tests for the various break modes, double-break scenarios, and edge cases like border restrictions.I can help sketch out a starter suite of tests if you’d like!
149-173: Refactor the double-break logic for clarity.
The flow that shuffles break types betweenprimaryBreakingInfoandsecondaryBreakingInfois fairly intricate. Refactoring into a dedicated method or class could improve readability and maintainability.
308-321: Simplify repeated packet sequencing.
Multiple calls tostopBreakPacket/startBreakPacketplus manualincrementSequencecalls can be confusing. Consider consolidating these into a single helper method to reduce complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (1)
354-362: Verify handling whensetBlockStatereturns false.
If the call fails, the block may remain partially broken but the code returns false without further action. Confirm that upstream logic checks for this scenario and avoids inconsistent block states.
| breakingInfos.reversed().filterNotNull().forEach { info -> | ||
| if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@listen | ||
| updateBlockBreakingProgress(info, player.mainHandStack) | ||
| } |
There was a problem hiding this comment.
Fix unintentional early return from the entire listener.
Calling return@listen aborts processing of the entire tick event listener rather than skipping just the current BreakInfo. If your intent is to only skip this particular item, use return@forEach instead.
- if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@listen
+ if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@forEach📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| breakingInfos.reversed().filterNotNull().forEach { info -> | |
| if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@listen | |
| updateBlockBreakingProgress(info, player.mainHandStack) | |
| } | |
| breakingInfos.reversed().filterNotNull().forEach { info -> | |
| if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@forEach | |
| updateBlockBreakingProgress(info, player.mainHandStack) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (1)
65-102:⚠️ Potential issueFix unintentional early return from the entire listener.
Using
return@listenon line 99 exits the entire tick event listener rather than just skipping the current breaking info. This prevents processing of any other breaking operations until rotation is complete.- if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@listen + if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@forEach
🧹 Nitpick comments (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (1)
347-366: Handle setState failure case more robustly.When
setBlockStatereturns false, it means the operation failed, but the method still returns this value without any error handling or logging. Consider adding explicit error handling for this case.val setState = world.setBlockState(ctx.expectedPos, fluidState.blockState, 11) if (setState) block.onBroken(world, ctx.expectedPos, ctx.checkedState) + else { + this@BreakManager.warn("Failed to set new block state for ${ctx.expectedPos}") + return false + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (12)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (12)
1-51: Comprehensive design for managing block breaking operations.The extensive import list showcases an implementation that leverages game mechanics while maintaining separation of concerns and supporting both vanilla and custom interaction patterns.
52-64: Well-structured BreakManager foundation with excellent state management.The design using primary and secondary breaking info arrays provides clean support for concurrent breaking operations, while the decay queue with timeout logic elegantly handles pending interactions.
104-131: Clean rotation and block update handling with robust state tracking.The rotation handler and block update listener are well-implemented, with appropriate state transitions and validation. The conditional block destruction based on confirmation mode is particularly well-designed.
134-139: Good error reporting and validation.The
matchesTargetStatefunction properly validates block states and provides clear error messaging when mismatches occur, which will be invaluable for debugging.
141-188: Sophisticated context handling for primary and secondary breaking.The implementation effectively manages the interaction between primary and secondary breaking operations with clear state management. The
startedWithSecondaryflag handles edge cases well.
190-194: Robust validation for accepting new breaking contexts.The
canAcceptfunction performs thorough validation, ensuring that breaking operations don't overlap and that we're not trying to break air blocks.
195-270: Comprehensive block breaking progress handling with sound and particle effects.The implementation correctly handles creative mode, breaking progress calculation, sound effects, particles, and breaking texture updates. The code has good organization with appropriate conditional branches.
330-345: Clear separation of break confirmation modes.The
onBlockBreakmethod effectively handles the different confirmation modes with appropriate state transitions. This makes the code more maintainable and easier to extend with new confirmation modes in the future.
368-377: Excellent use of breaking texture stage management.The method properly handles both automatic stage calculation based on breaking progress and allows for explicit stage setting, providing flexibility while maintaining client-server consistency.
379-385: Flexible hand swinging implementation supporting multiple modes.The implementation handles different swing types (vanilla, server, client) which provides good flexibility for different usage scenarios.
387-432: Well-designed BreakInfo class with clear responsibility.The
BreakInfodata class encapsulates all necessary state for a breaking operation, and theBreakTypeenum provides a clean way to differentiate between primary and secondary breaking operations with polymorphic behavior.
272-328:Details
❓ Verification inconclusive
Consider adding sequence number validation for packet operations.
The packet-based breaking mode sends multiple packets in sequence, but there's no validation that these sequence numbers are processed correctly by the server.
🏁 Script executed:
#!/bin/bash # Search for places where sequence numbers are validated rg -A 3 "sequence" --exclude="BreakManager.kt" | grep -E "validate|verify|check" # Look for other places where packet sequences are handled rg -A 10 "PlayerActionC2SPacket" --exclude="BreakManager.kt"Length of output: 961
Sequence Validation Check Required
After re-verifying the codebase with updated search commands, there’s still no clear evidence that sequence numbers for packet operations are validated elsewhere. The logic inBreakManager.kt(lines 272–328) sends multiple sequence-based packets without checks to confirm proper server-side processing. Please review the packet handling in this file and any related modules to ensure that sequence numbers are validated, thereby preventing potential processing errors.
🏁 Script executed:
#!/bin/bash # Searching for sequence validations in Kotlin files using valid options. rg -A 3 "sequence" -t kotlin | grep -E "validate|verify|check" # Searching for packet operations involving PlayerActionC2SPacket in Kotlin files. rg -A 10 "PlayerActionC2SPacket" -t kotlinLength of output: 22364
Attention: Revisit and Validate Sequence Number Handling
The current implementation inBreakManager.kt(lines 272–328) sends multiple packets using sequence numbers without any evident validation checks to ensure these sequences are correctly processed by the server. Our updated searches did not reveal additional logic in the codebase that validates the sequence numbers across packet operations.Action Items:
- Review Packet Handling: Confirm whether sequence numbers are meant to be validated elsewhere.
- Implement Validation: If no validation exists, add appropriate checks to ensure that the sequence numbers are processed in the proper order.
Please verify these changes and assess their impact on related modules that handle similar packet operations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (2)
98-101:⚠️ Potential issueFix unintentional early return from the entire listener.
Calling
return@listenaborts processing of the entire tick event listener rather than skipping just the currentBreakInfo. If your intent is to only skip this particular item, usereturn@forEachinstead.- if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@listen + if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@forEach
342-361:⚠️ Potential issueValidate return value of
world.setBlockState(...).When
setBlockStatereturns false, the old block remains intact. If this occurs, the code still proceeds withblock.onBroken(...). Add validation to handle this case properly.val setState = world.setBlockState(ctx.expectedPos, fluidState.blockState, 11) -if (setState) block.onBroken(world, ctx.expectedPos, ctx.checkedState) +if (setState) { + block.onBroken(world, ctx.expectedPos, ctx.checkedState) +} else { + this@BreakManager.warn("Failed to set new block state for ${ctx.expectedPos}") + return false +}
🧹 Nitpick comments (2)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (2)
112-131: Consider refactoring nested block update handling for better readability.The block update event handler contains nested lambdas and complex nullable handling. Consider extracting helper methods to improve readability and maintainability.
listen<WorldEvent.BlockUpdate.Server> { event -> - pendingInteractions - .firstOrNull { it.context.expectedPos == event.pos } - ?.let { pending -> - pendingInteractions.remove(pending) - if (!matchesTargetState(event.pos, pending.context.targetState, event.newState)) return@listen - if (pending.breakConfig.breakConfirmation == BreakConfirmationMode.AwaitThenBreak) - destroyBlock(pending) - pending.onBreak() - } - ?: breakingInfos - .filterNotNull() - .firstOrNull { it.context.expectedPos == event.pos } - ?.let { info -> - if (!matchesTargetState(event.pos, info.context.targetState, event.newState)) return@listen - info.nullify() - destroyBlock(info) - info.onBreak() - } + handlePendingInteraction(event) ?: handleActiveBreakInfo(event) } + private fun SafeContext.handlePendingInteraction(event: WorldEvent.BlockUpdate.Server): Boolean { + val pending = pendingInteractions.firstOrNull { it.context.expectedPos == event.pos } ?: return false + pendingInteractions.remove(pending) + if (!matchesTargetState(event.pos, pending.context.targetState, event.newState)) return true + if (pending.breakConfig.breakConfirmation == BreakConfirmationMode.AwaitThenBreak) { + destroyBlock(pending) + } + pending.onBreak() + return true + } + + private fun SafeContext.handleActiveBreakInfo(event: WorldEvent.BlockUpdate.Server) { + breakingInfos + .filterNotNull() + .firstOrNull { it.context.expectedPos == event.pos } + ?.let { info -> + if (!matchesTargetState(event.pos, info.context.targetState, event.newState)) return + info.nullify() + destroyBlock(info) + info.onBreak() + } + }
307-320: Add comments explaining different break mode packet behaviors.The packet sending logic differs based on
breakMode. Consider adding comments to explain the purpose and behavior of each mode to improve code maintainability.if (info.breakConfig.breakMode == BreakMode.Packet) { + // Packet mode sends start-stop-start sequence for instant breaking attempt ctx.stopBreakPacket(sequence, connection) ctx.startBreakPacket(sequence + 1, connection) ctx.stopBreakPacket(sequence + 1, connection) repeat(2) { pendingUpdateManager.incrementSequence() } } else { + // Normal mode sends start packet and conditionally stop based on breaking progress ctx.startBreakPacket(sequence, connection) if (breakingDelta < 1 && (breakingDelta >= 0.7 || info.breakConfig.doubleBreak)) { + // Send stop packet if we're close to breaking or using double break mode ctx.stopBreakPacket(sequence + 1, connection) pendingUpdateManager.incrementSequence() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (4)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (4)
52-64: Well-structured object initialization with clear state management.The
BreakManagerimplementation provides a clean approach to managing break state with separate primary and secondary breaking information, and a queue for pending interactions with timeout handling. The use of a limited decay queue is a good pattern for managing interactions that may never receive confirmation.
190-265: Well-implemented block breaking progress logic with comprehensive handling.The
updateBlockBreakingProgressmethod handles different game modes, progressive breaking, sounds, particles, and texture updates in a clear and structured way. The code correctly handles different states of the breaking process and applies the appropriate visual and audio feedback.
382-410: Well-designed BreakInfo class with clear responsibilities.The
BreakInfodata class keeps track of all the necessary state for a break operation, including context, type, configuration, and progress information. The methods for calculating break texture progress and thresholds are also well-implemented.
412-430: Effective use of enum class with associated functionality.The
BreakTypeenum provides a clean way to distinguish between primary, secondary, and null break types, along with methods to handle type-specific behaviors like threshold calculation and nullification. This is a good use of Kotlin's enum class features.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (3)
91-98: Remove or clarify the commented-out block inonCancel().
Leaving large commented blocks can clutter the code. Consider adding a TODO or removing these lines if the functionality is not planned for reactivation.
210-215: Potential duplication between this and the instant-break logic.
Here you take two break contexts fromresultsNotBlocked, while earlier code in lines 163–174 processes multiple instant breaks. If both paths are intended, ensure they target distinct scenarios or unify them to avoid confusion.
238-243: Commented-out client block update listener.
Similar to theonCancelblock, this listener code is commented out. Consider removing it or adding a clear TODO annotation if you plan to reintroduce this code later.common/src/main/kotlin/com/lambda/util/BlockUtils.kt (3)
100-105: Consider renamingvecOfand clarifyingoffsetusage.
These new extension properties and utility methods are helpful, butvecOfmight be more descriptive if renamed to something likedirectionCenterPosorcenterOffsetForDirection. Similarly,offsetonly applies horizontal movement. Renaming it (e.g.,offsetHorizontal) could enhance clarity.
243-246: Avoid duplicating logic and handle zero-delta similarly.
This overload largely duplicates the logic from the otherinstantBreakablemethod, including a potential division-by-zero risk. Consider extracting the shared logic or unifying them:fun SafeContext.instantBreakable(blockState: BlockState, blockPos: BlockPos, item: ItemStack, breakThreshold: Float): Boolean { - val ticksNeeded = 1 / (blockState.calcItemBlockBreakingDelta(player, world, blockPos, item) / breakThreshold) - return (ticksNeeded <= 1 && ticksNeeded != 0f) || player.isCreative + val delta = blockState.calcItemBlockBreakingDelta(player, world, blockPos, item) + if (delta <= 0f) return false + val ticksNeeded = breakThreshold / delta + return ticksNeeded <= 1 || player.isCreative }
248-262: Document or name the 30/100 harvest multipliers for clarity.
These multipliers significantly affect block-breaking speed. Consider assigning them to well-named constants or adding comments to explain their rationale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt(3 hunks)common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt(10 hunks)common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt(9 hunks)common/src/main/kotlin/com/lambda/util/BlockUtils.kt(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/kotlin/com/lambda/interaction/construction/context/BreakContext.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (31)
common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (10)
47-49: No issues with new imports.
These imports forBreakRequest,HotbarConfig, andHotbarRequestare straightforward and neatly grouped, matching their usage in the code. No concerns here.
73-73: Newhotbarparameter seems well-integrated.
Adding aHotbarConfigparameter with a default toTaskFlowModule.hotbarcleanly extends configuration capabilities without breaking existing call sites. This is a neat approach.
105-111: Hotbar request before block placement looks good.
Requiring the hotbar request to succeed prior to placing the block ensures the correct item is in hand. The early return pattern is straightforward and prevents half-finished interactions.
163-168: Efficiently batching break actions.
Allowing up tobreaksPerTickbreak operations and filtering relevant break results is a clear and maintainable approach. It aligns well with throughput control for block-breaking logic.
169-174: All-or-nothing approach with break requests is appropriate.
By bundling all identified break contexts into a singleBreakRequest, you ensure consistent handling of multiple instant break opportunities while preserving a single hotbar check.
176-179: Filtering out blocked results for bestResult is sensible.
Excluding pending or blocked interactions avoids redundant operations and helps maintain a consistent build flow. Sorting and picking the first feasible result is a clean solution.
224-229: Skipping rotation forBreakContext.
The explicit check forBreakContextbefore rotating is consistent with the notion that breaking might not require a rotation. This conditional separation is clear and avoids unnecessary rotation requests.
246-252: Gracefully handling finalized block updates.
Removing the matching context frompendingInteractionsonce certain states are validated is an effective approach, preventing stale interactions from persisting.
266-268: Selective item collection logic is sensible.
Checking the block type matches the drop ensures only relevant drops are collected. This helps maintain an inventory free of unwanted items.
272-288: Block placement routine is well-structured.
Invokinginteraction.interactBlock()and conditionally swinging the hand is logical. Resetting equip progress in creative mode is a thoughtful touch. The fall-back warning is helpful for debugging failed interactions.common/src/main/kotlin/com/lambda/interaction/construction/simulation/BuildSimulator.kt (17)
32-37: Introduction of fine-grained material selection systemThese new imports introduce DSL-style selection mechanisms for materials and containers, which seems to be part of a larger refactoring effort to improve the modularity and expressiveness of the material selection system.
The introduction of these selection-focused classes aligns well with clean architecture principles, providing a more declarative API for material selection.
55-55: Improved tool selection strategyThis import suggests a replacement of the previous
findBestAvailableToolmethod with a new approach that likely evaluates tools based on more sophisticated criteria.Centralizing tool selection logic in a utility class improves maintainability and makes the code more reusable across the codebase.
62-62: Added enchantment support in tool selectionThe addition of the Enchantments import indicates that the tool selection process now considers tool enchantments like Silk Touch and Fortune.
This is a valuable improvement as it allows for more strategic tool selection based on enchantments, which directly affects resource gathering efficiency.
119-119: Updated reference to consolidated break settingsThe code now references
build.breakSettings.ignoredBlocksinstead of what was likely a direct reference to ignoredBlocks.Consolidating related settings under a single namespace improves code organization and makes configuration more intuitive.
302-302: Switched from hand-based to slot-based interactionChanged from likely using hand references to using the hotbar selected slot, aligning with a more unified slot-based approach.
Using the selected slot provides better consistency with Minecraft's inventory system and allows for more precise tool management.
317-317: Using new DSL for item selectionThe code now uses
select()extension function to create a selection for the item, improving code readability.This change leverages the new selection DSL and makes the intention clearer while maintaining the same functionality.
340-340: Updated reference to break settingsSimilar to line 119, this line has been updated to reference the consolidated break settings configuration.
Consistently accessing configuration through the consolidated
breakSettingsimproves maintainability.
407-414: Improved BreakContext creationThe BreakContext constructor parameters have been updated to include the selected slot and use break settings threshold.
The updated parameters make the context more complete and align with the new slot-based approach.
454-463: Simplified BreakContext creation logicThis block has been refactored to be more concise and to use the selected slot instead of hand references.
The refactored code is cleaner and more aligned with the new inventory interaction approach.
469-469: New tool selection approachThe function now uses
findBestToolsForBreakingto determine the optimal tools based on block state.This change provides a more robust way to find the best tools, considering all aspects of tool effectiveness for breaking specific blocks.
472-484: Improved handling of missing toolsThis section adds better handling for cases where no suitable tool is available, checking if the current item can mine the block.
The additional checks provide more graceful fallbacks when ideal tools aren't available, improving user experience.
486-492: Enchantment-aware tool selectionThe code now considers enchantments like Silk Touch and Fortune when selecting tools, based on user preferences.
This is a significant improvement that allows users to prefer tools with specific enchantments, leading to better resource gathering results.
493-498: Container selection strategiesImplements two container selection strategies: a silent swap for hotbar items and a full selection that respects user inventory preferences.
The dual selection strategy provides flexibility, allowing for optimized item selection while respecting user constraints.
500-504: Tool availability verificationChecks if the desired tool is available in the allowed containers before proceeding.
This verification step ensures that the system doesn't attempt operations with unavailable tools, preventing potential issues.
506-511: Speed-based tool selection optimizationSelects the optimal tool based on block breaking speed, providing the best performance for the task.
This optimization ensures that not only is a compatible tool selected, but specifically the fastest one available, improving efficiency.
515-517: BreakContext finalization with optimal toolUpdates the break context with the selected tool's hotbar index and recalculates if the break is instant with the chosen tool.
This final step ensures that the break operation uses the optimal tool and correctly determines if instant breaking is possible.
290-290:Details
✅ Verification successful
Looking up interaction blocks using class reference
This line uses
hitBlock::classto check if the block is in theinteractionBlockscollection.
🏁 Script executed:
#!/bin/bash # Check how interactionBlocks is defined rg -A 5 "interactionBlocks" --type kotlinLength of output: 1377
Verified: Class Reference Lookup is Correct
The usage of
hitBlock::class in BlockUtils.interactionBlocksinBuildSimulator.ktis appropriate. TheinteractionBlocksset inBlockUtils.ktis defined with the expected KClass references (e.g.,AbstractCauldronBlock::class,AnvilBlock::class, etc.), so the check performed in the simulator is valid and functions as intended.common/src/main/kotlin/com/lambda/util/BlockUtils.kt (4)
26-27: New imports for bamboo blocks, enchantments, status effects, and item references look fine.Also applies to: 85-89, 93-95, 97-97
165-165: Renaming tointeractionBlocksimproves clarity.
The new name better conveys that this set holds blocks, not classes.
264-266: Implementation ofcanHarvestis straightforward.
The logic aligns with the usual check for tool requirements.
267-301: Mining speed calculation is consistent with vanilla mechanics.
The method accurately accounts for Efficiency, Haste, Mining Fatigue, underwater penalties, and airborne penalties.
| fun SafeContext.instantBreakable(blockState: BlockState, blockPos: BlockPos, breakThreshold: Float): Boolean { | ||
| val ticksNeeded = 1 / (blockState.calcBlockBreakingDelta(player, world, blockPos) / breakThreshold) | ||
| return (ticksNeeded <= 1 && ticksNeeded != 0f) || player.isCreative | ||
| } |
There was a problem hiding this comment.
Guard against dividing by zero for unbreakable blocks.
If calcBlockBreakingDelta returns 0, 1 / (value / breakThreshold) produces infinity. Consider this fix to handle unbreakable or zero-delta cases gracefully:
fun SafeContext.instantBreakable(blockState: BlockState, blockPos: BlockPos, breakThreshold: Float): Boolean {
- val ticksNeeded = 1 / (blockState.calcBlockBreakingDelta(player, world, blockPos) / breakThreshold)
+ val delta = blockState.calcBlockBreakingDelta(player, world, blockPos)
+ if (delta <= 0f) return false
+ val ticksNeeded = breakThreshold / delta
return (ticksNeeded <= 1 && ticksNeeded != 0f) || player.isCreative
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun SafeContext.instantBreakable(blockState: BlockState, blockPos: BlockPos, breakThreshold: Float): Boolean { | |
| val ticksNeeded = 1 / (blockState.calcBlockBreakingDelta(player, world, blockPos) / breakThreshold) | |
| return (ticksNeeded <= 1 && ticksNeeded != 0f) || player.isCreative | |
| } | |
| fun SafeContext.instantBreakable(blockState: BlockState, blockPos: BlockPos, breakThreshold: Float): Boolean { | |
| val delta = blockState.calcBlockBreakingDelta(player, world, blockPos) | |
| if (delta <= 0f) return false | |
| val ticksNeeded = breakThreshold / delta | |
| return (ticksNeeded <= 1 && ticksNeeded != 0f) || player.isCreative | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (2)
91-98: Remove or finalize the commented-out cancellation logic.
Leaving this cancellation code commented out could cause confusion. If you don't plan to implement it soon, consider removing it or marking it with a TODO explaining when it should be revisited.-// override fun SafeContext.onCancel() { -// // ... -//} +// TODO: Implement or remove onCancel() method if not needed.
271-287: Consider supporting off-hand placement if needed.
UsingHand.MAIN_HANDmay be intentional, but if the code must handle items from the off-hand in the future, consider a more flexible approach that checks which hand holds the relevant item.-def placeBlock(hand: Hand, ctx: PlaceContext) { +def placeBlock(preferredHand: Hand, ctx: PlaceContext) { // ... } // Possibly detect if the context item is in off-hand, defaulting to MAIN_HAND if not.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (1)
154-190: Clarify double-break logic and refactor branching.
The logic for handling secondary breaking and switching the primary info is a bit convoluted. A small utility method or reorganizing conditions could improve readability.-fun handleRequestContext(...) : BreakType { - ... - if (!primaryInfo.breaking) { - ... - } else { - ... - } - ... -} +fun handleRequestContext(...) : BreakType { + // Consider extracting stages of logic (e.g., setSecondary(), switchPrimary()) + // into dedicated helper methods. +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt(1 hunks)common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (4)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt (1)
27-37: Question whetherdoneshould always be false.
By always returningfalsefordone, this request can never complete from the framework's perspective. Confirm that this behavior is intentional and won't block any higher-level logic expecting completion.common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (1)
105-106: Verify proper hotbar handling.
The code unconditionally attempts to request the hotbar index fromcontext. If the context is invalid or the hotbar request fails, the function returns early. Confirm that this scenario is fully intended, and ensure it won't skip important steps in the building logic.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (2)
105-106: Repeats past listener return concern.
Returningreturn@listenhere terminates processing for the entire listener, not just the current iteration. If the intent is only to skip this particularBreakInfo, use a loop-level return instead.
362-368: Proceeding only ifsetBlockStatesucceeds is good.
This prevents invokingonBrokenif the state update fails. The approach aligns with avoiding partial or inconsistent block states. No concerns here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (5)
63-64: Synchronize initialization and runtime configuration values.The
pendingInteractionsqueue is initialized with values fromTaskFlowModule.build, but later updated with values frombuildConfig(lines 189-190). This could lead to inconsistent behavior if the initial values don't match the ones provided at runtime.Consider initializing the queue with default values and then immediately updating them in the init block, or create a method to ensure consistency between the two sources.
private val pendingInteractions = LimitedDecayQueue<BreakInfo>( - TaskFlowModule.build.maxPendingInteractions, TaskFlowModule.build.interactionTimeout * 50L + DEFAULT_MAX_PENDING_INTERACTIONS, DEFAULT_INTERACTION_TIMEOUT * 50L ) { info("${it::class.simpleName} at ${it.context.expectedPos.toShortString()} timed out") } + companion object { + private const val DEFAULT_MAX_PENDING_INTERACTIONS = 10 + private const val DEFAULT_INTERACTION_TIMEOUT = 5 + }
326-331: Clarify complex conditional logic for packet handling.The condition for sending a stop break packet (
breakingDelta < 1 && (breakingDelta >= 0.7 || info.breakConfig.doubleBreak)) is complex and lacks comments explaining its purpose or edge cases.Add comments explaining the logic and consider extracting it to a descriptive method:
- if (breakingDelta < 1 && (breakingDelta >= 0.7 || info.breakConfig.doubleBreak)) { + // Send stop packet if breaking is nearly complete or double break is enabled + if (shouldSendStopPacket(breakingDelta, info.breakConfig.doubleBreak)) { ctx.stopBreakPacket(sequence + 1, connection) pendingUpdateManager.incrementSequence() } + private fun shouldSendStopPacket(breakingDelta: Float, doubleBreak: Boolean): Boolean = + breakingDelta < 1 && (breakingDelta >= 0.7 || doubleBreak)
280-284: Optimize permission checks in attackBlock method.The player permission checks are performed after other potentially expensive operations. For better efficiency, verify permissions earlier in the method.
private fun SafeContext.attackBlock(info: BreakInfo): Boolean { val ctx = info.context + if (player.isBlockBreakingRestricted(world, ctx.expectedPos, interaction.currentGameMode)) return false + if (!world.worldBorder.contains(ctx.expectedPos)) return false if (interaction.currentGameMode.isCreative) { // Creative mode handling... } if (info.breaking) return false - if (player.isBlockBreakingRestricted(world, ctx.expectedPos, interaction.currentGameMode)) return false - if (!world.worldBorder.contains(ctx.expectedPos)) return false
103-105: Improve hotbar slot selection logic.The current implementation checks if a hotbar request is not done, and if so, returns from the entire listener. This prevents any further processing in the same tick when a hotbar change is in progress.
- if (!swapped && !request.hotbarConfig.request(HotbarRequest(info.context.hotbarIndex)).done) - return@listen + val hotbarRequest = request.hotbarConfig.request(HotbarRequest(info.context.hotbarIndex)) + if (!swapped && !hotbarRequest.done) { + return@listen + } + swapped = swapped || hotbarRequest.doneThis change makes the logic more explicit and updates the swapped flag to reflect successful hotbar swaps.
195-199: Consider adding time-based acceptance criteria for breaking requests.The current
canAcceptmethod only checks for position conflicts, but doesn't consider scenarios like rapidly toggling break operations at the same position, which could lead to server-side rate limiting or anti-cheat triggers.private fun SafeContext.canAccept(ctx: BreakContext) = pendingInteractions.none { it.context.expectedPos == ctx.expectedPos } && breakingInfos.none { info -> info?.context?.expectedPos == ctx.expectedPos } && !blockState(ctx.expectedPos).isAir + && lastBreakAttemptsMap.getOrDefault(ctx.expectedPos, 0L) + MIN_BREAK_INTERVAL_MS < System.currentTimeMillis() + companion object { + private val lastBreakAttemptsMap = mutableMapOf<BlockPos, Long>() + private const val MIN_BREAK_INTERVAL_MS = 50L // Adjust as needed + } + // Add this to attackBlock or handleRequestContext to track attempts + lastBreakAttemptsMap[ctx.expectedPos] = System.currentTimeMillis()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (4)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (4)
108-111: Fix unintentional early return from the entire listener.The current code uses
return@listenwhich aborts processing of the entire tick event listener rather than skipping just the currentBreakInfo. This can prevent processing other break operations in the same tick.- if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@listen + if (info.breakConfig.rotateForBreak && !info.context.rotation.done) return@forEach
366-368: Validate return value ofworld.setBlockState(...).When
setBlockStatereturns false, the old block remains intact. If this occurs, the code still proceeds withblock.onBroken(...), which could lead to inconsistent game state.- if (setState) block.onBroken(world, ctx.expectedPos, ctx.checkedState) + if (setState) { + block.onBroken(world, ctx.expectedPos, ctx.checkedState) + } else { + this@BreakManager.warn("Failed to set new block state for ${ctx.expectedPos}") + return false + }
81-87: Review early return logic in request processing.The
return@requeststatement at line 86 will exit the entire request label, skipping processing of all remaining contexts in the request. Ensure this is the intended behavior and not just meant to skip the current context.- if (breakType == BreakType.Null) return@request + if (breakType == BreakType.Null) return@forEachIf the intention is to stop processing the entire request when encountering a Null break type, keep the current code. Otherwise, use the suggested change to only skip the current context.
1-447: Overall the code is well-structured and handles complex breaking mechanics effectively.The implementation manages different break modes, confirmation strategies, and provides detailed controls for animations, sounds, and packets. The separation of concerns between different breaking phases (start, progress, completion) is clean and maintainable.
| val progress = (breakDelta * breakingTicks) / breakConfig.breakThreshold | ||
| return if (progress > 0.0f) (progress * 10.0f).toInt() else -1 | ||
| } |
There was a problem hiding this comment.
Prevent potential division by zero.
The calculation of progress involves division by breakConfig.breakThreshold. If this value is 0 (which could happen depending on configuration), it would result in a division by zero exception.
- val progress = (breakDelta * breakingTicks) / breakConfig.breakThreshold
+ val threshold = breakConfig.breakThreshold.takeIf { it > 0 } ?: 1.0f
+ val progress = (breakDelta * breakingTicks) / threshold📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val progress = (breakDelta * breakingTicks) / breakConfig.breakThreshold | |
| return if (progress > 0.0f) (progress * 10.0f).toInt() else -1 | |
| } | |
| val threshold = breakConfig.breakThreshold.takeIf { it > 0 } ?: 1.0f | |
| val progress = (breakDelta * breakingTicks) / threshold | |
| return if (progress > 0.0f) (progress * 10.0f).toInt() else -1 | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
115-119:⚠️ Potential issueFix unintentional early return from the entire listener.
The conditional return uses
return@listenwhich aborts processing of the entire tick event listener. If the intent is to skip just the current processing, consider different approaches.- if ((!swapped && !info.hotbarConfig.request(HotbarRequest(info.context.hotbarIndex)).done) - || (info.breakConfig.rotateForBreak && !info.context.rotation.done)) { - postEvent() - return@listen - } + // Check conditions to continue processing + val shouldContinueProcessing = swapped || info.hotbarConfig.request(HotbarRequest(info.context.hotbarIndex)).done + val canRotate = !info.breakConfig.rotateForBreak || info.context.rotation.done + if (!shouldContinueProcessing || !canRotate) { + postEvent() + return@listen + }
372-391:⚠️ Potential issueValidate return value of
world.setBlockState(...).When
setBlockStatereturns false at line 385, the old block remains intact, but the code still proceeds withblock.onBroken(...). Validate the return value to prevent potential issues.val setState = world.setBlockState(ctx.expectedPos, fluidState.blockState, 11) - if (setState) block.onBroken(world, ctx.expectedPos, ctx.checkedState) + if (setState) { + block.onBroken(world, ctx.expectedPos, ctx.checkedState) + } else { + this@BreakManager.warn("Failed to set new block state for ${ctx.expectedPos}") + return false + }
438-440:⚠️ Potential issuePrevent potential division by zero.
The calculation of progress involves division by
breakConfig.breakThreshold. This could cause a division by zero exception if the threshold is 0.- val progress = (breakDelta * breakingTicks) / breakConfig.breakThreshold + val threshold = breakConfig.breakThreshold.takeIf { it > 0 } ?: 1.0f + val progress = (breakDelta * breakingTicks) / threshold
🧹 Nitpick comments (7)
common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceRequest.kt (1)
30-43: Well-structured data class with appropriate error handling.The
PlaceRequestclass is designed with a clear structure, encapsulating all necessary components for block placement operations. The use ofrunSafefor error handling in thedoneproperty is a good practice to prevent exceptions during execution.Consider simplifying line 42 by removing the redundant comparison with
true:- } == true + }The design follows a consistent pattern with other request managers, making the code maintainable and extensible.
common/src/main/kotlin/com/lambda/event/events/UpdateManagerEvent.kt (1)
22-42: Well-structured event hierarchy using sealed classes.The event system is well-designed using Kotlin sealed classes, which provides good type safety and exhaustiveness checking. The consistent Pre/Post event pattern across all manager types indicates a cohesive design.
Consider adding KDoc comments for each event class to explain their specific purpose and usage context.
sealed class UpdateManagerEvent { + /** + * Events related to rotation updates in the game. + */ sealed class Rotation { + /** + * Triggered before rotation processing occurs. + */ class Pre : Event + /** + * Triggered after rotation processing is complete. + */ class Post : Event } + /** + * Events related to hotbar slot selection updates. + */ sealed class Hotbar { class Pre : Event class Post : Event } + /** + * Events related to block breaking operations. + */ sealed class Break { class Pre : Event class Post : Event } + /** + * Events related to block placement operations. + */ sealed class Place { class Pre : Event class Post : Event } }common/src/main/kotlin/com/lambda/interaction/request/rotation/RotationManager.kt (1)
99-99: Update comment to reflect removed code.There's a commented-out reference to
.fixSensitivity(prevRotation)on line 99. Consider either removing the comment entirely or updating it to explain why this method is no longer needed.- currentRotation = targetRotation/*.fixSensitivity(prevRotation)*/ + currentRotation = targetRotationcommon/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (2)
43-49: Consider refactoring the condition check.The nested condition checking is a bit complex. Consider extracting these checks into a separate method for better readability.
- if (request.placeContext.sneak && !player.isSneaking - || (request.buildConfig.placeSettings.rotateForPlace && !request.placeContext.rotation.done) - || (!request.hotbarConfig.request(HotbarRequest(request.placeContext.hotbarIndex)).done) - ) { + if (shouldSkipPlacement(request)) {And add this method:
private fun shouldSkipPlacement(request: PlaceRequest): Boolean { return (request.placeContext.sneak && !player.isSneaking) || (request.buildConfig.placeSettings.rotateForPlace && !request.placeContext.rotation.done) || (!request.hotbarConfig.request(HotbarRequest(request.placeContext.hotbarIndex)).done) }
68-84: Add a check for empty hand before interacting.It would be safer to verify that the hand isn't empty before attempting to interact with a block, especially in non-creative modes.
private fun SafeContext.placeBlock(request: PlaceRequest, hand: Hand) { + if (player.getStackInHand(hand).isEmpty && !interaction.hasCreativeInventory()) { + warn("Cannot place block with empty hand in non-creative mode") + return + } val actionResult = interaction.interactBlock( player, hand, request.placeContext.result )common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (1)
84-112: Consider breaking down the request processing logic.This section is complex with nested conditional logic. Consider extracting the request processing into a separate method to improve readability and maintainability.
- if (updateRequest { true }) { - currentRequest?.let request@ { request -> - var instaBreaks = 0 - request.contexts - .sortedBy { it.instantBreak } - .forEach { requestCtx -> - // ... existing code - } - } - } + if (updateRequest { true }) { + currentRequest?.let { request -> processBreakRequest(request) } + } // Add this method: private fun SafeContext.processBreakRequest(request: BreakRequest) { var instaBreaks = 0 var swapped = false request.contexts .sortedBy { it.instantBreak } .forEach { requestCtx -> if (!canAccept(requestCtx)) return@forEach val breakType = handleRequestContext( requestCtx, request.onBreak, request.buildConfig, request.rotationConfig, request.hotbarConfig ) if (breakType == BreakType.Null) return if (requestCtx.instantBreak && instaBreaks < request.buildConfig.breakSettings.breaksPerTick) { breakingInfos.getOrNull(breakType.index)?.let { info -> if (!swapped) { request.hotbarConfig.request(HotbarRequest(info.context.hotbarIndex)) swapped = true } updateBlockBreakingProgress(info, player.mainHandStack) instaBreaks++ } } } }common/src/main/kotlin/com/lambda/config/groups/BreakSettings.kt (1)
32-32: Consider adding a validation to prevent zero threshold.Given that the
breakThresholdis used in division operations in BreakManager, consider setting a minimum value slightly above zero to prevent potential division by zero errors.- override val breakThreshold by c.setting("Break Threshold", 1.0f, 0.1f..1.0f, 0.02f, "The break amount at which the block is considered broken") { vis() } + override val breakThreshold by c.setting("Break Threshold", 1.0f, 0.1f..1.0f, 0.02f, "The break amount at which the block is considered broken") { vis() } + .validateWith { it > 0.0f }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
common/src/main/kotlin/com/lambda/config/groups/BreakSettings.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/BuildConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/BuildSettings.kt(2 hunks)common/src/main/kotlin/com/lambda/config/groups/PlaceSettings.kt(1 hunks)common/src/main/kotlin/com/lambda/event/events/UpdateManagerEvent.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/construction/context/PlaceContext.kt(2 hunks)common/src/main/kotlin/com/lambda/interaction/request/RequestHandler.kt(2 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarManager.kt(2 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceRequest.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/rotation/RotationManager.kt(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (26)
common/src/main/kotlin/com/lambda/interaction/construction/context/PlaceContext.kt (5)
42-42: Architectural change from hand-based to hotbar index-based selection.The switch from
handtohotbarIndexrepresents a significant architectural change in how player interactions are managed. This approach provides more fine-grained control over item selection by directly referencing inventory slots rather than just main/offhand.
47-47: Minor syntax adjustment.The trailing comma has been removed as this is now the last parameter in the constructor. This maintains proper Kotlin syntax.
52-69: Comparison logic simplified with the removal of hand-based comparison.The
compareTomethod no longer includes a comparison based onhand, which simplifies the ordering logic. This is consistent with the architectural shift to hotbar index-based selection.
76-76: Configuration-based rotation decision.The
shouldRotatemethod now references theplaceSettings.rotateForPlaceconfiguration property, which centralizes rotation decision-making in the configuration system. This is a good practice for maintainability and consistency.
35-48:Details
❓ Verification inconclusive
Verify interaction handling with the removed 'interact' method.
The
interactmethod appears to have been completely removed from this class based on the AI summary. Ensure that interaction logic is properly handled elsewhere in the codebase to maintain functionality.
🏁 Script executed:
#!/bin/bash # Search for where interaction handling might have been moved to echo "Searching for new interaction handling locations..." rg -A 10 "class.*implements.*BuildContext" --glob "*.kt" echo "Checking if interaction handling was moved to another class..." rg -A 10 "fun interact" --glob "*.kt" --glob "**/construction/**"Length of output: 374
Action Required: Confirm Interaction Handling Logic
The initial automated search did not reveal any clear relocation of the
interactmethod functionality, and no alternative implementation was found in the repository. Please verify manually that the interaction handling logic removed from thePlaceContextclass is now properly implemented elsewhere. Specifically:
- Confirm that there is no lingering or hidden definition of
fun interact(…)controlling interaction behavior in any Kotlin file.- Review the overall interaction flow to ensure the removal is intentional and that the functionality has been migrated or refactored correctly.
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt (2)
29-36: Well-structured request class for block breaking operations.The
BreakRequestdata class is designed with clear responsibilities and a comprehensive set of parameters. It properly extends theRequestclass and initializes it with the provided priority.
37-43: Good implementation of the done property with proper error handling.The
doneproperty implementation correctly usesrunSafeto handle potential exceptions and verifies that all contexts' target states match the expected block states in the world. The comparison with== trueensures proper handling of potential null returns fromrunSafe.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceConfig.kt (2)
23-33: Clean abstract class design with appropriate delegation.The
PlaceConfigclass provides a good template for placement configurations with well-defined abstract properties. TherequestInternalmethod properly delegates to thePlaceManager, maintaining separation of concerns.
34-38: Clear enumeration for place confirmation strategies.The
PlaceConfirmationenum provides descriptive options for different confirmation behaviors during block placement. This enhances configurability and flexibility in the placement system.common/src/main/kotlin/com/lambda/interaction/request/RequestHandler.kt (1)
71-72: Good enhancement of the event system with pre/post events.Adding abstract methods for
preEvent()andpostEvent()improves the event handling system by allowing events to be fired before and after request processing. This enhances observability and allows for more flexible integrations.common/src/main/kotlin/com/lambda/config/groups/PlaceSettings.kt (2)
24-28: Well-designed settings class with flexible configuration options.The
PlaceSettingsclass extendsPlaceConfigwith a good constructor design that allows for configuration, priority, and visibility control. This provides flexibility while maintaining a clean interface.
29-31: Comprehensive settings with appropriate defaults and constraints.The implementation of the abstract properties uses the
settingmethod effectively, providing sensible defaults, clear descriptions, and appropriate constraints where needed. The range constraint forplacementsPerTickis particularly good for preventing performance issues.common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarManager.kt (3)
28-36: Good documentation addition for mixin references.The added documentation clearly links to relevant mixin methods, which improves code navigability and understanding.
65-66: Consistent implementation of event hooks.The addition of
preEventandpostEventmethods follows a consistent pattern across managers, improving the event handling architecture.
38-40:Details
❓ Verification inconclusive
Verify impact of changing serverSlot fallback value.
The fallback value for
serverSlothas been changed from-1to0. This means that when no slot is selected, it will return the first hotbar slot (0) instead of an invalid index (-1).
🏁 Script executed:
#!/bin/bash # Search for places where serverSlot is compared with -1 which might rely on the old behavior rg -A 3 "serverSlot.+(-1|==\s*-1)" --type kotlinLength of output: 50
Verify Impact of Fallback Change on
serverSlotValueThe recent change sets
serverSlot’s fallback to0(selecting the first hotbar slot) instead of-1. The initial search for comparisons against-1yielded no results, but due to the limited scope of the regex search, please manually verify that no downstream logic relies on the previous-1value to indicate an invalid or unselected slot.
- Confirm that unit or integration tests expecting a
-1fallback have been updated.- Revisit any conditional checks or documentation referencing the old behavior.
Updated snippet:
val serverSlot get() = runSafe { interaction.lastSelectedSlot } ?: 0common/src/main/kotlin/com/lambda/interaction/request/rotation/RotationManager.kt (3)
71-73: Added pre-event trigger at the beginning of event processing.This follows the consistent pattern of event processing across managers. Good addition for modular event handling.
113-115: Added post-event trigger at the end of event processing.This completes the event handling lifecycle, providing a consistent pattern with other managers.
266-267: Implementation of event methods is consistent with other managers.The implementation of
preEventandpostEventmethods follows the same pattern as seen in the HotbarManager, creating a unified approach to event handling.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (1)
32-66: Implementation structure looks solid.The initialization block with event listeners is well organized. The structure handles the placing logic in a clean, sequential manner, with appropriate condition checks before proceeding with the action.
common/src/main/kotlin/com/lambda/config/groups/BuildConfig.kt (1)
29-29: Good refactoring of configuration properties.Consolidating the breaking-related properties into a single
breakSettingsobject improves maintainability and organization of the configuration.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (1)
56-68: Well-structured manager with clean property management.The use of getters and setters for the breaking info arrays provides a clean API. The decay queue implementation for pending interactions is a good approach to manage timeouts.
common/src/main/kotlin/com/lambda/config/groups/BreakSettings.kt (1)
25-47: Well-structured configuration class with comprehensive options.The BreakSettings class provides a clean implementation of all break-related configurations with appropriate defaults, ranges, and visibility conditions. The use of dependent visibility for
minFortuneLevelbased onforceFortunePickaxeis a good design choice.common/src/main/kotlin/com/lambda/config/groups/BuildSettings.kt (4)
21-22: Import statements properly updated.The imports have been updated to reference the necessary classes for the new configuration structure, which aligns with the refactoring to use encapsulated settings classes.
41-41: Good consolidation of breaking-related settings.Replacing multiple individual breaking properties with a single encapsulated
breakSettingsobject improves code organization and follows the Single Responsibility Principle. This change makes the code more maintainable and aligns with the PR objective of enabling more fine-grained material control.
44-44: Good consolidation of placing-related settings.Similar to the breaking settings, consolidating placing properties into a single
placeSettingsobject is a good design choice that enhances modularity and maintainability.
46-47: Interaction timeout logic properly updated.The condition for showing the interaction timeout setting has been correctly adapted to reference the new encapsulated configuration properties. The logic preserves the original behavior while working with the new structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (5)
57-57: Remove or utilize theHandimport.
net.minecraft.util.Handis imported but not referenced in this file. If no usage is planned, consider removing it to avoid potential linter warnings for unused imports.-import net.minecraft.util.Hand
86-93: Consider removing or clarifying the commented-outonCancelimplementation.
Leaving large commented blocks might cause confusion. If theonCancelfunctionality is planned for future implementation, consider adding a TODO or explaining its purpose. Otherwise, remove it to keep the code clean.
97-97: Rename the method or field to avoid confusion.
There is a Boolean fieldcollectDropsand a functioncollectDrops(). Each has different semantics, which can be confusing. Consider renaming one to differentiate the property from the method’s action.
118-120: Optimize blocked-result checks for performance.
results.filterNot { it.blockPos in pendingInteractions.map { it.expectedPos } }can become O(n²) for large lists. ConvertingpendingInteractions.map { it.expectedPos }to aSetwould produce faster lookups.-val resultsNotBlocked = results.filterNot { result -> - result.blockPos in pendingInteractions.map { it.expectedPos } -}.sorted() +val blockedPositions = pendingInteractions.map { it.expectedPos }.toSet() +val resultsNotBlocked = results.filterNot { it.blockPos in blockedPositions }.sorted()
193-217: Enhance handling of multiple drops.
Currently, you only process the first item indropsToCollecteach tick. If multiple items are present, you might inadvertently ignore others until the next tick iteration. If that’s intentional, no changes needed; otherwise, consider iterating the entire set to handle multiple drops in a single tick. You could also rename the function to avoid confusion with thecollectDropsfield.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (1)
41-65: Reduce repetitive calls topostEvent().
Multiple early-return paths callpostEvent(). You can simplify by wrapping the core logic in a try-finally block to ensurepostEvent()is always invoked exactly once.listen<TickEvent.Pre>(Int.MIN_VALUE) { preEvent() - if (!updateRequest { true }) { - postEvent() - return@listen - } try { + if (!updateRequest { true }) return@listen currentRequest?.let { request -> if ( (request.placeContext.sneak && !player.isSneaking) || (request.buildConfig.placeSettings.rotateForPlace && !request.placeContext.rotation.done) || (!request.hotbarConfig.request(HotbarRequest(request.placeContext.hotbarIndex)).done) ) return@listen pendingInteractions.setMaxSize(request.buildConfig.maxPendingInteractions) pendingInteractions.setDecayTime(request.buildConfig.interactionTimeout * 50L) placeBlock(request, Hand.MAIN_HAND) } } finally { postEvent() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarManager.kt(3 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceRequest.kt(1 hunks)common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt(8 hunks)common/src/main/resources/lambda.accesswidener(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceRequest.kt
- common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarManager.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (16)
common/src/main/resources/lambda.accesswidener (2)
31-32: Added access to hand swing duration and slot synchronization methods.These new access wideners enable important functionality for improving building mechanics:
getHandSwingDurationprovides timing information for hand animationssyncSelectedSlotallows proper synchronization of the hotbar selection with the serverThese changes align well with the PR objective of enhancing building mechanics and supporting the transition from hand-based logic to hotbar indexing.
71-72: Added access to packet management capabilities.These new access wideners provide critical functionality for the "Packetmine" feature:
sendSequencedPacketenables optimized packet sending for block interactionspendingUpdateManageraccess allows coordination of pending client-side block operationsThese changes directly support the PR's objective of enhancing building with Packetmine and providing more fine-grained material control.
common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (11)
42-45: No immediate concerns with new imports.
These additional imports are consistent with the refactoring toward breakup of request handling (breaking vs hotbar vs placing).
68-68: Introducinghotbarparameter appears reasonable.
The newhotbar: HotbarConfigconstructor parameter seems in line with the updated design of handling hotbar interactions. Ensure existing calls to this constructor provide the requiredHotbarConfigor rely on the default.
106-110: ValidatebreaksPerTickusage.
Splitting multiple breaks in a single tick makes sense for performance. Confirm that the remainder of the system can handle multiple simultaneous break requests and won't conflict with subsequent actions.
112-115: Break requests look properly batched.
Aggregating break contexts in oneBreakRequestcall is efficient for batch processing. Just ensure concurrency is properly managed if there's asynchronous logic elsewhere.
128-128: Skipping if there are pending interactions is logical.
Continuing only when all pending interactions are resolved avoids unintended overlaps. No objections here.
138-138: Conditional pathing check is concise.
Returning early if pathing is disabled helps keep the logic straightforward.
149-149: Ensuring maximum pending interactions are not exceeded.
This gate prevents queue overflow and potential indefinite wait states. Looks good.
150-155: Efficient grouping of break contexts.
Collecting and processing multiple break contexts at once helps reduce overhead. The logic is consistent with the bigger break handling changes.
156-159: Placement request is straightforward.
InvokingPlaceRequestonly for place results is a clean separation of concerns. No further suggestions.
170-171: Tick-based blueprint updates are appropriate.
Invokingtick()on aTickingBlueprintinsideTickEvent.Postis a sensible approach for continuous updates.
173-175: Handling empty structures gracefully.
Failing the task if the blueprint is inherently empty prevents idle or confusing states.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (3)
36-40: Consider removing or utilizingpendingInteractions.
Currently, this queue is initialized and configured but never used in the shown code. If it’s intended for a future feature, consider adding a TODO comment. Otherwise, removing it may reduce confusion and improve clarity.
96-97: Event invocation looks good.
preEvent()andpostEvent()are invoked consistently. The naming is clear and aligned with the surrounding code.
66-76:Details
✅ Verification successful
Confirm potential single-threaded environment.
onRotatemodifiescurrentRequestin the same object that is also referenced byTickEvent.Pre. If these events are triggered on different threads or concurrently, it might introduce a data race. Verify the event bus processes them on the same thread.
🏁 Script executed:
#!/bin/bash # Description: Search for concurrency disclaimers or scheduling logic around event calls. rg -A 5 "listen<|onRotate|Thread"Length of output: 65572
Confirmed: Event Bus Uses Single-Threaded Execution
Our investigation shows that the event listeners—including theonRotatecallback and theTickEvent.Prelistener—are processed on the main thread (as evidenced by the consistent use of our mainThread utilities in similar contexts). This minimizes the risk of data races between these events.
- Please ensure that any future changes to event-scheduling or threading do not inadvertently enable concurrent execution of these handlers.
| private fun SafeContext.placeBlock(request: PlaceRequest, hand: Hand) { | ||
| val actionResult = interaction.interactBlock( | ||
| player, hand, request.placeContext.result | ||
| ) | ||
|
|
||
| if (actionResult.isAccepted) { | ||
| if (actionResult.shouldSwingHand() && request.interactionConfig.swingHand) { | ||
| player.swingHand(hand) | ||
| } | ||
|
|
||
| if (!player.getStackInHand(hand).isEmpty && interaction.hasCreativeInventory()) { | ||
| mc.gameRenderer.firstPersonRenderer.resetEquipProgress(hand) | ||
| } | ||
| } else { | ||
| warn("Internal interaction failed with $actionResult") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling around actionResult.
When actionResult is rejected, the code currently logs a warning and stops. Consider adding a fallback or additional handling logic to keep the user informed or revert state if necessary.
if (!actionResult.isAccepted) {
- warn("Internal interaction failed with $actionResult")
+ warn("Interaction was not accepted: $actionResult. Consider fallback or UI notification.")
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt (1)
1-16: Consider removing the licensing comments if the repository already includes a LICENSE file.
While it's common to include the license header in every file, some projects rely on a top-level LICENSE file to declare licensing.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (1)
74-75: Beware of forcing sneaking without user feedback.
By unconditionally settingit.input.sneaking = true, you override user input. Consider notifying the user or providing a configurable toggle to prevent confusion.common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (2)
83-90: Remove or clarify commented-out code.
The commented-outonCancelcode creates confusion. If it’s not needed, remove it; otherwise, add a TODO or rationale for keeping it.
198-223: Handle multiple drops or rename the method to reflect a single item approach.
Currently,collectDrops()processes only the first matchingitemDrop. Consider iterating over all items indropsToCollector rename the method to avoid confusion.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (5)
86-86: Fix typo in TODO comment.Minor issue, but for better code readability, correct the TODO comment formatting.
-//ToDo: improve instamine / non instamine integration +// TODO: improve instamine / non instamine integration
354-358: Unclear conditional logic for packet sending.The condition
breakingDelta < 1 && (breakingDelta >= 0.7 || info.breakConfig.doubleBreak)has complex logic that would benefit from a comment explaining when and why a stop packet is sent in this case.- if (breakingDelta < 1 && (breakingDelta >= 0.7 || info.breakConfig.doubleBreak)) { + // Send stop packet for blocks that are almost broken (70%+) or when double breaking is enabled + if (breakingDelta < 1 && (breakingDelta >= 0.7 || info.breakConfig.doubleBreak)) {
458-475: Consider extending enum with methods for better encapsulation.The
BreakTypeenum uses a direct reference toprimaryBreakingInfoandsecondaryBreakingInfoin thenullifymethod. This creates tight coupling between the enum and the outer object.Improve encapsulation by passing a reference to the parent object:
enum class BreakType(val index: Int) { Primary(0), Secondary(1), Null(-1); fun getBreakThreshold(breakConfig: BreakConfig) = when (this) { Primary -> breakConfig.breakThreshold Secondary -> 1.0f else -> -1.0f } - fun nullify() = + fun nullify(manager: BreakManager = BreakManager) = when (this) { - Primary -> primaryBreakingInfo = null - Secondary -> secondaryBreakingInfo = null + Primary -> manager.setPrimaryBreakingInfo(null) + Secondary -> manager.setSecondaryBreakingInfo(null) else -> {} } } // And add these methods to the BreakManager object: // private fun setPrimaryBreakingInfo(value: BreakInfo?) { breakingInfos[0] = value } // private fun setSecondaryBreakingInfo(value: BreakInfo?) { breakingInfos[1] = value }
1-16: Copyright year should be updated.The copyright year is set to 2025, which is in the future. It should be updated to the current year or the year when the code was created.
-/* Copyright 2025 Lambda +/* Copyright 2024 Lambda
159-169:Details
❓ Verification inconclusive
Incomplete feature marked by commented code.
The code contains commented-out functionality for drop callbacks with a TODO comment. Consider implementing this feature or adding more specific comments about what needs to be done.
Would you like me to outline an implementation for this functionality?
🏁 Script executed:
#!/bin/bash # Search for any usages of a function that might be expecting this drop callback to work rg -A 3 "onItemDrop" --glob "*.kt" --no-filenameLength of output: 1104
Action Required: Clarify and Finalize the Drop Callback Implementation
The commented-out drop callback logic in
BreakManager.kt(lines 159–169) indicates an incomplete feature. The code currently leaves the invocation ofonItemDropcommented out, while the callback parameter is defined and used elsewhere. Please address the following:
- Confirm whether the drop callback functionality should be fully implemented.
- If it’s intended to be implemented later, update the TODO comment with clearer details on the expected behavior and any dependencies (e.g., handling the tracked data order).
- Alternatively, if the feature is no longer needed, remove the commented code to avoid confusion.
Would you like guidance or an outline for a proposed implementation of the drop callback feature?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt(1 hunks)common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (6)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakRequest.kt (1)
30-45: Validatedoneproperty usage and handle potential null values.
TherunSafefunction returns a nullable Boolean. Using== trueis acceptable, but ensure that any exceptions thrown inside the block are correctly handled and won't silently fail.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (1)
92-93: Enhance error handling aroundactionResult.
(This comment updates a similar past recommendation.)
Consider adding a fallback or UI notification so the user isn’t left in the dark when the interaction fails.common/src/main/kotlin/com/lambda/task/tasks/BuildTask.kt (1)
103-117: Verify concurrency control when issuing multiple break requests.
You request instant breaks and then create aBreakRequestwithin the same tick ifbreakContextsis not empty. Confirm that both scenarios can’t conflict (e.g., double-breaking the same blocks).common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (3)
93-93: Use better named return target.Using
return@forEachmakes it clearer that you're only skipping the current iteration, not exiting the entire lambda.- if (!canAccept(requestCtx)) return@forEach + if (!canAccept(requestCtx)) return@forEach
393-394: Validate return value ofworld.setBlockState(...).When
setBlockStatereturns false, the old block remains intact. If this occurs, the code still proceeds withblock.onBroken(...). Confirm this mismatch doesn't lead to unintended states or partial break.val setState = world.setBlockState(ctx.expectedPos, fluidState.blockState, 11) -if (setState) block.onBroken(world, ctx.expectedPos, ctx.checkedState) +if (setState) { + block.onBroken(world, ctx.expectedPos, ctx.checkedState) +} else { + this@BreakManager.warn("Failed to set new block state for ${ctx.expectedPos}") + return false +}
447-449: Prevent potential division by zero.The calculation of progress involves division by
breakConfig.breakThreshold. If this value is 0 (which could happen depending on configuration), it would result in a division by zero exception.-val progress = (breakDelta * breakingTicks) / breakConfig.breakThreshold +val threshold = breakConfig.breakThreshold.takeIf { it > 0 } ?: 1.0f +val progress = (breakDelta * breakingTicks) / threshold
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
common/src/main/kotlin/com/lambda/util/player/PlayerUtils.kt (2)
46-51: Consider allowing a custom hand parameter instead of relying onplayer.activeHand.
This could provide more flexibility if an off-hand swing is required in the future.-fun SafeContext.swingHand(swingType: BuildConfig.SwingType) = - when (swingType) { - BuildConfig.SwingType.Vanilla -> player.swingHand(player.activeHand) - BuildConfig.SwingType.Server -> connection.sendPacket(HandSwingC2SPacket(player.activeHand)) - BuildConfig.SwingType.Client -> swingHandClient(player.activeHand) - } +fun SafeContext.swingHand(swingType: BuildConfig.SwingType, hand: Hand = player.activeHand) = + when (swingType) { + BuildConfig.SwingType.Vanilla -> player.swingHand(hand) + BuildConfig.SwingType.Server -> connection.sendPacket(HandSwingC2SPacket(hand)) + BuildConfig.SwingType.Client -> swingHandClient(hand) + }
53-59: Add a clarifying comment for negativehandSwingTicks.
A brief explanation would help maintainers understand that settinghandSwingTicksto -1 forces a swing reset.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceConfig.kt (1)
24-44: Add KotlinDoc for the enum constants inPlaceConfirmation.
Providing a brief description of each confirmation mode will clarify their differences for future maintainers.enum class PlaceConfirmation { + /** + * No confirmation is requested; places immediately. + */ None, + /** + * Immediately place the block, then wait for confirmation. + */ PlaceThenAwait, + /** + * Wait for confirmation first, then place the block. + */ AwaitThenPlace }common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (2)
57-57: Add a fallback for unresponsive server interactions.The TODO at line 57 hints at a need to handle scenarios where the server may never confirm or deny a placement, leaving entries stuck in
pendingInteractions. Consider timing out or otherwise providing a fallback to avoid indefinite state.
99-117: Improve logging for placement failures.When
actionResult.isNotAccepted, only a warning is logged. Consider clarifying the reason for failure or providing user feedback. For instance, if the server specifically denies the action, the user may be stuck without a clear explanation.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (2)
395-397: Consider logging unplaced states and providing a fallback path.When
setBlockStatefails (setState == false), the code silently skips callingblock.onBroken(...). If needed, add fallback behavior or more explicit logging to track these situations, preventing silent failures that could leave the game state inconsistent.
139-159: Review concurrency of block updates in server event.The logic in this block update listener modifies
pendingInteractionsandbreakingInfoswhile iterating over them, which is typically safe in a single-threaded environment (the main game loop). However, if future concurrency or parallel event handling is introduced, consider concurrency-safe collections or synchronization to avoid potential data races.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
common/src/main/kotlin/com/lambda/config/groups/BreakSettings.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/BuildConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/config/groups/PlaceSettings.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt(1 hunks)common/src/main/kotlin/com/lambda/util/player/PlayerUtils.kt(2 hunks)common/src/main/resources/lambda.accesswidener(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakConfig.kt
- common/src/main/kotlin/com/lambda/config/groups/BuildConfig.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (8)
common/src/main/resources/lambda.accesswidener (3)
19-22: Enhancing Block Placement Capabilities with BlockItem MethodsThese new accessible methods from the BlockItem class provide finer control over block placement mechanics, which aligns with the PR objective of enhancing building functionality and material control. The access to these methods will allow for more sophisticated block placement features in the mod.
35-36: Improved Player Interaction ControlMaking these methods accessible enhances the mod's ability to control player interactions:
getHandSwingDurationallows for better synchronization of animation timingsyncSelectedSlotenables proper hotbar selection management between client and serverThis supports the PR's goal of moving from hand-based logic to hotbar indexing as mentioned in the summary.
75-76: Client-Server Synchronization ImprovementsThese additions provide crucial access to packet sequencing and update management:
sendSequencedPacketallows for controlled packet sending with proper sequencing- Access to
pendingUpdateManagerenables monitoring and manipulation of pending world updatesThese are essential for implementing reliable block breaking and placement features, especially for the "Packetmine" functionality mentioned in the PR title.
common/src/main/kotlin/com/lambda/util/player/PlayerUtils.kt (1)
3-3: No issues with these new imports.Also applies to: 10-11
common/src/main/kotlin/com/lambda/config/groups/PlaceSettings.kt (2)
18-18: Also applies to: 20-23
24-36: All configurations look consistent with the rest of the codebase.
Nice use ofc.settingto define each blocking placement parameter. No concerns here.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (1)
72-75: Verify partial condition checks.Here, the request is exited early if sneaking, rotation, or hotbar conditions are not met. If the game logic changes or new constraints are introduced, ensure these checks remain comprehensive or that further conditions are also accounted for.
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (1)
441-443: Prevent division by zero or negative threshold issues.Here,
progressis derived by dividing bybreakConfig.breakThreshold. Although the default range is0.1f..1.0f, you may still want a fallback to avoid potential undefined behavior if a future config update allows zero or negative values.
| override val forceSilkTouch by c.setting("Force Silk Touch", false, "Force silk touch when breaking blocks") { vis() } | ||
| override val forceFortunePickaxe by c.setting("Force Fortune Pickaxe", false, "Force fortune pickaxe when breaking blocks") { vis() } | ||
| override val minFortuneLevel by c.setting("Min Fortune Level", 1, 1..3, 1, "The minimum fortune level to use") { vis() && forceFortunePickaxe } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure mutual exclusivity for Silk Touch & Fortune.
When both forceSilkTouch and forceFortunePickaxe are enabled, there may be a conflict in the final drop outcome. Consider enforcing mutual exclusivity or clarifying which option overrides the other if both are true.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (1)
120-122: Consider graceful handling for non-block items.When the held item is not a
BlockItem, the function returns without feedback. This might confuse users if they attempt to place other items. Provide a log or user-facing message to clarify why no placement occurred, or allow alternative item handling if applicable.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceConfig.kt (1)
39-43: Add explanatory KDoc for place confirmation modes.Providing short documentation for each enum constant (
None,PlaceThenAwait,AwaitThenPlace) helps clarify how they differ, aiding future maintainers and contributors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
common/src/main/kotlin/com/lambda/config/groups/BuildSettings.kt(2 hunks)common/src/main/kotlin/com/lambda/config/groups/PlaceSettings.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceConfig.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- common/src/main/kotlin/com/lambda/config/groups/PlaceSettings.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (8)
common/src/main/kotlin/com/lambda/config/groups/BuildSettings.kt (4)
20-22: Added imports for configuration modes.These imports are necessary to support the new encapsulated configuration structure, providing access to the confirmation mode enums used in the refactored condition.
41-41: Good refactoring: Consolidated breaking settings into a single object.Breaking settings have been effectively encapsulated in a
BreakSettingsclass, improving organization and maintainability by grouping related properties together.
44-44: Good refactoring: Consolidated placing settings into a single object.Placing settings have been effectively encapsulated in a
PlaceSettingsclass, following the same pattern as the breaking settings, which creates a consistent approach to configuration management.
46-46: Updated interaction timeout conditions to work with new encapsulated properties.The condition for interaction timeout has been correctly updated to use the new property paths for confirmation modes. This maintains the original behavior while adapting to the new structure.
common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (4)
59-59: Question on event priority usage.The
listen<TickEvent.Pre>(Int.MIN_VALUE)registration operates at the highest priority possible. Ensure this early event interception is intentional and doesn't unintentionally interfere with other event handlers that might also rely on these early ticks for essential logic.
69-80: Clarify usage of maxPendingPlacements vs maxPendingInteractions.In line 69, the code limits new placements by
request.buildConfig.placeSettings.maxPendingPlacements, while on line 79, you set the queue’s maximum size torequest.buildConfig.maxPendingInteractions. Verify these two limits are intentionally distinct, or consider consolidating them for consistency.
125-148: Enhance error handling for rejected placement.When
actionResult.isAcceptedis false, the code only logs a warning. Improving fallback or recovery steps would be beneficial for user feedback and robust error handling.
173-176: Recheck inverted condition for canceling interaction.val handNotEmpty = !player.getStackInHand(hand).isEmpty val cantInteract = player.shouldCancelInteraction() && handNotEmpty if (!cantInteract) return ActionResult.PASSCurrently, if the player should not cancel interaction, the method returns
PASS. Verify that this logic (invertingcantInteract) aligns with intended usage scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (4)
95-118: Block update event & rotation synergy.
This part checks for block state changes and conditionally triggers placement completion. Consider adding a fallback or re-try logic if the state fails to match unexpectedly, to handle transient server mismatches.
127-157: Refine diagnostic aroundactionResultinplaceBlock().
When placement fails (actionResultnot accepted), you only log a generic warning. More granular logging (e.g., “Out of items”, “Position blocked”) could help track down issues.
173-198: Consider logging reasons for returningActionResult.PASS.
IninteractBlockInternal(), you returnActionResult.PASSfor various conditions (e.g.,cantInteract, empty hand). Logging or commenting these reasons might help debugging.
271-272: Use a shared base for event posting if needed.
preEvent()andpostEvent()replicate the pattern from other managers (e.g.,RotationManager). If you find these repeating in multiple managers, consider a shared base class or interface for uniform event posting.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (2)
95-124: Sorting byinstantBreakis a useful approach.
Collecting and sorting contexts ensures instant-break items get handled quickly. Consider clarifying in code comments any tie-breaking logic if multiple items have the same priority.
176-187: Remove or revisit commented-out item drop code.
This large commented block for entity updates still lingers. If it’s no longer needed, consider removing it to keep the codebase clean. Otherwise, re-enable it (with tests) if it’s a planned feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
common/src/main/kotlin/com/lambda/interaction/request/RequestHandler.kt(3 hunks)common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarManager.kt(3 hunks)common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt(1 hunks)common/src/main/kotlin/com/lambda/interaction/request/rotation/RotationManager.kt(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- common/src/main/kotlin/com/lambda/interaction/request/RequestHandler.kt
- common/src/main/kotlin/com/lambda/interaction/request/hotbar/HotbarManager.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (11)
common/src/main/kotlin/com/lambda/interaction/request/rotation/RotationManager.kt (4)
71-72: InvokepreEvent()carefully within event listener.
You are callingpreEvent()at the very start of thePlayerPacketEvent.Postlistener. This helps ensure upstream logic runs consistently before other actions occur. No immediate concerns, but be mindful of potential re-entry if future logic re-triggers this listener within the same tick.
80-81: SetactiveThisTickwhencurrentRequestis non-null.
UsingactiveThisTick = truesignals that the manager is engaged in the current tick, helping coordinate with other managers (e.g.,PlaceManager). This looks appropriate, with no obvious conflicts.
116-117: Spot-on usage ofpostEvent().
CallingpostEvent()right before leaving the listener ensures that any subscribers to the post-update phase can reliably follow up on this manager’s actions.
268-269: NewpreEvent()andpostEvent()overrides align with your event architecture.
Exposing these overrides in theRotationManagerobject is consistent with other managers. Good addition for structured pre- and post-event hooks.common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (3)
1-16: License header check.
The license block is present and well-formed. No issues here.
60-93: Ensure non-conflicting interactions withBreakManager.
This initialization segment defers placement whenBreakManageris active in the same tick. While this makes sense to avoid conflicts, it’s worth verifying if partial overlap scenarios can arise (e.g., very short break tasks).Please confirm whether fast or partial breaks could inadvertently block placement for multiple consecutive ticks more than intended.
264-269:PlaceInfodata class organizes placement context well.
It neatly captures the context, item, and success callback for the pending placement. Great approach for decoupled interactions.common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (4)
1-16: License block verified.
The license header is properly included. No issues here.
58-70: Pending interactions queue capacity checks.
TheLimitedDecayQueueusage andmaxPendingInteractionsconstraint look appropriate. Verify that extreme concurrency or spammy requests don’t exceed this configuration unexpectedly.
74-93: Conditional skip logic for break cooldown and place manager.
WhenPlaceManageris active or the break cooldown is set, you skip breaking logic. This helps avoid collisions, though ensure that no indefinite skipping can occur if multiple managers constantly toggle active states.
410-414: Good handling ofsetBlockStatereturn value.
You properly guardonBroken(...)behind a successfulsetStatecheck. This avoids mismatched states if the server declines the block break.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (5)
56-58: Consider providing user feedback on timeouts.
pendingInteractionscurrently only logs a timeout message when an interaction expires. Consider providing in-game feedback or automatically retrying the placement to improve user experience.
77-79: Add user-facing fallback when max pending placements is reached.
The code returns early ifpendingInteractions.sizeexceedsmaxPendingPlacements. You might consider queuing further requests or providing a user notification instead of silently aborting.
117-119: Reevaluate forced sneaking approach.
Automatically settinginput.sneaking = truemay conflict with the user's intended actions. Consider a more nuanced approach, such as prompting the user or toggling only if they explicitly allow auto-sneak.
213-214: Notify when modify-world privilege is lacking.
Ifplayer.abilities.allowModifyWorldis false, the code silently passes. Providing a clear in-game message would help the user understand why placement did not occur.
236-238: Implement restriction checks or block them earlier.
A TODO comment indicates further validation is required when usingAwaitThenPlace. Proactively blocking invalid placements as early as possible avoids unnecessary server requests and improves reliability.Do you want me to prepare an implementation of these restriction checks or open a new issue to track it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (2)
common/src/main/kotlin/com/lambda/interaction/request/placing/PlaceManager.kt (2)
129-159: Enhance error handling on rejected placement.
WhenactionResultis not accepted, the code only logs a warning and stops. Consider a fallback action or user notification to clarify the failure reason or allow retry.
256-266: Ensure sound playback aligns with server-client consistency.
TheplaceSoundmethod plays the sound locally in the current world context. Validate that this does not lead to discrepancies in multiplayer scenarios, where the server might need to handle sound events.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (4)
4-16: Copyright notice has future date.The copyright year in the license header is set to 2025, which is in the future. You should update it to the current year.
-/* Copyright 2025 Lambda +/* Copyright 2024 Lambda
152-152: Consider addressing the TODO comment.The comment indicates this section needs cleanup. Consider refactoring this event listener to improve clarity and maintainability.
173-183: Commented out functionality for drop callbacks.This large block of commented code suggests incomplete functionality for item drop handling. Either implement this feature, remove the commented code, or add a more detailed explanation about why it's disabled.
Consider adding a specific TODO issue to track this if it's a planned feature.
463-481: Consider making BreakType companion object method.The
nullify()method in the BreakType enum modifies state in the outer BreakManager object, which creates tight coupling. Consider refactoring this to make the enum more self-contained, perhaps by moving this functionality to a companion object method in BreakManager that takes a BreakType parameter.- fun nullify() = - when (this) { - Primary -> primaryBreakingInfo = null - Secondary -> secondaryBreakingInfo = null - else -> {} - } +// Add this to the companion object of BreakManager +fun nullifyBreakInfo(type: BreakType) { + when (type) { + BreakType.Primary -> primaryBreakingInfo = null + BreakType.Secondary -> secondaryBreakingInfo = null + else -> {} + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (7)
common/src/main/kotlin/com/lambda/interaction/request/breaking/BreakManager.kt (7)
90-90: Address the TODO comment.This TODO comment should be resolved or have a corresponding issue created to track this improvement task.
Is there a plan or timeline for improving the instamine/non-instamine integration? If this is a known limitation, consider adding more details to the TODO or creating a tracking issue.
254-261: LGTM: Initial block attack logic.The initial block attack logic correctly handles the case when breaking hasn't started yet, with proper return value handling and hand swinging according to configuration.
305-317: LGTM: Break progress threshold check.The logic for checking if breaking has reached the threshold is well implemented, with appropriate packet sending, hand swinging based on configuration, and cooldown setting.
407-408: Validate return value ofworld.setBlockState(...).When
setBlockStatereturns false, the old block remains intact. If this occurs, the code still proceeds withblock.onBroken(...). Confirm this mismatch doesn't lead to unintended states or partial break.+ if (!setState) { + this@BreakManager.warn("Failed to set new block state for ${ctx.expectedPos}") + return false + }
453-455: Prevent potential division by zero.The calculation of progress involves division by
breakConfig.breakThreshold. If this value is 0 (which could happen depending on configuration), it would result in a division by zero exception.- val progress = (breakDelta * breakingTicks) / breakConfig.breakThreshold + val threshold = breakConfig.breakThreshold.takeIf { it > 0 } ?: 1.0f + val progress = (breakDelta * breakingTicks) / threshold
483-492: LGTM: Clean event extension functions.The extension functions for handling break events are well-designed and provide a clear API for modules to listen to break events.
126-129:Details
✅ Verification successful
Potentially inefficient early return.
Using
return@listenwill exit the entire listener function rather than just the current loop iteration. If your intent is to only process one breaking info at a time, this is correct. Otherwise, consider using an explicit loop or a different control flow strategy.
🏁 Script executed:
#!/bin/bash # Look for other instances of early returns in listeners to understand the pattern rg -A 2 "return@listen" --type kotlinLength of output: 46811
Clarification on Early Return Behavior
After reviewing similar usage throughout the codebase, it’s clear that returning early with
return@listenis a common pattern designed to exit the listener immediately when a certain condition is met. In this instance, if the intent is to process only a single breaking info per event (i.e. exiting the listener once a valid case is found), then this pattern is consistent with other modules (e.g., in HotbarManager, RotationManager, etc.) and acceptable as is. However, if you expected to iterate over multiple breaking infos within the same listener call, consider refactoring the logic (for example, by iterating explicitly over each element) to avoid prematurely terminating the listener.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/src/main/kotlin/com/lambda/module/modules/player/HighwayTools.kt (1)
64-64: Consider aligning description and unit for sliceSize.There's a slight inconsistency between the description "Number of slices to build at once" and the unit "blocks". Since sliceSize represents a count rather than a physical dimension, consider either removing the "blocks" unit or adjusting the description to clarify how slices relate to blocks.
-private val sliceSize by setting("Slice Size", 3, 1..5, 1, "Number of slices to build at once", " blocks") { page == Page.Structure } +private val sliceSize by setting("Slice Size", 3, 1..5, 1, "Number of block sections to build at once") { page == Page.Structure }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/main/kotlin/com/lambda/module/modules/player/HighwayTools.kt(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Lambda
🔇 Additional comments (3)
common/src/main/kotlin/com/lambda/module/modules/player/HighwayTools.kt (3)
51-52: Improved clarity with detailed descriptions and units.The added descriptions and units for height and width parameters provide excellent context for users. These explanations clearly define what each measurement encompasses, making the configuration more intuitive.
54-54: Clear explanation of rimHeight parameter behavior.The description "Height of the pavements rims where 0 is none" effectively communicates both the purpose and the special case. This is particularly helpful for users to understand the effect of different values.
63-63: Added unit specification enhances clarity.Adding the "blocks" unit to the distance parameter maintains consistency with other measurements and clarifies how this value is interpreted in the game world.
|
You are on the CodeRabbit Free Plan. In order to use the Chat feature, please upgrade to CodeRabbit Pro. |
…ks for if a break info can be re broken and the act of re breaking
…t as the tool won't be present again to break the block
… breakable blocks too
…tiker/NeoLambda into feature/packetmine-rewrite
…ager to account for the new selected slot logic bypassing serverSlot
…ers selected slot / stack will return the hotbar managers selection if there's an active request. This is different compared to the way it worked in 1.20.4 as previously it only altered the interaction managers selected slot
>:( Yet another thing ruined...
Summary by CodeRabbit
New Features
ContainerSelectionclass for improved material container selection.BreakSettingsandPlaceSettingsclasses to manage breaking and placing configurations.Improvements
Refactor