Prop Bop #555
Prop Bop #555
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant updates to the Modern Warfare Cubed project, including a comprehensive rework of tooltips for weapons and attachments, new translations, and sound effects. It modifies existing functionalities such as fire mode hints and ammunition counters, aligns reloading mechanics with vanilla behavior, and overhauls textures and models. The internal structure is refined with updates to dependencies and build scripts. Several unused features are removed, and optimizations are made to enhance performance. Additionally, various fixes are implemented to address issues in crafting mappings and state management. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Outside diff range and nitpick comments (10)
src/main/java/com/paneedah/weaponlib/tile/CustomTileEntityBlock.java (4)
33-33: Prefer usingListoverArrayListin field declarationIt's recommended to use the
Listinterface instead of the concrete implementationArrayListfor thecustomBoundingBoxesfield. This promotes abstraction and allows for flexibility in changing the underlying list implementation without affecting the rest of the code.Apply this diff to update the field declaration:
-private Function<IBlockState, ArrayList<AxisAlignedBB>> customBoundingBoxes; +private Function<IBlockState, List<AxisAlignedBB>> customBoundingBoxes;
45-46: Update parameter type toListinsetBoundingBoxesmethodFor consistency and abstraction, consider changing the parameter type of the
setBoundingBoxesmethod toFunction<IBlockState, List<AxisAlignedBB>>.Apply this diff to update the method signature:
-public void setBoundingBoxes(Function<IBlockState, ArrayList<AxisAlignedBB>> customBoundingBoxes) { +public void setBoundingBoxes(Function<IBlockState, List<AxisAlignedBB>> customBoundingBoxes) { this.customBoundingBoxes = customBoundingBoxes; }
51-76: RefactorgetBoundingBoxmethod to reduce nested complexityThe
getBoundingBoxmethod has a nested complexity depth of 4, which can impact readability and maintainability. Consider refactoring to reduce nesting by extracting parts of the logic into helper methods or simplifying conditional structures.One possible refactor is to extract the inner loop into a separate method:
public AxisAlignedBB getBoundingBox(IBlockState state, IBlockAccess source, BlockPos pos) { if (customBoundingBoxes != null) { EnumFacing facing = state.getValue(FACING); List<AxisAlignedBB> boundingBoxes = customBoundingBoxes.apply(state); - if (!boundingBoxes.isEmpty()) { - AxisAlignedBB combinedBoundingBox = null; - for (AxisAlignedBB boundingBox : boundingBoxes) { - AxisAlignedBB orientedBoundingBox = AABBUtil.orientAABB(facing, boundingBox); - if (combinedBoundingBox == null) { - combinedBoundingBox = orientedBoundingBox; - } else { - combinedBoundingBox = combinedBoundingBox.union(orientedBoundingBox); - } - } - return combinedBoundingBox != null ? combinedBoundingBox : super.getBoundingBox(state, source, pos); - } + AxisAlignedBB combinedBoundingBox = getCombinedBoundingBox(boundingBoxes, facing); + return combinedBoundingBox != null ? combinedBoundingBox : super.getBoundingBox(state, source, pos); } return super.getBoundingBox(state, source, pos); } + +private AxisAlignedBB getCombinedBoundingBox(List<AxisAlignedBB> boundingBoxes, EnumFacing facing) { + if (boundingBoxes.isEmpty()) { + return null; + } + AxisAlignedBB combinedBoundingBox = null; + for (AxisAlignedBB boundingBox : boundingBoxes) { + AxisAlignedBB orientedBoundingBox = AABBUtil.orientAABB(facing, boundingBox); + if (combinedBoundingBox == null) { + combinedBoundingBox = orientedBoundingBox; + } else { + combinedBoundingBox = combinedBoundingBox.union(orientedBoundingBox); + } + } + return combinedBoundingBox; +}🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (next)
[warning] 51-76: ❌ New issue: Deep, Nested Complexity
getBoundingBox has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
153-153: UseTextComponentTranslationfor localization supportConsider using
TextComponentTranslationinstead ofTextComponentStringto allow the message to be localized, improving internationalization support.Apply this diff to update the message:
-placer.sendMessage(new TextComponentString("Invalid damage property. Please pick in [0, 1, 2]")); +placer.sendMessage(new TextComponentTranslation("message.invalid_damage_property"));Ensure that the key
"message.invalid_damage_property"is defined in the localization files with the appropriate translation.src/main/java/com/paneedah/mwc/equipment/Backpacks.java (2)
53-59: Consider adding inventory positioning for consistency.Several backpack builders lack inventory positioning configurations that are present in other backpacks. This inconsistency might affect how these backpacks are displayed in the inventory.
Consider adding inventory positioning similar to the combat sustainment backpack:
new ItemBackpack.Builder() .withName("assault_backpack_tan") .withSize(16) .withModel("AssaultBackpack") .withTexture("equipment/carryable/backpacks/assault_backpack_tan") + .withInventoryPositioning(() -> new Transform() + .withPosition(-0.15F, -4.6F, 0.35F) + .withRotation(18, -50, 0) + .withScale(3.3F, 3.3F, 3.3F) + .applyTransformations()) .build();Also applies to: 60-66, 67-73, 74-80, 81-87, 88-94, 95-101
Line range hint
12-101: Document backpack size differences.Different backpack types have varying sizes (10, 16, 20, 24 slots) which might not be immediately obvious to players or other developers.
Consider adding class-level documentation explaining the size tiers and their purpose:
+/** + * Backpack configurations with different storage capacities: + * - Combat Sustainment Backpack: 10 slots (Basic) + * - Assault Backpack: 16 slots (Medium) + * - TruSpec Cordura Backpack: 20 slots (Large) + * - Duffle Bag: 24 slots (Extra Large) + */ public class Backpacks {src/main/java/com/paneedah/weaponlib/tile/CustomTileEntityConfiguration.java (1)
88-102: Consider adding parameter validation.While the implementation is good, consider adding validation:
- Add null check for the AxisAlignedBB parameter
- Validate that dimensions are non-negative
public T withBoundingBox(double x1, double y1, double z1, double x2, double y2, double z2) { + if (x2 < x1 || y2 < y1 || z2 < z1) { + throw new IllegalArgumentException("Invalid bounding box dimensions"); + } boundingBoxes.add(new AxisAlignedBB(x1, y1, z1, x2, y2, z2)); return safeCast(this); } public T withBoundingBox(AxisAlignedBB axisAlignedBB) { + if (axisAlignedBB == null) { + throw new IllegalArgumentException("AxisAlignedBB cannot be null"); + } boundingBoxes.add(axisAlignedBB); return safeCast(this); }Changelog.md (1)
65-80: Remove duplicate entries in the props list.The following entries appear to be duplicated:
- "Gravestone Cross" appears multiple times with different ALT variations
- "Spooky Ghost" appears multiple times with different ALT variations
Consider consolidating these entries to improve readability:
- Gravestone Cross - Gravestone Cross (ALT 1) - Gravestone Cross (ALT 2) + Gravestone Cross (including ALT 1 & 2 variants) - Spooky Ghost - Spooky Ghost (ALT 1) - Spooky Ghost (ALT 2) + Spooky Ghost (including ALT 1 & 2 variants)🧰 Tools
🪛 LanguageTool
[duplication] ~66-~66: Possible typo: you repeated a word
Context: ...dded icons for the following props: - Gravestone - Gravestone Skull - Gravestone Cross - Gravesto...(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~68-~68: This phrase is duplicated. You should probably use “Gravestone Cross” only once.
Context: ... - Gravestone - Gravestone Skull - Gravestone Cross - Gravestone Cross (ALT 1) - Gravestone Cross (ALT 2) ...(PHRASE_REPETITION)
[duplication] ~71-~71: Possible typo: you repeated a word
Context: ...ALT 1) - Gravestone Cross (ALT 2) - Sandbag - Sandbag Wall - Spooky Ghost - Spooky Ghost ...(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~73-~73: This phrase is duplicated. You should probably use “Spooky Ghost” only once.
Context: ...ALT 2) - Sandbag - Sandbag Wall - Spooky Ghost - Spooky Ghost (ALT 1) - Spooky Ghost (ALT 2) - To...(PHRASE_REPETITION)
src/main/java/com/paneedah/mwc/tileentities/TileEntities.java (2)
20-20: Consider extracting common bounding box dimensions into constants.Many tile entities use similar bounding box dimensions. Consider extracting these into constants for better maintainability:
+private static final AxisAlignedBB FULL_BLOCK_AABB = new AxisAlignedBB(0, 0, 0, 1, 1, 1); +private static final AxisAlignedBB HALF_HEIGHT_AABB = new AxisAlignedBB(0, 0, 0, 1, 0.5, 1); +private static final AxisAlignedBB CHAIR_HEIGHT_AABB = new AxisAlignedBB(0, 0, 0, 1, 0.7, 1);Then use these constants:
-withBoundingBox(new AxisAlignedBB(0, 0, 0, 1, 1, 1)) +withBoundingBox(FULL_BLOCK_AABB)Also applies to: 34-34, 48-48, 62-62, 90-90, 104-104, 146-146, 160-160, 174-174, 229-229, 312-312, 327-327, 369-369, 384-384, 502-502, 529-529, 581-581, 595-595, 609-609, 624-624, 859-859, 938-938, 991-991, 1109-1109, 1123-1123, 1137-1137, 1150-1150, 1163-1163, 1177-1177, 1190-1190, 1204-1204, 1232-1232, 1325-1325
🧰 Tools
🪛 GitHub Check: CodeScene Cloud Delta Analysis (next)
[notice] 20-1325: ✅ Getting better: Large Method
createTileEntity decreases from 1412 to 1269 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
581-586: Adjust body bag positioning for better alignment.The body bag's bounding box and positioning seem slightly misaligned:
- Bounding box is full width (0 to 1)
- But scale is 0.9 and translation is offset
Consider adjusting the bounding box to match the visual model more precisely.-withBoundingBox(new AxisAlignedBB(0, 0, 0, 1, 0.4, 1)) +withBoundingBox(new AxisAlignedBB(0.1, 0, 0.1, 0.9, 0.4, 0.9))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (39)
src/main/resources/assets/mwc/models/item/barrier_rotated.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/crossgravestone.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/crossgravestone2.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/crossgravestone3.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/gravestone.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/gravestoneskull.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/home_chair_rotated.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/outdoor_chair_rotated.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/propanetank.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/radio_rotated.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/sandbag.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/sandbagwall.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/scarecrow.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/serverrackleftalt3.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/spookyghost.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/spookyghost2.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/spookyghost3.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/swat_truck.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/models/item/towablefloodlight.jsonis excluded by!**/*.jsonsrc/main/resources/assets/mwc/textures/items/barrier_rotated.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/gravestone.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/gravestone_cross.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/gravestone_cross_2.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/gravestone_cross_3.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/gravestone_skull.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/home_chair_rotated.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/outdoor_chair_rotated.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/prop_rotated.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/propane_tank.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/radio_rotated.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/sandbag.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/sandbag_wall.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/scarecrow.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/server_rack_left_alt_3.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/spooky_ghost.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/spooky_ghost_2.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/spooky_ghost_3.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/swat_truck.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/assets/mwc/textures/items/towable_floodlight.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (12)
Changelog.md(2 hunks)src/main/java/com/paneedah/mwc/creativetab/PropsTab.java(2 hunks)src/main/java/com/paneedah/mwc/equipment/Backpacks.java(4 hunks)src/main/java/com/paneedah/mwc/tileentities/TileEntities.java(33 hunks)src/main/java/com/paneedah/mwc/tileentities/TurretBaseFactory.java(2 hunks)src/main/java/com/paneedah/weaponlib/tile/CustomTileEntity.java(2 hunks)src/main/java/com/paneedah/weaponlib/tile/CustomTileEntityBlock.java(4 hunks)src/main/java/com/paneedah/weaponlib/tile/CustomTileEntityClassFactory.java(2 hunks)src/main/java/com/paneedah/weaponlib/tile/CustomTileEntityConfiguration.java(6 hunks)src/main/java/com/paneedah/weaponlib/tile/LootBoxConfiguration.java(5 hunks)src/main/java/com/paneedah/weaponlib/tile/LootBoxTileEntity.java(2 hunks)src/main/resources/assets/mwc/lang/en_US.lang(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/assets/mwc/lang/en_US.lang
🧰 Additional context used
🪛 GitHub Check: CodeScene Cloud Delta Analysis (next)
src/main/java/com/paneedah/mwc/tileentities/TileEntities.java
[notice] 20-1325: ✅ Getting better: Large Method
createTileEntity decreases from 1412 to 1269 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
[notice] 20-1325: ✅ Getting better: Large Method
createTileEntity decreases from 1412 to 1269 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
src/main/java/com/paneedah/weaponlib/tile/CustomTileEntityBlock.java
[warning] 51-76: ❌ New issue: Deep, Nested Complexity
getBoundingBox has a nested complexity depth of 4, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
🪛 LanguageTool
Changelog.md
[duplication] ~66-~66: Possible typo: you repeated a word
Context: ...dded icons for the following props: - Gravestone - Gravestone Skull - Gravestone Cross - Gravesto...
(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~68-~68: This phrase is duplicated. You should probably use “Gravestone Cross” only once.
Context: ... - Gravestone - Gravestone Skull - Gravestone Cross - Gravestone Cross (ALT 1) - Gravestone Cross (ALT 2) ...
(PHRASE_REPETITION)
[duplication] ~71-~71: Possible typo: you repeated a word
Context: ...ALT 1) - Gravestone Cross (ALT 2) - Sandbag - Sandbag Wall - Spooky Ghost - Spooky Ghost ...
(ENGLISH_WORD_REPEAT_RULE)
[grammar] ~73-~73: This phrase is duplicated. You should probably use “Spooky Ghost” only once.
Context: ...ALT 2) - Sandbag - Sandbag Wall - Spooky Ghost - Spooky Ghost (ALT 1) - Spooky Ghost (ALT 2) - To...
(PHRASE_REPETITION)
🔇 Additional comments (11)
src/main/java/com/paneedah/mwc/tileentities/TurretBaseFactory.java (3)
7-7: LGTM! Import addition is appropriate.
The explicit import of AxisAlignedBB aligns with the PR's goal of improving AABB support.
Line range hint 21-23: Verify if the commented GL transformations are still needed.
There are commented-out GL transformations for scaling and rotation. Given that the PR mentions support for rotated props, we should verify if these transformations are still relevant.
Let's check for similar patterns in other prop configurations:
19-19: Verify collision behavior with the 0.2 height bounding box.
The change to use AxisAlignedBB is good and aligns with the PR objectives. However, the low height (0.2) of the bounding box might affect player interaction or collision detection.
Let's check if other turret bases use similar dimensions:
✅ Verification successful
Bounding box height of 0.2 is consistent with other base entities
The height of 0.2 for the turret base's bounding box is appropriate and consistent with similar entities in the codebase:
- Other flat/base entities use similar low heights:
- Another turret base uses exactly 0.2 height:
new AxisAlignedBB(0, 0, 0, 1, 0.2, 1) - Ammo press base uses 0.2 height:
COLLISION_BOX_BOTTOM = new AxisAlignedBB(0.1D, 0.0D, 0.1D, 0.9D, 0.2D, 0.9D) - Various other base entities use heights between 0.2-0.5 for their collision boxes
- Another turret base uses exactly 0.2 height:
The low height is intentional for base components and won't negatively impact collision detection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other turret-related bounding box definitions
rg -A 2 "withBoundingBox.*turret"
Length of output: 35
Script:
#!/bin/bash
# Let's search for all bounding box definitions to understand the common patterns
rg -A 2 "withBoundingBox\("
# Also search for any AxisAlignedBB usage
rg -A 2 "AxisAlignedBB\("
Length of output: 43624
src/main/java/com/paneedah/weaponlib/tile/CustomTileEntity.java (1)
20-20: Use of Lombok annotations for getter and setter
The application of @Getter and @Setter annotations on the side field effectively reduces boilerplate code and improves readability. Ensure that Lombok is properly configured in the project's build process.
src/main/java/com/paneedah/weaponlib/tile/CustomTileEntityClassFactory.java (1)
35-35: Adding Lombok @Getter for the instance field
Using @Getter on the instance field simplifies access to the singleton instance of CustomTileEntityClassFactory. Ensure that Lombok is properly set up in the project to generate the necessary methods.
src/main/java/com/paneedah/weaponlib/tile/LootBoxTileEntity.java (2)
45-45: Sound handling improvements look good.
The changes to use dedicated sound event getters improve code clarity and maintain consistency with the configuration class.
Also applies to: 69-69
41-43: Verify logging level change impact.
The change from info to debug logging level might affect troubleshooting in production environments.
✅ Verification successful
Let me gather more context about the logging patterns in this codebase.
Based on the analysis of the logging patterns in the codebase:
Logging level change is appropriate
The change from info to debug for item dropping is consistent with the codebase's logging patterns:
- Debug level is used throughout the codebase for similar operational events (state changes, transitions, etc.)
- Info level is primarily used for registration and initialization events
- Error/warn levels are properly used for actual issues and warnings
The change aligns with standard logging practices where:
- Debug = Detailed information for debugging
- Info = Notable but normal operations
- Warn/Error = Issues requiring attention
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if debug logging is used elsewhere for similar operations
rg "LOGGER\.(debug|info)\s*\(\s*\"Dropping" -A 2
Length of output: 416
Script:
#!/bin/bash
# Check overall logging patterns in the codebase
rg "LOGGER\.(debug|info|warn|error)" -A 1
# Also check if this logger is a static field
ast-grep --pattern 'private static final Logger LOGGER'
Length of output: 110042
src/main/java/com/paneedah/weaponlib/tile/LootBoxConfiguration.java (2)
6-7: Good use of Lombok for reducing boilerplate.
The addition of Lombok annotations improves code maintainability by reducing boilerplate getter methods.
Also applies to: 97-97, 99-99, 104-105
173-174: Verify sound registration across the mod.
Ensure that the sound registration is consistent with other sound events in the mod.
✅ Verification successful
Sound registration is consistent across the mod
The sound registration pattern in LootBoxConfiguration.java follows the same consistent pattern used throughout the codebase:
- All sound registrations use
modContext.registerSound()orcontext.registerSound() - Sound events are properly stored in private SoundEvent fields
- Registration happens during initialization/configuration phase
The implementation aligns with other sound registrations found in:
- Weapons (
Weapon.java) - Vehicles (
EntityVehicleConfiguration.java,GeneralVehicleSounds.java) - Grenades (
ItemGrenade.java) - Armor (
CustomArmor.java) - Melee weapons (
ItemMelee.java)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check sound registration patterns across the codebase
rg "registerSound\(" -A 1
# Check sound event field declarations
ast-grep --pattern 'private SoundEvent $name;'
Length of output: 24966
src/main/java/com/paneedah/weaponlib/tile/CustomTileEntityConfiguration.java (1)
18-19: LGTM! Good initialization of the bounding boxes collection.
The changes properly introduce support for multiple bounding boxes with appropriate imports and field initialization.
Also applies to: 38-38
src/main/java/com/paneedah/mwc/tileentities/TileEntities.java (1)
Line range hint 1325-1330: Verify the tall bounding box for the scarecrow.
The scarecrow's bounding box height of 3.5 blocks seems unusually tall and might cause issues with player interaction or collision detection. Consider verifying if this height is intentional and necessary.
✅ Verification successful
These commands will help us understand:
- Other bounding box dimensions in the codebase for comparison
- Locate scarecrow-related files for context
- Find any explicit height configurations for the scarecrow
The scarecrow's height of 3.5 blocks is intentionally tall and consistent with its purpose.
Looking at the bounding boxes across the codebase:
- Most decorative objects have heights between 0.2 to 1.0 blocks
- Only two entities have notably tall bounding boxes:
- Impaled body at 2.8 blocks
- Scarecrow at 3.5 blocks
The scarecrow's height is appropriate because:
- Scarecrows are traditionally tall structures meant to be visible from a distance
- The narrow width (0.3 to 0.7) balances the height, creating a pole-like structure
- The model's texture height of 150 pixels supports the tall design
- The 1:1 scale factor in positioning (glScalef(1f, 1f, 1f)) confirms the intended size
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other tall entities in the codebase for comparison
ast-grep --pattern 'withBoundingBox($$$new AxisAlignedBB($_, $_, $_, $_, $height, $_)$$$)' | grep -B 2 -A 2 'height.*[2-9]'
Length of output: 125
Script:
#!/bin/bash
# Search for bounding box definitions with specific focus on height values
rg -B 2 -A 2 "AxisAlignedBB.*\d+\.\d+" src/
Length of output: 19353
Script:
#!/bin/bash
# Look for scarecrow-related files and configurations
fd scarecrow -t f
Length of output: 248
Script:
#!/bin/bash
# Search for any model or entity height configurations
rg -i "height|tall" src/ | grep -i "scarecrow"
Length of output: 128
src/main/java/com/paneedah/weaponlib/tile/CustomTileEntityConfiguration.java
Show resolved
Hide resolved
Qodana for JVM6 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at qodana-support@jetbrains.com
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/main/java/com/paneedah/mwc/tileentities/TileEntities.java (2)
Line range hint
859-864: Fix incorrect bounding box offset for white_desk_middleThe bounding box starts at Y=0.5, which creates a floating collision box. This could cause gameplay issues where items fall through the top half of the desk.
-.withBoundingBox(new AxisAlignedBB(0, 0.5, 0, 1, 1, 1)) +.withBoundingBox(new AxisAlignedBB(0, 0, 0, 1, 0.8, 1))
Line range hint
624-629: Fix asymmetric bounding box for dumpsterThe bounding box extends into negative X coordinates (-1 to 1) while maintaining standard Z coordinates (0 to 1). This asymmetry could cause:
- Collision detection issues
- Unexpected behavior when placing the dumpster
- Problems with adjacent block interactions
-.withBoundingBox(new AxisAlignedBB(-1, 0, 0, 1, 1, 1)) +.withBoundingBox(new AxisAlignedBB(0, 0, 0, 1, 1, 1))
🧹 Nitpick comments (3)
src/main/java/com/paneedah/mwc/tileentities/TileEntities.java (3)
1133-1136: Inconsistent naming convention for flood light variantsThe naming convention is inconsistent between flood light variants:
- "duelfloodlight" uses lowercase and no spacing
- "floodLight" uses camelCase
Standardize the naming convention. Suggest using camelCase for all variants:
-.withName("duelfloodlight") +.withName("dualFloodLight")Also applies to: 1146-1149
Line range hint
1-1400: Consider implementing support for multiple AABBsThe PR objectives mention adding support for multiple Axis-Aligned Bounding Boxes (AABBs), but the current implementation only uses single AABB instances. This could limit the accuracy of collision detection for complex props.
Consider:
- Extending
LootBoxConfigurationto support multiple bounding boxes- Adding a method like
withBoundingBoxes(List<AxisAlignedBB>)- Implementing compound collision detection for props with complex shapes
This would improve collision accuracy for props like:
- The forklift (which has a mast and forks)
- The server racks (which have protruding components)
- The vending machine (which has a dispensing area)
Line range hint
1-1400: Reduce code duplication in prop configurationsThere's significant duplication in the prop configurations, particularly in:
- Material definitions
- Creative tab assignments
- Scale and rotation transformations
Consider introducing factory methods or builder patterns to reduce duplication:
public class PropConfigurationFactory { public static LootBoxConfiguration createStandardProp(String name, String modelClass) { return new LootBoxConfiguration() .withMaterial(Material.IRON) .withCreativeTab(MWC.PROPS_TAB) .withName(name) .withModelClassName(modelClass); } public static LootBoxConfiguration createRotatedProp(String name, String modelClass) { return createStandardProp(name, modelClass) .withPositioning(tileEntity -> { GL11.glScalef(1f, 1f, 1f); GL11.glTranslatef(0.5f, 0f, 0.5f); GL11.glRotatef(-90F, 0f, 1f, 0f); }); } }
| .withBoundingBox(new AxisAlignedBB(0, 0, 0, 1, 0.4, 1)) | ||
| .withPositioning(tileEntity -> { | ||
| GL11.glScalef(0.9f, 0.9f, 0.9f); | ||
| GL11.glTranslatef(0.5f, -0.9f, 0.55f); | ||
| GL11.glTranslatef(0.5f, 0.1f, 0.55f); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust bounding box height for body_bag
The current bounding box height of 0.4 blocks seems too high for a body bag that should be relatively flat on the ground. Consider reducing the height to improve realism and prevent floating effects.
-.withBoundingBox(new AxisAlignedBB(0, 0, 0, 1, 0.4, 1))
+.withBoundingBox(new AxisAlignedBB(0, 0, 0, 1, 0.2, 1))📝 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.
| .withBoundingBox(new AxisAlignedBB(0, 0, 0, 1, 0.4, 1)) | |
| .withPositioning(tileEntity -> { | |
| GL11.glScalef(0.9f, 0.9f, 0.9f); | |
| GL11.glTranslatef(0.5f, -0.9f, 0.55f); | |
| GL11.glTranslatef(0.5f, 0.1f, 0.55f); | |
| .withBoundingBox(new AxisAlignedBB(0, 0, 0, 1, 0.2, 1)) | |
| .withPositioning(tileEntity -> { | |
| GL11.glScalef(0.9f, 0.9f, 0.9f); | |
| GL11.glTranslatef(0.5f, 0.1f, 0.55f); |
📝 Description
Greatly improves the current props. Also adds the basics of support more than one AABB.
🎯 Goals
❌ Non Goals
🚦 Testing
Build mod, join world, observe new props
⏮️ Backwards Compatibility
No issues with old saves
📚 Related Issues & Documents
#341
🖼️ Screenshots/Recordings
📖 Added to documentation?