Revamped the check for side invisibility. #51
Conversation
|
|
WalkthroughThe pull request introduces significant refactoring to the glass-related block classes in the Wurst Client project. The primary changes involve modifying the inheritance structure, simplifying visibility logic, and streamlining method implementations across multiple glass block types. The modifications affect Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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: 0
🧹 Nitpick comments (9)
src/main/java/net/wurstclient/glass/GlassSlabBlock.java (1)
32-42: Consolidate multipleifchecks into a unified approach.Currently, the method checks each related block type individually. Consider a shared property or a single condition to handle all glass-like blocks. This avoids repetitive
ifstatements and simplifies maintenance.public boolean isSideInvisible(BlockState state, BlockState stateFrom, Direction direction) { - if(stateFrom.getBlock() == Blocks.GLASS) - return true; - - if(stateFrom.getBlock() == this && isInvisible(state, stateFrom, direction)) - return true; - - if(stateFrom.getBlock() == MoGlassBlocks.GLASS_STAIRS && isInvisible(state, stateFrom, direction)) - return true; + if (isGlassVariant(stateFrom.getBlock()) && isInvisible(state, stateFrom, direction)) + return true; return super.isSideInvisible(state, stateFrom, direction); }Add a helper method,
isGlassVariant(Block block), to handle all glass-like blocks comprehensively.src/main/java/net/wurstclient/glass/StainedGlassStairsBlock.java (1)
39-40: Avoid code duplication when dealing with similarly typed blocks.Calling
isInvisiblefor slabs is valid, but multiple repeated checks for each block type can bloat the code. Consider a single method or shared helper to unify these checks.src/main/java/net/wurstclient/glass/TintedGlassSlabBlock.java (1)
33-39: Consider making the code more concise and consistent.The spacing between conditions is inconsistent. Consider this more concise version:
- if(stateFrom.getBlock() == this && isInvisible(state, stateFrom, direction)) - return true; - - if(stateFrom.getBlock() == MoGlassBlocks.TINTED_GLASS_STAIRS && isInvisible(state, stateFrom, direction)) - return true; - + if(isInvisible(state, stateFrom, direction) + && (stateFrom.getBlock() == this + || stateFrom.getBlock() == MoGlassBlocks.TINTED_GLASS_STAIRS)) + return true; +src/main/java/net/wurstclient/glass/mixin/GlassBlockMixin.java (1)
39-40: Consider merging these checks for maintainability.
The repeated pattern for checkingGLASS_SLABand then callingisInvisiblemay be consolidated with the checks in lines 42-43 to reduce duplication:- if(stateFrom.getBlock() == MoGlassBlocks.GLASS_SLAB && isInvisible(state, stateFrom, direction)) - return true; - if(stateFrom.getBlock() == MoGlassBlocks.GLASS_STAIRS && isInvisible(state, stateFrom, direction)) - return true; + if ((stateFrom.getBlock() == MoGlassBlocks.GLASS_SLAB + || stateFrom.getBlock() == MoGlassBlocks.GLASS_STAIRS) + && isInvisible(state, stateFrom, direction)) + { + return true; + }src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java (1)
60-66: Geometry-based check is more robust.Using
getOutlineShapeandVoxelShapes.isSideCoveredis a cleaner, more scalable mechanism than type checks for slabs/stairs. Consider caching or reusing shapes if performance becomes an issue in heavily populated worlds.src/main/java/net/wurstclient/glass/GlassStairsBlock.java (1)
32-42: Replace repetitive checks with a combined logic.You have multiple checks for specific block types (
GLASS_SLABandthis). Combining them into a single conditional block can reduce duplication and improve readability.Consider applying this diff:
public boolean isSideInvisible(BlockState state, BlockState stateFrom, Direction direction) { if (stateFrom.getBlock() == Blocks.GLASS) return true; - if(stateFrom.getBlock() == MoGlassBlocks.GLASS_SLAB) - if(isInvisible(state, stateFrom, direction)) - return true; - - if(stateFrom.getBlock() == this) - if(isInvisible(state, stateFrom, direction)) - return true; + if (stateFrom.getBlock() == MoGlassBlocks.GLASS_SLAB + || stateFrom.getBlock() == this) + { + if (isInvisible(state, stateFrom, direction)) + return true; + } return super.isSideInvisible(state, stateFrom, direction); }src/main/java/net/wurstclient/glass/TintedGlassStairsBlock.java (2)
27-31: Immediate invisibility check for vanilla tinted glass.
Returningtruestraightaway forBlocks.TINTED_GLASSmakes sense to handle any direct adjacency to vanilla tinted glass. If you anticipate adding more tinted variants, consider consolidating these checks into a shared helper method for easier maintainability.
32-37: Unify repeated checks for tinted glass blocks.
These conditions forMoGlassBlocks.TINTED_GLASS_SLAB, plus the check forthisclass, seem modular, but could be refactored to reduce repetition. For example, a single method could handle the invisibility logic for the entire tinted glass family.src/main/java/net/wurstclient/glass/mixin/TintedGlassBlockMixin.java (1)
47-53: Fix method parameter formatting and approve implementation.The new geometric approach using
VoxelShapes.isSideCoveredis a more reliable way to check visibility. However, there's inconsistent indentation in the method parameters.Apply this formatting fix:
- private boolean isInvisible(BlockState state, - BlockState stateFrom, Direction direction) + private boolean isInvisible(BlockState state, BlockState stateFrom, + Direction direction)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/net/wurstclient/glass/GlassSlabBlock.java(2 hunks)src/main/java/net/wurstclient/glass/GlassStairsBlock.java(1 hunks)src/main/java/net/wurstclient/glass/StainedGlassSlabBlock.java(2 hunks)src/main/java/net/wurstclient/glass/StainedGlassStairsBlock.java(2 hunks)src/main/java/net/wurstclient/glass/TintedGlassSlabBlock.java(2 hunks)src/main/java/net/wurstclient/glass/TintedGlassStairsBlock.java(1 hunks)src/main/java/net/wurstclient/glass/mixin/GlassBlockMixin.java(2 hunks)src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java(3 hunks)src/main/java/net/wurstclient/glass/mixin/TintedGlassBlockMixin.java(2 hunks)
🔇 Additional comments (37)
src/main/java/net/wurstclient/glass/GlassSlabBlock.java (3)
21-21: No issues with the new import.The import of
EmptyBlockViewis necessary for shape rendering logic. Good use of the built-in singleton.
23-23: Removal offinalis understandable but ensure consistency across the hierarchy.Converting
GlassSlabBlockfromfinalto non-final signals subclass intent. Please verify that any subclasses correctly override the newisInvisiblemethod.
46-49: Verify the usage ofEmptyBlockViewfor shape calculation.Using
EmptyBlockView.INSTANCEand a fixed position (BlockPos.ORIGIN) may not account for position-specific shape differences. This is likely acceptable for glass side invisibility, but confirm that no block-level orientation or context is lost.If necessary, consider using the actual
BlockViewand block coordinates if future features require more complex shape calculations.src/main/java/net/wurstclient/glass/StainedGlassStairsBlock.java (6)
18-18: Switching fromStairsBlocktoGlassStairsBlocklooks good.This aligns with the unified approach to handle glass rendering logic, reducing duplication across classes. Marking as approved.
25-25: Constructor call simplified.Removing the previous block state parameter is consistent with inheriting from
GlassStairsBlock; no issues observed.
36-36: Same-colored stained glass is correctly marked invisible.Ensuring visibility checks on matching stained glass color is appropriate and matches the user expectation.
43-43: Verify reference tothisinstead of color matching.Using
blockFrom == thismight be acceptable here, but if you intend to allow multiple block variants that share the same color or logic, comparing classes or color attributes might be more flexible.
48-48: No content change.No new code or comment here.
56-56: Color getter is straightforward.Method is concise and correctly returns the stored color.
src/main/java/net/wurstclient/glass/TintedGlassSlabBlock.java (3)
Line range hint
19-25: LGTM! Clean inheritance structure.The class properly extends
GlassSlabBlockand maintains immutability with thefinalmodifier. The constructor implementation is correct and minimal.
28-40: LGTM! Clear and logical visibility checks.The implementation correctly handles all three cases:
- Full tinted glass blocks
- Tinted glass slabs
- Tinted glass stairs
The reuse of the parent's
isInvisiblemethod for complex shape checks is a good practice.
Line range hint
42-60: LGTM! Correct light and opacity handling.The methods properly implement the expected behavior for tinted glass:
- Maximum ambient occlusion light level
- Non-transparent block
- Correct opacity values for single and double slabs
src/main/java/net/wurstclient/glass/mixin/GlassBlockMixin.java (6)
11-11: Good use of the@Uniqueannotation.
This annotation helps ensure that the declared element doesn’t collide with existing members in the class being mixed into.
17-17: These imports align well with the introduced culling logic.
All four imports (BlockPos,VoxelShape,VoxelShapes, andEmptyBlockView) are necessary for the new shape-based side invisibility approach.Also applies to: 19-19, 20-20, 21-21
28-28: Constructor parameter removal is appropriate.
Removing the unusedMoGlassparameter keeps the constructor clean and clearly indicates that any dependency onMoGlassis no longer needed here.
42-43: Duplicate logic noted.
This is effectively the same logic as the slab check above, reinforcing the suggestion to unify these conditions.
49-50: Proper use of a dedicated private Mixin method.
Marking this helper as@Uniqueensures it’s restricted to this Mixin and reduces collision risks.
52-54: 🛠️ Refactor suggestionVerify behavior with partial or custom block shapes.
isSideCoveredworks well for ordinary blocks, but partial shapes or custom geometry might produce corner cases. Consider testing or adding fallback logic for blocks that don’t fully occupy their bounding box.src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java (4)
13-13: Use of@Uniqueannotation is correct.This annotation helps prevent naming conflicts within Mixin classes and keeps the method scoped to this class only.
20-25: Imports align with new geometry-based logic.Bringing in
BlockPos,VoxelShape,VoxelShapes, andEmptyBlockViewis consistent with the expanded usage of geometric checks for block invisibility.
49-54: Early return improves readability.The quick return from
isInvisible(...)is a neat approach, reducing nested if conditions and making it clear when the side is indeed invisible.
36-36: Verify removal ofMoGlassdependency.By removing the
MoGlassparameter, ensure no external calls or references in other files depend on this constructor. This change may break code that calls the old constructor.✅ Verification successful
Constructor modification is safe ✅
The constructor change is safe as this is a Mixin class (annotated with
@Mixin) that isn't directly instantiated. The constructor is private and the class is abstract, serving only as a template for runtime code injection intoStainedGlassBlock.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if references to the old constructor still exist ast-grep --pattern $'new StainedGlassBlockMixin($_, $$$)'Length of output: 58
Script:
#!/bin/bash # Check if it's a Mixin class and look for any references echo "=== Checking Mixin annotation ===" rg "@Mixin" "src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java" echo -e "\n=== Looking for any references to StainedGlassBlockMixin ===" rg "StainedGlassBlockMixin" --type java echo -e "\n=== Looking for potential constructor patterns ===" ast-grep --pattern 'StainedGlassBlockMixin($$$)'Length of output: 799
src/main/java/net/wurstclient/glass/GlassStairsBlock.java (4)
21-21: Looks good!The new import for
EmptyBlockViewis appropriate for retrieving outline shapes without a full world context.
23-23: Class no longer marked final.Making this class extendable is reasonable if you plan on letting other blocks or mods build upon its functionality. Just ensure that any subclassing remains aligned with the overall design.
25-25: Constructor changed from protected to package-private.This is fine if your design requires allowing creation of
GlassStairsBlockwithin other classes in the same package. Verify that external code needing this constructor is also in the same package.
48-53: Shape-based invisibility check looks correct.Using
EmptyBlockView.INSTANCEto retrieve outline shapes and then leveragingVoxelShapes.isSideCoveredis a concise approach. However, confirm it gracefully handles special cases (e.g., tinted or stained glass with dynamic shapes).Run the following script to locate references and verify that no unexpected shape overrides interfere with this logic:
✅ Verification successful
Shape-based invisibility check is verified as correct and handles all glass variants uniformly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching codebase for references to shape overrides in tinted/stained glass rg -A 5 "getOutlineShape"Length of output: 3541
src/main/java/net/wurstclient/glass/TintedGlassStairsBlock.java (3)
18-18: ExtendingGlassStairsBlockfor consistency with other glass blocks.
This change appears intentional and aligns with the mod’s approach for glass-based blocks.
20-22: Constructor transition to usesettingsdirectly.
Calling the parent constructor withsettingsis the correct approach and ensures all relevant block properties are inherited properly.
40-40: No issues detected.
The additional blank line / spacing change does not impact functionality.src/main/java/net/wurstclient/glass/StainedGlassSlabBlock.java (4)
21-21: Reusing the glass invisibility logic.Switching the parent class to
GlassSlabBlockis a clear improvement that ensures consistent invisibility checks across all glass-related slabs. This also reduces code duplication.
49-49: Validate ambient occlusion settings.Returning
1.0Fforces full brightness, which may be intentional for glass blocks in certain rendering contexts. Double-check that this behavior is desired if you want a more translucent or softer lighting effect.
57-57: Color retrieval logic is correct.Returning the stored
coloraligns with theStainableinterface requirements. No issues found here.
33-46: Consider consolidating similar block checks.When verifying if sides are invisible, there are parallel checks for
StainedGlassBlock,this, andStainedGlassStairsBlock. You could streamline the approach by grouping color checks and instance checks into a single condition to make the logic more concise. Also, confirm that the parent’sisInvisiblemethod is thoroughly tested for edge cases (e.g., mixing tinted and non-tinted glass).src/main/java/net/wurstclient/glass/mixin/TintedGlassBlockMixin.java (4)
11-11: LGTM! Imports are properly aligned with the new implementation.The new imports support the enhanced visibility checking approach using
VoxelShapes.Also applies to: 16-21
27-30: LGTM! Constructor signature simplified.Good cleanup by removing the unused
MoGlassparameter from the constructor.
Line range hint
33-45: LGTM! Improved visibility check logic.The refactored implementation provides:
- Consistent visibility checking for both slab and stairs
- Clean separation of concerns with the extracted
isInvisiblemethod- Proper fallback to parent implementation
51-53: Verify the VoxelShape-based visibility check across different scenarios.The new geometric approach using
VoxelShapes.isSideCoveredshould be tested with various block configurations to ensure it works correctly in all cases.Let's verify the implementation with the following script:
✅ Verification successful
✓ VoxelShape-based visibility check is consistently implemented
The implementation is correctly and uniformly applied across all glass block variants (regular, stained, tinted) and shapes (full blocks, stairs, slabs), ensuring consistent behavior throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering the new visibility check implementation rg -l "test.*Invisible" --type java # Look for any existing VoxelShapes.isSideCovered usage in tests rg "VoxelShapes\.isSideCovered.*test" --type java # Find all block classes that might need similar visibility checks ast-grep --pattern 'class $_ extends TransparentBlock'Length of output: 146
Script:
#!/bin/bash # Search for glass-related block classes rg -l "Glass.*Block" --type java # Look for any VoxelShape usage in block classes rg "VoxelShape" -A 3 --type java # Search for similar visibility check implementations rg "shouldDrawSide|getVisibility" --type javaLength of output: 7930
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/net/wurstclient/glass/mixin/TintedGlassBlockMixin.java (1)
47-57: Add documentation for the geometric visibility calculation.While the implementation using
VoxelShapes.isSideCoveredis robust, consider adding a brief comment explaining how the geometric calculation determines side invisibility. This would help future maintainers understand the logic more quickly.@Unique +// Determines if the side should be invisible by checking if the neighboring block's +// outline shape fully covers the current block's face in the given direction private boolean isInvisible(BlockState state, BlockState stateFrom, Direction direction)src/main/java/net/wurstclient/glass/TintedGlassSlabBlock.java (1)
Line range hint
28-42: Clean implementation with good separation of concerns.The refactored implementation is more maintainable and follows DRY principles by leveraging the parent class's
isInvisiblemethod.Consider improving readability by extracting the block type checks into descriptive boolean variables:
@Override @Environment(EnvType.CLIENT) public boolean isSideInvisible(BlockState state, BlockState stateFrom, Direction direction) { - if(stateFrom.getBlock() == Blocks.TINTED_GLASS) + boolean isTintedGlass = stateFrom.getBlock() == Blocks.TINTED_GLASS; + boolean isTintedGlassSlab = stateFrom.getBlock() == this; + boolean isTintedGlassStairs = stateFrom.getBlock() == MoGlassBlocks.TINTED_GLASS_STAIRS; + + if(isTintedGlass) return true; - if(stateFrom.getBlock() == this + if(isTintedGlassSlab && isInvisible(state, stateFrom, direction)) return true; - if(stateFrom.getBlock() == MoGlassBlocks.TINTED_GLASS_STAIRS + if(isTintedGlassStairs && isInvisible(state, stateFrom, direction)) return true; return super.isSideInvisible(state, stateFrom, direction); }src/main/java/net/wurstclient/glass/TintedGlassStairsBlock.java (1)
33-41: Consider extracting block type checks for better readability.The implementation effectively leverages the parent class's
isInvisiblemethod. However, the block type checks could be made more readable.Consider this minor refactoring:
- if(stateFrom.getBlock() == MoGlassBlocks.TINTED_GLASS_SLAB - && isInvisible(state, stateFrom, direction)) + boolean isTintedGlassSlab = stateFrom.getBlock() == MoGlassBlocks.TINTED_GLASS_SLAB; + boolean isTintedGlassStairs = stateFrom.getBlock() == this; + + if(isTintedGlassSlab && isInvisible(state, stateFrom, direction)) return true; - if(stateFrom.getBlock() == this - && isInvisible(state, stateFrom, direction)) + if(isTintedGlassStairs && isInvisible(state, stateFrom, direction)) return true;src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java (1)
60-70: Add documentation for the geometric visibility check approach.The implementation using
VoxelShapes.isSideCoveredis robust and efficient. Consider adding a brief comment explaining the geometric approach to help future maintainers understand the rationale.@Unique +// Determines if a side should be invisible by checking if the adjacent block's shape +// fully covers the current block's face in the given direction private boolean isInvisible(BlockState state, BlockState stateFrom, Direction direction)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/net/wurstclient/glass/GlassSlabBlock.java(2 hunks)src/main/java/net/wurstclient/glass/GlassStairsBlock.java(2 hunks)src/main/java/net/wurstclient/glass/StainedGlassSlabBlock.java(2 hunks)src/main/java/net/wurstclient/glass/StainedGlassStairsBlock.java(2 hunks)src/main/java/net/wurstclient/glass/TintedGlassSlabBlock.java(2 hunks)src/main/java/net/wurstclient/glass/TintedGlassStairsBlock.java(2 hunks)src/main/java/net/wurstclient/glass/mixin/GlassBlockMixin.java(2 hunks)src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java(3 hunks)src/main/java/net/wurstclient/glass/mixin/TintedGlassBlockMixin.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (25)
src/main/java/net/wurstclient/glass/mixin/TintedGlassBlockMixin.java (3)
11-21: LGTM! Clean up of imports and constructor.The removal of the unused
MoGlassparameter and the addition of necessary VoxelShape-related imports improve code clarity.Also applies to: 27-30
Line range hint
33-45: LGTM! Improved consistency in visibility checks.The refactored method provides a unified approach to checking visibility for both slab and stairs blocks, making the code more maintainable.
Line range hint
33-57: Verify the visibility behavior with various block configurations.The new implementation looks solid, but please ensure it's tested with various combinations of:
- Different block orientations
- Mixed glass types (tinted, stained, regular)
- Complex stair configurations
- Corner cases where multiple glass blocks meet
Would you like me to help create a comprehensive test suite for these scenarios?
src/main/java/net/wurstclient/glass/TintedGlassSlabBlock.java (1)
Line range hint
19-24: LGTM! Good inheritance restructuring.The change to extend
GlassSlabBlockis a solid improvement that promotes code reuse and better represents the relationship between glass types.src/main/java/net/wurstclient/glass/TintedGlassStairsBlock.java (2)
18-23: LGTM! Clean inheritance restructuring.The changes to extend
GlassStairsBlockand simplify the constructor align well with the goal of revamping side invisibility checks. The package-private visibility is appropriate for this implementation.
Line range hint
44-56: LGTM! Correct ambient occlusion and transparency handling.The implementations correctly maintain the expected behavior for tinted glass blocks.
src/main/java/net/wurstclient/glass/mixin/GlassBlockMixin.java (4)
11-22: LGTM! All new imports are necessary for the refactored visibility logic.
28-31: LGTM! Good simplification of the constructor.The removal of the
MoGlassparameter reduces coupling, making the class more maintainable.
39-48: LGTM! Clean and well-structured visibility checks.The refactored logic handles both glass slabs and stairs consistently using the new
isInvisiblemethod.
51-61: LGTM! Excellent use of vanilla Minecraft's shape-based visibility system.The new
isInvisiblemethod is a significant improvement:
- Uses
VoxelShapes.isSideCoveredfor robust shape-based visibility checks- Works correctly with all block shapes
- Follows vanilla Minecraft's approach to side visibility
The
@Uniqueannotation correctly prevents method conflicts in the mixin.src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java (3)
13-13: LGTM! New imports support the VoxelShape-based visibility checking.The added imports are necessary and properly utilized for the new geometric-based approach to visibility checking.
Also applies to: 20-25
36-39: LGTM! Constructor simplified by removing unnecessary dependency.The constructor has been appropriately simplified to only accept the essential
Settingsparameter, improving the class's independence.
Line range hint
42-58: LGTM! Clean refactoring of visibility checks.The method maintains proper color matching while delegating the geometric visibility check to the new
isInvisiblemethod. The logic is clear and the fallback to super is correctly preserved.src/main/java/net/wurstclient/glass/GlassStairsBlock.java (3)
23-25: LGTM! Well-structured inheritance changes.The removal of
finaland constructor access changes support better encapsulation while enabling inheritance for specialized glass stair blocks within the package.
39-43: LGTM! Clean refactoring of visibility checks.The visibility logic is now more maintainable by delegating to the new
isInvisiblemethod while preserving backward compatibility.
49-57: Verify edge cases in the new visibility implementation.While the new implementation using
VoxelShapes.isSideCoveredis cleaner, please verify:
- That
EmptyBlockView.INSTANCEprovides accurate shapes for all possible block states- Edge cases where block shapes might not align perfectly
- That null states/directions are handled appropriately
Consider adding null checks for the input parameters.
✅ Verification successful
Implementation verified as safe and consistent
The visibility check implementation follows the standard pattern used consistently across all glass-related blocks in the mod and matches vanilla Minecraft's approach. The use of
EmptyBlockView.INSTANCEis appropriate for outline shape calculations in this context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of EmptyBlockView to understand common patterns rg -A 2 "EmptyBlockView" --type java # Search for test cases involving VoxelShapes.isSideCovered rg -A 5 "isSideCovered.*direction" --type javaLength of output: 4481
src/main/java/net/wurstclient/glass/StainedGlassStairsBlock.java (4)
18-18: LGTM! Good inheritance restructuring.The change to extend
GlassStairsBlockbetter represents the class hierarchy and promotes code reuse for glass-specific functionality while maintaining the stainable behavior.
25-27: LGTM! Constructor simplified appropriately.The constructor has been streamlined while maintaining proper initialization of the color field.
Line range hint
52-63: LGTM! Ambient occlusion and color handling preserved.The ambient occlusion and color functionality remain correctly implemented, ensuring proper rendering and staining behavior.
42-48: Verify behavior with different stair orientations.The unified
isInvisiblecheck looks good, but please ensure it handles all stair orientation combinations correctly:
- Stairs facing each other
- Stairs at right angles
- Inverted stairs
- Corner stairs
✅ Verification successful
Glass visibility changes don't affect stair orientation behavior
The
isInvisiblecheck only handles glass-specific properties while all stair orientation logic is managed by the unchanged parentStairsBlockclass. The changes are safe and won't affect how different stair orientations connect.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering stair orientations rg -l "StainedGlassStairsBlock.*test" src/Length of output: 44
Script:
#!/bin/bash # Find parent class and related implementations echo "=== Parent class implementation ===" ast-grep --pattern 'class $_ extends $_ { $$$ isInvisible($$$) { $$$ } $$$ }' echo "=== Related stair implementations ===" rg -A 5 "extends.*Stairs.*Block" src/ echo "=== Block state handling ===" rg -A 10 "getStateForPlacement|createBlockState" src/main/java/net/wurstclient/glass/StainedGlassStairsBlock.javaLength of output: 2101
src/main/java/net/wurstclient/glass/GlassSlabBlock.java (3)
23-23: LGTM! Good architectural change.Removing the
finalmodifier enables proper inheritance hierarchy for specialized glass slab blocks.
38-46: LGTM! Clean implementation of side invisibility checks.The logic is now more maintainable with clear separation of concerns:
- Glass block check
- Same block type check
- Glass stairs check
49-57: Verify the behavior of EmptyBlockView usage.The implementation uses
EmptyBlockView.INSTANCEandBlockPos.ORIGINfor shape calculations. While this approach is more efficient than creating temporary objects, we should verify it works correctly in all scenarios.Consider adding a comment explaining why
EmptyBlockViewis sufficient here, as it might not be immediately obvious to other developers that block state's outline shape doesn't need world context in this case.src/main/java/net/wurstclient/glass/StainedGlassSlabBlock.java (2)
21-22: LGTM! Good use of inheritance.Extending
GlassSlabBlockwhile implementingStainableprovides a clean separation between glass behavior and staining capability.
43-45: Verify color-specific invisibility behavior.The implementation correctly handles invisibility checks for:
- Regular stained glass blocks
- Same colored stained glass slabs
- Same colored stained glass stairs
However, we should verify that the invisibility behavior works correctly when different colored glass blocks are adjacent.
Also applies to: 48-48
✅ Verification successful
Different colored glass blocks are correctly non-invisible to each other
The implementation in
StainedGlassSlabBlockexplicitly requires matching colors for invisibility behavior, which is the correct and expected behavior matching vanilla Minecraft's stained glass mechanics. The code properly checks color equality usinggetColor() == colorwhen comparing with adjacent stained glass blocks, slabs, and stairs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests covering different colored glass block interactions rg -l "StainedGlassSlabBlock.*getColor|isSideInvisible.*StainedGlass" src/test/Length of output: 131
Script:
#!/bin/bash # Find the implementation of isInvisible and color-related methods ast-grep --pattern 'class StainedGlassSlabBlock { $$$ isInvisible($$$) { $$$ } $$$ }' # Search for color-related methods and comparisons rg "getColor|DyeColor" --type java # Find test files in the repository fd -e java -e kt testLength of output: 3151
Script:
#!/bin/bash # Check the test implementation cat src/main/java/net/wurstclient/glass/test/MoGlassTestClient.java # Also check the actual invisibility implementation in StainedGlassSlabBlock cat src/main/java/net/wurstclient/glass/StainedGlassSlabBlock.javaLength of output: 12471
There was a problem hiding this comment.
Hi @Fellteros, thanks for the PR!
Looking through the changed files, there seem to be quite a few changes here that are not actually related to the goal of using the VoxelShape system for visibility checks.
For example, removing final modifiers, changing constructor visibility and parameters, and even changing the tinted glass pieces to inherit directly from the transparent ones.
Could you please revert all those unrelated changes, so that the PR only migrates to the VoxelShape system without changing anything else?
|
ofc |
|
Though I don't quite understand why. Why don't you want to inherit from those blocks? Also having protected constructor in a final class is redundant. Why not just make it public? just curious |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/main/java/net/wurstclient/glass/TintedGlassStairsBlock.java (1)
50-58: Consider caching shape calculations for better performance.The current implementation calculates outline shapes for every visibility check. While correct, this could be optimized by caching the shapes, especially since they're calculated using
EmptyBlockViewand don't depend on world state.Example optimization:
+ private static final Map<BlockState, VoxelShape> SHAPE_CACHE = new ConcurrentHashMap<>(); + + private VoxelShape getCachedShape(BlockState state) { + return SHAPE_CACHE.computeIfAbsent(state, + s -> s.getOutlineShape(EmptyBlockView.INSTANCE, BlockPos.ORIGIN)); + } + protected boolean isInvisible(BlockState state, BlockState stateFrom, Direction direction) { - VoxelShape stateCullingShape = - state.getOutlineShape(EmptyBlockView.INSTANCE, BlockPos.ORIGIN); - VoxelShape stateFromCullingShape = - stateFrom.getOutlineShape(EmptyBlockView.INSTANCE, BlockPos.ORIGIN); + VoxelShape stateCullingShape = getCachedShape(state); + VoxelShape stateFromCullingShape = getCachedShape(stateFrom); return VoxelShapes.isSideCovered(stateCullingShape, stateFromCullingShape, direction); }src/main/java/net/wurstclient/glass/mixin/TintedGlassBlockMixin.java (1)
10-10: Remove unused import.The
MoGlassimport is no longer used after the constructor parameter was removed.-import net.wurstclient.glass.MoGlass;src/main/java/net/wurstclient/glass/mixin/GlassBlockMixin.java (1)
10-10: Remove unused import.The
MoGlassimport is no longer used after the constructor parameter was removed.-import net.wurstclient.glass.MoGlass;src/main/java/net/wurstclient/glass/GlassStairsBlock.java (1)
Line range hint
27-29: Consider making the constructor public.Since the class is no longer final and might be extended, consider making the constructor public to allow instantiation from other packages.
-protected GlassStairsBlock(Settings settings) +public GlassStairsBlock(Settings settings)src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java (1)
10-10: Remove unused import.The
MoGlassimport is no longer used after the constructor parameter was removed.-import net.wurstclient.glass.MoGlass;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/net/wurstclient/glass/GlassSlabBlock.java(2 hunks)src/main/java/net/wurstclient/glass/GlassStairsBlock.java(2 hunks)src/main/java/net/wurstclient/glass/StainedGlassSlabBlock.java(2 hunks)src/main/java/net/wurstclient/glass/StainedGlassStairsBlock.java(2 hunks)src/main/java/net/wurstclient/glass/TintedGlassSlabBlock.java(2 hunks)src/main/java/net/wurstclient/glass/TintedGlassStairsBlock.java(2 hunks)src/main/java/net/wurstclient/glass/mixin/GlassBlockMixin.java(2 hunks)src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java(2 hunks)src/main/java/net/wurstclient/glass/mixin/TintedGlassBlockMixin.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/net/wurstclient/glass/StainedGlassStairsBlock.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
src/main/java/net/wurstclient/glass/TintedGlassStairsBlock.java (2)
19-22: LGTM! Necessary imports added for the new implementation.The new imports support the refactored visibility logic and client-side annotations.
40-44: LGTM! Simplified visibility logic.The refactored code improves maintainability by:
- Consolidating visibility checks into a single
isInvisiblemethod- Maintaining compatibility with the parent class
src/main/java/net/wurstclient/glass/mixin/TintedGlassBlockMixin.java (1)
48-57: LGTM! Improved visibility check using geometric shapes.The new
isInvisiblemethod usingVoxelShapes.isSideCoveredis a more robust approach as it checks the actual block shapes rather than relying on block-specific logic. This change:
- Improves maintainability by centralizing the visibility logic
- Makes the check more accurate by using geometric shapes
- Allows for better extensibility through the protected visibility
src/main/java/net/wurstclient/glass/mixin/GlassBlockMixin.java (1)
52-61: LGTM! Consistent implementation with TintedGlassBlockMixin.The implementation follows the same pattern as TintedGlassBlockMixin, which is good for maintainability and consistency across the codebase.
src/main/java/net/wurstclient/glass/GlassSlabBlock.java (1)
Line range hint
24-58: LGTM! Good refactoring for extensibility.The changes improve the code in several ways:
- Removing
finalallows for inheritance, which is useful for specialized glass types- The new
isInvisiblemethod follows the same pattern as the mixin classes- Special case for
Blocks.GLASSis handled correctlysrc/main/java/net/wurstclient/glass/GlassStairsBlock.java (1)
50-58: LGTM! Consistent implementation with other glass blocks.The implementation follows the same pattern as other glass blocks, which is good for maintainability.
src/main/java/net/wurstclient/glass/mixin/StainedGlassBlockMixin.java (1)
61-70: LGTM! Consistent implementation with proper color handling.The implementation:
- Follows the same pattern as other glass blocks
- Correctly handles color matching for stained glass variants
- Uses the same robust geometric approach for visibility checks
src/main/java/net/wurstclient/glass/StainedGlassSlabBlock.java (3)
17-17: LGTM! Import addition is appropriate.The addition of
EmptyBlockViewimport is necessary for the new shape-based visibility calculations.
44-51: LGTM! Improved visibility check implementation.The refactoring simplifies the logic by using a unified
isInvisiblemethod while maintaining proper type checking. This makes the code more maintainable and consistent.
55-64: Consider caching BlockPos.ORIGIN and verify shape-based visibility.The implementation looks good, using VoxelShapes for accurate visibility checks. However:
- Consider caching
BlockPos.ORIGINas a static final field to avoid repeated references.- Let's verify that the shape-based visibility checks work correctly for all block states.
✅ Verification successful
Shape-based visibility implementation verified and consistent
The implementation is correctly aligned with all other glass block variants in the codebase, including full blocks, slabs, and stairs, ensuring consistent behavior across different glass types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of shape-based visibility checks to ensure consistency rg -l "VoxelShapes.isSideCovered" . # Look for potential test cases or related block implementations ast-grep --pattern 'class $_ implements Stainable { $$$ isSideInvisible($$$) { $$$ } }'Length of output: 723
src/main/java/net/wurstclient/glass/TintedGlassSlabBlock.java (3)
20-23: LGTM: Required imports added for shape calculations and client-side annotations.
41-47: LGTM: Simplified visibility checks using consolidated helper method.The refactoring improves maintainability by removing duplicate logic and consolidating visibility checks into a single
isInvisiblemethod.
51-59: Verify performance and thread safety of shape calculations.The new implementation uses
VoxelShapes.isSideCoveredfor visibility checks, which might have performance implications. Additionally, usingEmptyBlockView.INSTANCEsingleton warrants thread safety consideration.Let's verify the usage of similar patterns in other Minecraft mods:
Also, please confirm:
- Are shape calculations cached to avoid performance overhead?
- Is
EmptyBlockView.INSTANCEthread-safe in the context of client-side rendering?✅ Verification successful
Implementation follows Minecraft's standard block culling pattern
The current implementation is both performant and thread-safe:
- Uses Minecraft's official
VoxelShapes.isSideCoveredAPI for cullingEmptyBlockView.INSTANCEis a stateless singleton designed for this exact use case- Consistent with implementation across all glass block types in the mod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar VoxelShape visibility check patterns in other mods rg -U "VoxelShapes\.isSideCovered.*EmptyBlockView\.INSTANCE" \ --type java \ -A 2 -B 2Length of output: 84
Script:
#!/bin/bash # Search for VoxelShapes.isSideCovered usage echo "=== VoxelShapes.isSideCovered usage ===" rg "VoxelShapes\.isSideCovered" --type java -B 2 -A 2 echo -e "\n=== EmptyBlockView usage ===" rg "EmptyBlockView" --type java -B 2 -A 2 echo -e "\n=== Block culling/visibility patterns ===" rg "getOutlineShape.*BlockPos\.ORIGIN" --type java -B 2 -A 2Length of output: 23226
There was a problem hiding this comment.
Hey @Fellteros, thanks for reverting those other changes!
I'm currently working on automating my testing process for the visibility/transparency stuff. Once that's ready, I'll use it to check if the VoxelShape method does indeed work in all cases. Until then, please be patient as this PR will likely just sit here for a while.
As for why I asked you to revert the other changes:
First, I want to point out that PRs in general should be small and focused. They should change only what is necessary to get the desired result and otherwise follow the project's existing conventions rather than challenging them. Unrelated changes make the diff harder to read, which can obscure bugs, but more importantly they create pointless arguments about whether coding pattern X is better than Y, which can get your PR stalled for reasons that have nothing to do with the original PR objective.
Second, the GlassSlabBlock and GlassStairsBlock are intended to be only the clear glass variants of these blocks. They are not intended as an abstract parent class of all three variants (or as a non-abstract parent of some variants while also being the clear variant at the same time). This change would have changed the meaning of these classes in a way that could have all kinds of unintended downstream effects, for example, silently breaking other mods that use instanceof GlassStairsBlock to detect only clear glass stairs. (Notice how this has nothing to do with the VoxelShape system - better to cut this out than to make the PR depend on it.)
Hope that makes sense!
|
ok, thanks for explanation |
Alexander01998
added
status:never-stale
Hi there,
recently I have been trying to find an easy way to make the sides of glass blocks invisible. Since I've taken some inspiration from your mod before, I thought I'd return the favor and provide you with my solution (see the changed files).
Here's how it works: We get two voxel shapes, from both blocks we are checking. Since there are no
BlockView&BlockPosparameters, we use an instance ofEmptyBlockViewandBlockPos.ORIGIN. Then we use a method from theVoxelShapesclass, that checks if two adjacent faces are covered. It's as simple as that.It should work in all cases, if not, please notify me.
There's no need to credit me, though I would be grateful if you do.