Conversation
WalkthroughThe changes update the MSRHandler annotation interface by requiring an explicit value for the Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant DBR as DirectionalBlockRule
participant DU as DirectionUtils
Player->>DBR: Initiate block placement (PlacementState)
DBR->>DU: getDirection(player.yaw)
DU-->>DBR: Return cardinal direction
DBR-->>Player: Place block with proper facing
✨ Finishing Touches
🪧 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: 1
🔭 Outside diff range comments (1)
src/main/java/dev/nateweisz/msr/rules/SignBlockRule.java (1)
26-28:⚠️ Potential issueFix potential null pointer and incomplete wall sign handling.
The block assignment needs null initialization and the TODO should be addressed.
- // TODO: place on the wall, should we make dif handlers for the dif sign types. - block = this.block; + else { + // Handle wall signs + block = this.block.withProperty("facing", DirectionUtils.getDirection(placement.playerPosition().yaw())); + }
🧹 Nitpick comments (3)
src/main/java/dev/nateweisz/msr/utils/DirectionUtils.java (1)
4-14: Add input validation and documentation.The direction calculation logic is correct, but could benefit from these improvements:
- Add input validation for yaw range
- Document why yaw is adjusted by 180
public class DirectionUtils { + /** + * Converts a Minecraft yaw angle to a cardinal direction. + * Yaw is adjusted by 180 degrees to align with Minecraft's coordinate system where: + * 0° = South, 90° = West, 180° = North, 270° = East + * + * @param yaw Player's yaw angle in degrees (-180 to 180) + * @return Cardinal direction as a string: "north", "east", "south", or "west" + * @throws IllegalStateException if the calculation results in an unexpected value + */ public static String getDirection(float yaw) { + if (yaw < -180 || yaw > 180) { + throw new IllegalArgumentException("Yaw must be between -180 and 180 degrees"); + } + yaw += 180; return switch (Math.round(yaw / 90) % 4) {src/main/java/dev/nateweisz/msr/rules/DirectionalBlockRule.java (1)
15-17: Consider using an enum for block types.The regex predicate works but could be more maintainable using an enum to list supported block types.
+public enum DirectionalBlockType { + FURNACE(".*furnace"), + SMOKER("smoker"), + TABLE(".*table"), + BREWING_STAND("brewing_stand"), + ANVIL("anvil"), + GRINDSTONE("grindstone"), + LECTERN("lectern"), + STONECUTTER("stonecutter"), + BARREL("barrel"); + + private final String pattern; + + DirectionalBlockType(String pattern) { + this.pattern = pattern; + } + + public static String getPredicatePattern() { + return Arrays.stream(values()) + .map(type -> type.pattern) + .collect(Collectors.joining("|", "^(", ")$")); + } +} @MSRHandler( - predicate = "^(smoker|.*furnace|.*table|brewing_stand|anvil|grindstone|lectern|stonecutter|barrel)$" + predicate = DirectionalBlockType.getPredicatePattern() )src/main/java/dev/nateweisz/msr/rules/SignBlockRule.java (1)
33-37: Consider using DirectionUtils for rotation calculation.The rotation calculation could be unified with DirectionUtils to maintain consistency.
private static int calculateRotation(float yaw) { - yaw += 180; // convert it to all positive yaw values - - return Math.round(yaw / 22.5f) % 16; + // Convert cardinal direction to sign rotation (16 possible values) + String direction = DirectionUtils.getDirection(yaw); + float baseRotation = switch (direction) { + case "north" -> 0; + case "east" -> 4; + case "south" -> 8; + case "west" -> 12; + default -> throw new IllegalStateException("Invalid direction: " + direction); + }; + + // Fine-tune rotation within the cardinal direction + float fineRotation = (yaw + 180) % 90 / 22.5f; + return Math.round(baseRotation + fineRotation) % 16; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/src/main/java/dev/nateweisz/msr/MSRHandler.java(1 hunks)src/main/java/dev/nateweisz/msr/rules/DirectionalBlockRule.java(1 hunks)src/main/java/dev/nateweisz/msr/rules/SignBlockRule.java(2 hunks)src/main/java/dev/nateweisz/msr/utils/DirectionUtils.java(1 hunks)
🔇 Additional comments (1)
api/src/main/java/dev/nateweisz/msr/MSRHandler.java (1)
13-13:Details
✅ Verification successful
Verify impact of removing default predicate value.
The change to require explicit predicate values improves code clarity. However, since this is a breaking change, ensure all existing MSRHandler usages are updated.
🏁 Script executed:
#!/bin/bash # Description: Find all MSRHandler usages that might need updates. # Test: Search for MSRHandler annotations without predicate values. rg -A 5 '@MSRHandler\s*\(\s*\)' || echo "No MSRHandler annotations without predicate values found."Length of output: 158
Explicit predicate value change verified as safe.
The removal of the default predicate value is confirmed to be safe because all MSRHandler annotations now include explicit predicate values. No instances were found where default values were used, so no additional updates are necessary.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/dev/nateweisz/msr/rules/DirectionalBlockRule.java (2)
11-14: Fix documentation - duplicate entry.The documentation lists "crafting table" twice in the block types this handler supports.
/** * Handler for the following types, all furnace types, crafting table, enchantment table, brewing stand, anvil, - * crafting table, grindstone, lectern, smithing table, stonecutter, and barrel. + * grindstone, lectern, smithing table, stonecutter, and barrel. */
29-32: Unnecessary null check for direction.The
DirectionUtils.getDirection()method never returns null - it throws anIllegalStateExceptionfor unexpected values. Therefore, this null check is unnecessary and can be removed.String direction = DirectionUtils.getDirection(placement.playerPosition().yaw()); -if (direction == null) { - return null; -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/dev/nateweisz/msr/rules/DirectionalBlockRule.java(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/main/java/dev/nateweisz/msr/rules/DirectionalBlockRule.java (2)
src/main/java/dev/nateweisz/msr/rules/SignBlockRule.java (1)
MSRHandler(10-38)src/main/java/dev/nateweisz/msr/utils/DirectionUtils.java (1)
DirectionUtils(3-15)
🔇 Additional comments (1)
src/main/java/dev/nateweisz/msr/rules/DirectionalBlockRule.java (1)
34-34:Details
✅ Verification successful
Verify block property compatibility.
Ensure all block types listed in the predicate support the "facing" property. If some blocks use a different property name for direction, consider implementing type-specific property setting.
🏁 Script executed:
#!/bin/bash # Try to find documentation or usage of block properties to confirm compatibility # Search for examples of different block types using "facing" property # Look for block property definitions rg -A 5 "class.*Block" --type java # Search for usages of withProperty with "facing" rg "withProperty\\(\"facing\"" --type javaLength of output: 1304
Action: Confirm Block Property Compatibility
The search confirms that the only usage of
withProperty("facing", …)is in theDirectionalBlockRule. No alternative property names have been detected in similar block classes (e.g., inSignBlockRule). Please ensure that all block types referenced in your predicates indeed support the"facing"property. If a block type employs a different property name for direction, consider implementing type‐specific property handling.
| public @Nullable Block blockPlace(@NotNull PlacementState placement) { | ||
| if (placement == null || placement.playerPosition() == null) { | ||
| return null; | ||
| } | ||
|
|
||
| String direction = DirectionUtils.getDirection(placement.playerPosition().yaw()); | ||
| if (direction == null) { | ||
| return null; | ||
| } | ||
|
|
||
| return block.withProperty("facing", direction); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding exception handling.
The DirectionUtils.getDirection() method can throw an IllegalStateException for unexpected values. This exception should be caught and handled to prevent crashes.
@Override
public @Nullable Block blockPlace(@NotNull PlacementState placement) {
if (placement == null || placement.playerPosition() == null) {
return null;
}
- String direction = DirectionUtils.getDirection(placement.playerPosition().yaw());
- if (direction == null) {
- return null;
- }
-
- return block.withProperty("facing", direction);
+ try {
+ String direction = DirectionUtils.getDirection(placement.playerPosition().yaw());
+ return block.withProperty("facing", direction);
+ } catch (IllegalStateException e) {
+ // Log the exception if you have a logging framework
+ // logger.error("Failed to determine direction from yaw", e);
+ return null;
+ }
}📝 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.
| public @Nullable Block blockPlace(@NotNull PlacementState placement) { | |
| if (placement == null || placement.playerPosition() == null) { | |
| return null; | |
| } | |
| String direction = DirectionUtils.getDirection(placement.playerPosition().yaw()); | |
| if (direction == null) { | |
| return null; | |
| } | |
| return block.withProperty("facing", direction); | |
| } | |
| @Override | |
| public @Nullable Block blockPlace(@NotNull PlacementState placement) { | |
| if (placement == null || placement.playerPosition() == null) { | |
| return null; | |
| } | |
| try { | |
| String direction = DirectionUtils.getDirection(placement.playerPosition().yaw()); | |
| return block.withProperty("facing", direction); | |
| } catch (IllegalStateException e) { | |
| // Log the exception if you have a logging framework | |
| // logger.error("Failed to determine direction from yaw", e); | |
| return null; | |
| } | |
| } |
1 participant
This PR adds a generic block rule that applies to various blocks
Summary by CodeRabbit
New Features
Refactor