Repository navigation
Rework BlockEntityRenderFabricExtension to support alternative implementations - #342
Conversation
WalkthroughBlock entity render bounds now use a helper that supports extension implementations and compatible reflected methods. The caller uses this helper, and the ChangesBlock entity render bounds
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant EntityCullingMod
participant BlockEntityRenderFabricExtension
participant RENDER_BOUNDING_BOX_GETTER
participant BlockEntityRenderer
EntityCullingMod->>BlockEntityRenderFabricExtension: tryGetRenderBoundingBox(renderer, blockEntity)
alt renderer implements the extension
BlockEntityRenderFabricExtension->>BlockEntityRenderer: getRenderBoundingBox(blockEntity)
else renderer requires reflection
BlockEntityRenderFabricExtension->>RENDER_BOUNDING_BOX_GETTER: look up method for renderer class
RENDER_BOUNDING_BOX_GETTER-->>BlockEntityRenderFabricExtension: compatible method or no method
alt compatible method found
BlockEntityRenderFabricExtension->>BlockEntityRenderer: invoke getRenderBoundingBox(blockEntity)
else no compatible method
BlockEntityRenderFabricExtension-->>EntityCullingMod: return block-position box
end
end
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The reviewed change preserves the existing bounds fallback, and no demonstrated regression currently blocks merging. Compatibility with Kilt remains unverified at runtime. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Issue #1020requires EntityCulling to avoid the Kilt default-method conflict and to load into a world with block-entity culling active. The reviewed code removes the injected `BlockEntityRendererMixin…Out of Scope Changes check The reviewed changes are limited to block-entity render-bounding-box dispatch. The reflection cache, fallback behavior, caller update, and removal of the interface-injection mixin directly support iss… Title check The title clearly summarizes the main change: updating BlockEntityRenderFabricExtension to support alternative implementations. Description check The description directly explains support for Kilt interface injection, alternative implementations, compatibility, and ClassValue-based method caching.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
-
-
🧪 Generate unit tests (beta)
-
-
-
- Create a new PR
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (4)
src/main/java/dev/tr7zw/entityculling/EntityCullingMod.java (1)
58-59: LGTM!src/main/resources/entityculling.mixins.json (1)
17-17: LGTM!src/main/java/dev/tr7zw/entityculling/BlockEntityRenderFabricExtension.java-51-54 (1)
51-54: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Do not wrap reflection failures in an uncaught
RuntimeException.
setupAABBcalls this helper on the render and culling path. If the reflectedgetRenderBoundingBoxthrows,InvocationTargetExceptionbecomes aRuntimeException. The exception propagates to the caller and can crash the client. A block entity renderer that fails ingetRenderBoundingBoxthen causes a crash instead of a fallback.Fall back to the block-position box on failure. Unwrap the cause only if you want to log it.
Proposed fix
try { return (AABB) method.orElseThrow().invoke(renderer, blockEntity); - } catch (IllegalAccessException | InvocationTargetException e) { - throw new RuntimeException(e); + } catch (IllegalAccessException | InvocationTargetException e) { + // Fall through to the default box. }src/main/java/dev/tr7zw/entityculling/BlockEntityRenderFabricExtension.java (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Add a comment inside the empty
catchblock.SonarCloud flags the empty block. State that a missing method is expected and falls back to the default box.
Proposed fix
- } catch (NoSuchMethodException ignored) { - } + } catch (NoSuchMethodException ignored) { + // Expected: the renderer has no such method. Use the default box. + }Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b2a813a8-e5a5-4d57-808d-6646e7f5ed32
📒 Files selected for processing (4)
src/main/java/dev/tr7zw/entityculling/BlockEntityRenderFabricExtension.javasrc/main/java/dev/tr7zw/entityculling/EntityCullingMod.javasrc/main/java/dev/tr7zw/entityculling/mixin/BlockEntityRendererMixin.javasrc/main/resources/entityculling.mixins.json
💤 Files with no reviewable changes (1)
- src/main/java/dev/tr7zw/entityculling/mixin/BlockEntityRendererMixin.java
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
|
Ok that's an interesting approach. Will check out out tomorrow and merge it if the implementation is sound. Using reflections usually is "not great ™️" but this might be a good usecase for such a flimsy method. |
2 participants
Supersedes #339, fixes KiltMC/Kilt#1020.
Also closes #335.
Allows properly supporting Kilt's interface injection of the same method, and also allows alternative implementations (e.g. Porting Lib) to be handled too. Still retains support for the interface, just in case there is a performance impact (edit: reading back, that sounds kinda stupid since it doesn't bypass on Vanilla BEs lmao), and also for the sake of mods that are already dependent on this interface.
Utilizes Java's
ClassValuefor caching the method reflections, Kilt uses a similar system and the performance impact seems to be pretty much next to none and thread-safe.