Merged
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @KB01111, your pull request is larger than the review limit of 150000 diff characters
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request downgrades the project to Java 17 and Minecraft 1.20.1 from Java 21 and 1.21.4 respectively, refactors vertex/buffer handling with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
codeant-ai
bot
added
the
size:XXL
74 new issues
|
Sequence DiagramThis PR adapts the world rendering pipeline to Minecraft 1.20.1 by rebuilding chunk and entity geometry using the new rendering APIs and forwarding that geometry into the Vulkan renderer instead of the legacy OpenGL path. sequenceDiagram
participant GameRenderer
participant WorldRenderer
participant ChunkProxy
participant EntityProxy
participant VulkanRenderer
GameRenderer->>WorldRenderer: Render world frame
WorldRenderer->>WorldRenderer: Setup terrain and visible chunks
WorldRenderer->>EntityProxy: Queue entity and block entity geometry
WorldRenderer->>ChunkProxy: Enqueue dirty chunks for rebuild
WorldRenderer->>ChunkProxy: Rebuild queued chunks based on camera
ChunkProxy->>ChunkProxy: Build chunk region and PBR vertex data
ChunkProxy->>VulkanRenderer: Submit rebuilt chunk geometry buffers
EntityProxy->>VulkanRenderer: Submit entity geometry buffers for this frame
VulkanRenderer-->>GameRenderer: Use submitted geometry for Vulkan world rendering
Generated by CodeAnt AI |
Nitpicks 🔍
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant effort to port the project to Minecraft 1.20.1 and ensure compatibility with JDK 17. The changes involve updating build configurations, adapting to new Minecraft rendering APIs, and temporarily simplifying or stubbing out complex rendering features to achieve a functional baseline for the new version. Further work will be needed to fully restore and optimize the temporarily disabled rendering functionalities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+265
to
+296
| if (buffers.isEmpty()) { | ||
| ChunkBuilder.ChunkData chunkData = new ChunkBuilder.ChunkData() { | ||
| @Override | ||
| public List<BlockEntity> getBlockEntities() { | ||
| return renderData.blockEntities; | ||
| return blockEntities; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isVisibleThrough(Direction from, Direction to) { | ||
| return renderData.chunkOcclusionData.isVisibleThrough(from, to); | ||
| return occlusionBuilder.build().isVisibleThrough(from, to); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isEmpty(RenderLayer layer) { | ||
| return false; | ||
| return true; | ||
| } | ||
| }; | ||
| builtChunk.data.set(chunkData); | ||
| builtChunkNum++; | ||
| invalidateSingle(builtChunk.index); | ||
| return; | ||
| } | ||
|
|
||
| ByteBuffer geometryTypeBB = null; | ||
| ByteBuffer geometryGroupNameBB = null; | ||
| ByteBuffer geometryTextureBB = null; | ||
| ByteBuffer vertexFormatBB = null; | ||
| ByteBuffer vertexCountBB = null; | ||
| ByteBuffer verticesBB = null; | ||
| List<ByteBuffer> geometryGroupNameBuffers = new ArrayList<>(buffers.size()); | ||
| ChunkBuilder.ChunkData chunkData = new ChunkBuilder.ChunkData() { | ||
| @Override | ||
| public List<BlockEntity> getBlockEntities() { | ||
| return blockEntities; | ||
| } | ||
|
|
||
| try { | ||
| int geometryTypeSize = buffers.size() * Integer.BYTES; | ||
| geometryTypeBB = MemoryUtil.memAlloc(geometryTypeSize); | ||
| long geometryTypeAddr = memAddress(geometryTypeBB); | ||
| int geometryTypeBaseAddr = 0; | ||
|
|
||
| int geometryGroupNameSize = buffers.size() * Long.BYTES; | ||
| geometryGroupNameBB = MemoryUtil.memAlloc(geometryGroupNameSize); | ||
| long geometryGroupNameAddr = memAddress(geometryGroupNameBB); | ||
| int geometryGroupNameBaseAddr = 0; | ||
|
|
||
| int geometryTextureSize = buffers.size() * Integer.BYTES; | ||
| geometryTextureBB = MemoryUtil.memAlloc(geometryTextureSize); | ||
| long geometryTextureAddr = memAddress(geometryTextureBB); | ||
| int geometryTextureBaseAddr = 0; | ||
|
|
||
| int vertexFormatSize = buffers.size() * Integer.BYTES; | ||
| vertexFormatBB = MemoryUtil.memAlloc(vertexFormatSize); | ||
| long vertexFormatAddr = memAddress(vertexFormatBB); | ||
| int vertexFormatBaseAddr = 0; | ||
|
|
||
| int vertexCountSize = buffers.size() * Integer.BYTES; | ||
| vertexCountBB = MemoryUtil.memAlloc(vertexCountSize); | ||
| long vertexCountAddr = memAddress(vertexCountBB); | ||
| int vertexCountBaseAddr = 0; | ||
|
|
||
| int verticesSize = buffers.size() * Long.BYTES; | ||
| verticesBB = MemoryUtil.memAlloc(verticesSize); | ||
| long verticesAddr = memAddress(verticesBB); | ||
| int verticesBaseAddr = 0; | ||
|
|
||
| for (Map.Entry<RenderLayer, BuiltBuffer> entry : buffers.entrySet()) { | ||
| RenderLayer renderLayer = entry.getKey(); | ||
| assert renderLayer.getDrawMode() == QUADS; | ||
|
|
||
| BuiltBuffer vertexBuffer = entry.getValue(); | ||
| BufferProxy.BufferInfo vertexBufferInfo = BufferProxy.getBufferInfo( | ||
| vertexBuffer.getBuffer()); | ||
| assert vertexBuffer.getDrawParameters() | ||
| .indexCount() == vertexBuffer.getDrawParameters() | ||
| .vertexCount() / 4 * 6; | ||
|
|
||
| TextureManager | ||
| textureManager = | ||
| MinecraftClient.getInstance() | ||
| .getTextureManager(); | ||
|
|
||
| int | ||
| geometryTypeID = | ||
| Constants.GeometryTypes.getGeometryType(renderLayer, true) | ||
| .getValue(); | ||
| int | ||
| geometryTextureID = | ||
| textureManager.getTexture( | ||
| ((RenderLayer.MultiPhase) renderLayer).phases.texture.getId() | ||
| .orElse(MissingSprite.getMissingSpriteId())) | ||
| .getGlId(); | ||
| int vertexFormatID = Constants.VertexFormats.getValue( | ||
| vertexBuffer.getDrawParameters() | ||
| .format()); | ||
|
|
||
| geometryTypeBB.putInt(geometryTypeBaseAddr, geometryTypeID); | ||
| geometryTypeBaseAddr += Integer.BYTES; | ||
|
|
||
| ByteBuffer geometryGroupNameBuffer = MemoryUtil.memUTF8(renderLayer.name, true); | ||
| geometryGroupNameBuffers.add(geometryGroupNameBuffer); | ||
| geometryGroupNameBB.putLong(geometryGroupNameBaseAddr, | ||
| memAddress(geometryGroupNameBuffer)); | ||
| geometryGroupNameBaseAddr += Long.BYTES; | ||
|
|
||
| geometryTextureBB.putInt(geometryTextureBaseAddr, geometryTextureID); | ||
| geometryTextureBaseAddr += Integer.BYTES; | ||
|
|
||
| vertexFormatBB.putInt(vertexFormatBaseAddr, vertexFormatID); | ||
| vertexFormatBaseAddr += Integer.BYTES; | ||
|
|
||
| vertexCountBB.putInt(vertexCountBaseAddr, | ||
| vertexBuffer.getDrawParameters() | ||
| .vertexCount()); | ||
| vertexCountBaseAddr += Integer.BYTES; | ||
|
|
||
| verticesBB.putLong(verticesBaseAddr, vertexBufferInfo.addr()); | ||
| verticesBaseAddr += Long.BYTES; | ||
| } | ||
| @Override | ||
| public boolean isVisibleThrough(Direction from, Direction to) { | ||
| return occlusionBuilder.build().isVisibleThrough(from, to); |
There was a problem hiding this comment.
Suggestion: The occlusion data is rebuilt on every visibility check by calling occlusionBuilder.build() inside isVisibleThrough, which is both unnecessarily expensive and potentially incorrect if the builder is not intended to be reused; instead, occlusion should be built once per chunk and the resulting data reused for all visibility queries. [logic error]
Severity Level: Major ⚠️
- ⚠️ Extra occlusion recomputation on each visibility test per chunk.
- ⚠️ Chunk rendering path may suffer unnecessary CPU overhead.
- ⚠️ Potential frame drops when many chunks visible concurrently.
Suggested change
| if (buffers.isEmpty()) { | |
| ChunkBuilder.ChunkData chunkData = new ChunkBuilder.ChunkData() { | |
| @Override | |
| public List<BlockEntity> getBlockEntities() { | |
| return renderData.blockEntities; | |
| return blockEntities; | |
| } | |
| @Override | |
| public boolean isVisibleThrough(Direction from, Direction to) { | |
| return renderData.chunkOcclusionData.isVisibleThrough(from, to); | |
| return occlusionBuilder.build().isVisibleThrough(from, to); | |
| } | |
| @Override | |
| public boolean isEmpty(RenderLayer layer) { | |
| return false; | |
| return true; | |
| } | |
| }; | |
| builtChunk.data.set(chunkData); | |
| builtChunkNum++; | |
| invalidateSingle(builtChunk.index); | |
| return; | |
| } | |
| ByteBuffer geometryTypeBB = null; | |
| ByteBuffer geometryGroupNameBB = null; | |
| ByteBuffer geometryTextureBB = null; | |
| ByteBuffer vertexFormatBB = null; | |
| ByteBuffer vertexCountBB = null; | |
| ByteBuffer verticesBB = null; | |
| List<ByteBuffer> geometryGroupNameBuffers = new ArrayList<>(buffers.size()); | |
| ChunkBuilder.ChunkData chunkData = new ChunkBuilder.ChunkData() { | |
| @Override | |
| public List<BlockEntity> getBlockEntities() { | |
| return blockEntities; | |
| } | |
| try { | |
| int geometryTypeSize = buffers.size() * Integer.BYTES; | |
| geometryTypeBB = MemoryUtil.memAlloc(geometryTypeSize); | |
| long geometryTypeAddr = memAddress(geometryTypeBB); | |
| int geometryTypeBaseAddr = 0; | |
| int geometryGroupNameSize = buffers.size() * Long.BYTES; | |
| geometryGroupNameBB = MemoryUtil.memAlloc(geometryGroupNameSize); | |
| long geometryGroupNameAddr = memAddress(geometryGroupNameBB); | |
| int geometryGroupNameBaseAddr = 0; | |
| int geometryTextureSize = buffers.size() * Integer.BYTES; | |
| geometryTextureBB = MemoryUtil.memAlloc(geometryTextureSize); | |
| long geometryTextureAddr = memAddress(geometryTextureBB); | |
| int geometryTextureBaseAddr = 0; | |
| int vertexFormatSize = buffers.size() * Integer.BYTES; | |
| vertexFormatBB = MemoryUtil.memAlloc(vertexFormatSize); | |
| long vertexFormatAddr = memAddress(vertexFormatBB); | |
| int vertexFormatBaseAddr = 0; | |
| int vertexCountSize = buffers.size() * Integer.BYTES; | |
| vertexCountBB = MemoryUtil.memAlloc(vertexCountSize); | |
| long vertexCountAddr = memAddress(vertexCountBB); | |
| int vertexCountBaseAddr = 0; | |
| int verticesSize = buffers.size() * Long.BYTES; | |
| verticesBB = MemoryUtil.memAlloc(verticesSize); | |
| long verticesAddr = memAddress(verticesBB); | |
| int verticesBaseAddr = 0; | |
| for (Map.Entry<RenderLayer, BuiltBuffer> entry : buffers.entrySet()) { | |
| RenderLayer renderLayer = entry.getKey(); | |
| assert renderLayer.getDrawMode() == QUADS; | |
| BuiltBuffer vertexBuffer = entry.getValue(); | |
| BufferProxy.BufferInfo vertexBufferInfo = BufferProxy.getBufferInfo( | |
| vertexBuffer.getBuffer()); | |
| assert vertexBuffer.getDrawParameters() | |
| .indexCount() == vertexBuffer.getDrawParameters() | |
| .vertexCount() / 4 * 6; | |
| TextureManager | |
| textureManager = | |
| MinecraftClient.getInstance() | |
| .getTextureManager(); | |
| int | |
| geometryTypeID = | |
| Constants.GeometryTypes.getGeometryType(renderLayer, true) | |
| .getValue(); | |
| int | |
| geometryTextureID = | |
| textureManager.getTexture( | |
| ((RenderLayer.MultiPhase) renderLayer).phases.texture.getId() | |
| .orElse(MissingSprite.getMissingSpriteId())) | |
| .getGlId(); | |
| int vertexFormatID = Constants.VertexFormats.getValue( | |
| vertexBuffer.getDrawParameters() | |
| .format()); | |
| geometryTypeBB.putInt(geometryTypeBaseAddr, geometryTypeID); | |
| geometryTypeBaseAddr += Integer.BYTES; | |
| ByteBuffer geometryGroupNameBuffer = MemoryUtil.memUTF8(renderLayer.name, true); | |
| geometryGroupNameBuffers.add(geometryGroupNameBuffer); | |
| geometryGroupNameBB.putLong(geometryGroupNameBaseAddr, | |
| memAddress(geometryGroupNameBuffer)); | |
| geometryGroupNameBaseAddr += Long.BYTES; | |
| geometryTextureBB.putInt(geometryTextureBaseAddr, geometryTextureID); | |
| geometryTextureBaseAddr += Integer.BYTES; | |
| vertexFormatBB.putInt(vertexFormatBaseAddr, vertexFormatID); | |
| vertexFormatBaseAddr += Integer.BYTES; | |
| vertexCountBB.putInt(vertexCountBaseAddr, | |
| vertexBuffer.getDrawParameters() | |
| .vertexCount()); | |
| vertexCountBaseAddr += Integer.BYTES; | |
| verticesBB.putLong(verticesBaseAddr, vertexBufferInfo.addr()); | |
| verticesBaseAddr += Long.BYTES; | |
| } | |
| @Override | |
| public boolean isVisibleThrough(Direction from, Direction to) { | |
| return occlusionBuilder.build().isVisibleThrough(from, to); | |
| net.minecraft.client.render.chunk.ChunkOcclusionData occlusionData = occlusionBuilder.build(); | |
| if (buffers.isEmpty()) { | |
| ChunkBuilder.ChunkData chunkData = new ChunkBuilder.ChunkData() { | |
| @Override | |
| public List<BlockEntity> getBlockEntities() { | |
| return blockEntities; | |
| } | |
| @Override | |
| public boolean isVisibleThrough(Direction from, Direction to) { | |
| return occlusionData.isVisibleThrough(from, to); | |
| } | |
| @Override | |
| public boolean isEmpty(RenderLayer layer) { | |
| return true; | |
| } | |
| }; | |
| builtChunk.data.set(chunkData); | |
| builtChunkNum++; | |
| invalidateSingle(builtChunk.index); | |
| return; | |
| } | |
| ChunkBuilder.ChunkData chunkData = new ChunkBuilder.ChunkData() { | |
| @Override | |
| public List<BlockEntity> getBlockEntities() { | |
| return blockEntities; | |
| } | |
| @Override | |
| public boolean isVisibleThrough(Direction from, Direction to) { | |
| return occlusionData.isVisibleThrough(from, to); |
Steps of Reproduction ✅
1. Trigger a chunk rebuild in-game so that `ChunkBuilder.BuiltChunk.clear()` or
`scheduleRebuild(boolean)` is called; the mixin `ChunkBuilderBuiltChunkMixins` at
`src/main/java/com/radiance/mixins/vulkan_render_integration/ChunkBuilderBuiltChunkMixins.java:4–13`
injects into these methods and calls `ChunkProxy.enqueueRebuild(self)`, adding the chunk
to `ChunkProxy`'s `rebuildQueue`.
2. During rendering, `WorldRendererMixins.redirectRender(...)` in
`src/main/java/com/radiance/mixins/vulkan_render_integration/WorldRendererMixins.java:147–285`
replaces `WorldRenderer.render` and at line 282 calls `ChunkProxy.rebuild(camera)`, which
iterates `rebuildQueue` and for each `BuiltChunk` eventually calls
`rebuildSingle(ChunkRendererRegion, BuiltChunk, boolean)` in `ChunkProxy.java:192–305`.
3. Inside `ChunkProxy.rebuildSingle(ChunkRendererRegion, BuiltChunk, boolean)` at
`ChunkProxy.java:195–247`, a `ChunkOcclusionDataBuilder occlusionBuilder` is created (line
197) and populated in the block loop where opaque full cubes cause
`occlusionBuilder.markClosed(blockPos)` (lines 210–214), computing all occluder
information for that chunk rebuild.
4. After geometry buffers are assembled, anonymous `ChunkBuilder.ChunkData`
implementations are created at `ChunkProxy.java:266–303`; in both the `buffers.isEmpty()`
branch (lines 266–281) and the normal branch (lines 288–303), `isVisibleThrough(Direction
from, Direction to)` is implemented as `return
occlusionBuilder.build().isVisibleThrough(from, to);`. Because `occlusionBuilder` is a
closed-over builder, every visibility query on that chunk's `ChunkData` causes
`occlusionBuilder.build()` to be invoked again, recomputing occlusion data on each call
instead of reusing a single `ChunkOcclusionData` instance built once per chunk rebuild.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/radiance/client/proxy/world/ChunkProxy.java
**Line:** 265:296
**Comment:**
*Logic Error: The occlusion data is rebuilt on every visibility check by calling `occlusionBuilder.build()` inside `isVisibleThrough`, which is both unnecessarily expensive and potentially incorrect if the builder is not intended to be reused; instead, occlusion should be built once per chunk and the resulting data reused for all visibility queries.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+256
to
+267
| StorageVertexConsumerProvider storageVertexConsumerProvider = | ||
| new StorageVertexConsumerProvider(8192); | ||
| storageVertexConsumerProviders.add(storageVertexConsumerProvider); | ||
|
|
||
| matrixStack.push(); | ||
|
|
||
| boolean bl = client.getCameraEntity() instanceof LivingEntity | ||
| && ((LivingEntity) client.getCameraEntity()).isSleeping(); | ||
| if (client.options.getPerspective() | ||
| .isFirstPerson() && !bl && !client.options.hudHidden && | ||
| client.interactionManager.getCurrentGameMode() != GameMode.SPECTATOR) { | ||
| boolean sleeping = client.getCameraEntity() instanceof LivingEntity livingEntity | ||
| && livingEntity.isSleeping(); | ||
| if (client.options.getPerspective().isFirstPerson() && !sleeping | ||
| && !client.options.hudHidden | ||
| && client.interactionManager != null | ||
| && client.interactionManager.getCurrentGameMode() != GameMode.SPECTATOR |
There was a problem hiding this comment.
Suggestion: In queueHandRebuild, a StorageVertexConsumerProvider is always allocated and added to storageVertexConsumerProviders even when the first-person hand rendering condition is false; in those cases queueBuild(...) is never called, so the provider is never closed, causing a per-frame native/resource leak whenever the hand is not rendered (e.g., third-person view, spectator, HUD hidden). [resource leak]
Severity Level: Critical 🚨
- ❌ Third-person or spectator rendering leaks native vertex buffers.
- ❌ Long sessions risk increased memory usage or instability.
- ⚠️ Vulkan raytracing hand path wastes per-frame GPU resources.
Suggested change
| StorageVertexConsumerProvider storageVertexConsumerProvider = | |
| new StorageVertexConsumerProvider(8192); | |
| storageVertexConsumerProviders.add(storageVertexConsumerProvider); | |
| matrixStack.push(); | |
| boolean bl = client.getCameraEntity() instanceof LivingEntity | |
| && ((LivingEntity) client.getCameraEntity()).isSleeping(); | |
| if (client.options.getPerspective() | |
| .isFirstPerson() && !bl && !client.options.hudHidden && | |
| client.interactionManager.getCurrentGameMode() != GameMode.SPECTATOR) { | |
| boolean sleeping = client.getCameraEntity() instanceof LivingEntity livingEntity | |
| && livingEntity.isSleeping(); | |
| if (client.options.getPerspective().isFirstPerson() && !sleeping | |
| && !client.options.hudHidden | |
| && client.interactionManager != null | |
| && client.interactionManager.getCurrentGameMode() != GameMode.SPECTATOR | |
| matrixStack.push(); | |
| boolean sleeping = client.getCameraEntity() instanceof LivingEntity livingEntity | |
| && livingEntity.isSleeping(); | |
| if (client.options.getPerspective().isFirstPerson() && !sleeping | |
| && !client.options.hudHidden | |
| && client.interactionManager != null | |
| && client.interactionManager.getCurrentGameMode() != GameMode.SPECTATOR | |
| && client.player != null) { | |
| StorageVertexConsumerProvider storageVertexConsumerProvider = | |
| new StorageVertexConsumerProvider(8192); | |
| storageVertexConsumerProviders.add(storageVertexConsumerProvider); | |
Steps of Reproduction ✅
1. Start the client with the Radiance mod and enter any world so that the standard
Minecraft rendering pipeline is active; the mixin `GameRendererMixins.redirectRenderHand`
at
`src/main/java/com/radiance/mixins/vulkan_render_integration/GameRendererMixins.java:12-18`
injects into `GameRenderer.renderHand(...)` and unconditionally calls
`EntityProxy.queueHandRebuild(this.buffers, tickDelta, this.firstPersonRenderer)`.
2. While in-game, switch the camera to a mode where the vanilla hand renderer would not
draw the hand (for example, a non-first-person perspective or spectator mode); in these
modes the injected `redirectRenderHand` is still called at the head of `renderHand`, so
`queueHandRebuild(...)` is invoked every frame from
`GameRendererMixins.redirectRenderHand(...)`.
3. In `EntityProxy.queueHandRebuild(...)` at
`src/main/java/com/radiance/client/proxy/world/EntityProxy.java:30-65` (see lines 37-39 in
the tool output), a `StorageVertexConsumerProvider` with size 8192 bytes is allocated and
added to `storageVertexConsumerProviders` before the condition starting at line 43
(`boolean sleeping = ...` and the subsequent `if
(client.options.getPerspective().isFirstPerson() && !sleeping && ... )`).
4. When the condition in step 3 evaluates to false (hand should not be rendered), the body
of the `if` is skipped: `radiance$renderItem(...)`, `processWorldEntityRenderData(...)`,
and crucially the call to `queueBuild(storageVertexConsumerProviders, renderDataList,
0.0f, Constants.Coordinates.CAMERA, false)` at lines 57-58 are never executed. Since
`queueBuild(...)` is the only place that closes each `StorageVertexConsumerProvider` it
receives (see the `queueBuild(List<StorageVertexConsumerProvider> ...)` implementation at
`EntityProxy.java:68-77` and its `finally` block at lines 139-176 where
`storageVertexConsumerProvider.close()` is called), the provider created before the `if`
is leaked for that frame. Repeating this each frame in such a camera mode accumulates
unclosed `StorageVertexConsumerProvider` instances, leaking native memory/buffers.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/radiance/client/proxy/world/EntityProxy.java
**Line:** 256:267
**Comment:**
*Resource Leak: In `queueHandRebuild`, a `StorageVertexConsumerProvider` is always allocated and added to `storageVertexConsumerProviders` even when the first-person hand rendering condition is false; in those cases `queueBuild(...)` is never called, so the provider is never closed, causing a per-frame native/resource leak whenever the hand is not rendered (e.g., third-person view, spectator, HUD hidden).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| public static float getColorFraction(int value) { | ||
| return COLOR_FRACTIONS[value & 0xFF]; | ||
| public static NativeImage getSpecificMipmapLevelImage(NativeImage source, int level) { | ||
| return source; |
There was a problem hiding this comment.
Suggestion: The helper method currently ignores the requested mipmap level and always returns the original image, so any caller expecting a specific mipmap level will instead receive the base level image, leading to incorrect texture sizes and rendering behavior; it should actually derive and return the correct mipmap level from the source image. [logic error]
Severity Level: Major ⚠️
- ⚠️ Currently unused helper; no impact until first caller.
- ⚠️ Future uses will get wrong mipmap image level.
- ⚠️ Potentially breaks texture scaling in Vulkan render integration.
Suggested change
| return source; | |
| NativeImage[] levels = net.minecraft.client.texture.MipmapHelper | |
| .getMipmapLevelsImages(new NativeImage[] { source }, level); | |
| return levels[level]; |
Steps of Reproduction ✅
1. Open `src/main/java/com/radiance/client/texture/MipmapUtil.java:5-12` and observe
`getSpecificMipmapLevelImage(NativeImage source, int level)` simply returns `source` and
does not use `level` at all.
2. Use Grep on the repository (`functions.Grep` over `/workspace/Radiance/**/*.java`) for
`getSpecificMipmapLevelImage`; only the definition in `MipmapUtil.java:10-12` is found and
no callers exist in the current codebase.
3. Once a caller is added that needs a non-base mipmap (e.g.,
`MipmapUtil.getSpecificMipmapLevelImage(sourceImage, 1)` to obtain level-1), the call will
execute this helper at `MipmapUtil.java:10-12`.
4. At runtime, regardless of the `level` argument, the helper returns the original
base-level `NativeImage`, so the caller will receive an image with the wrong mipmap level
and likely an unexpected size, leading to incorrect texture usage; this follows directly
from the implementation at `MipmapUtil.java:10-12` and the project's use of
`MipmapHelper.getMipmapLevelsImages` in
`src/main/java/com/radiance/mixins/vulkan_render_integration/MipmapHelperMixins.java:15-22`
which indicates mipmap levels are meaningful in this codebase.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/radiance/client/texture/MipmapUtil.java
**Line:** 11:11
**Comment:**
*Logic Error: The helper method currently ignores the requested mipmap level and always returns the original image, so any caller expecting a specific mipmap level will instead receive the base level image, leading to incorrect texture sizes and rendering behavior; it should actually derive and return the correct mipmap level from the source image.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| @Override | ||
| public void fixedColor(int red, int green, int blue, int alpha) { | ||
| this.color(red, green, blue, alpha); |
There was a problem hiding this comment.
Suggestion: The current implementation of the fixed-color API delegates directly to the per-vertex color method, which enforces that a vertex is already in progress; this breaks the VertexConsumer contract where calling fixedColor() before vertex() should be safe and effectively a no-op, and will cause an IllegalStateException when external code sets a fixed color before starting vertices. [logic error]
Severity Level: Major ⚠️
- ❌ Entity or block render may crash on fixedColor usage.
- ⚠️ Future renderers using fixedColor with PBR become unsafe.
Suggested change
| this.color(red, green, blue, alpha); | |
| // Only apply to the current vertex if one is in progress; otherwise behave as a safe no-op. | |
| if (this.hasPendingVertex) { | |
| this.color(red, green, blue, alpha); | |
| } |
Steps of Reproduction ✅
1. During entity rendering, `EntityRenderDispatcher.render(...)` is invoked with
`StorageVertexConsumerProvider` as the `VertexConsumerProvider` (see
`EntityProxy.java:150-160` where `entityRenderDispatcher.render(entity, ...,
storageVertexConsumerProvider, ...)` is called).
2. Inside that render path, vanilla or modded renderers obtain a `VertexConsumer` for a
quad-based `RenderLayer` by calling `storageVertexConsumerProvider.getBuffer(layer)`,
which returns a `PBRVertexConsumer` when `drawMode == QUADS` (see
`StorageVertexConsumerProvider.getBuffer`, `StorageVertexConsumerProvider.java:25-41`,
especially line 36 where `new PBRVertexConsumer(initialSize, renderLayer)` is created).
3. The Minecraft `VertexConsumer` contract allows `fixedColor()` to be called before any
vertices, and this pattern is already assumed in this codebase:
`StorageOutlineVertexConsumerProvider.OutlineVertexConsumer` implements `fixedColor(...)`
as a no-op (no state checks) at `StorageOutlineVertexConsumerProvider.java:22-28`,
indicating callers may legitimately invoke `fixedColor()` regardless of vertex state.
4. When such a renderer calls `fixedColor(...)` on a `PBRVertexConsumer` before any
`vertex(...)` call, execution reaches `PBRVertexConsumer.fixedColor(...)` at
`PBRVertexConsumer.java:112-115`, which directly calls `this.color(...)`. `color(...)`
first calls `ensureVertexStarted()` (`PBRVertexConsumer.java:53-56`), which checks
`hasPendingVertex` and throws `IllegalStateException("Not currently building vertex")` if
no vertex has been begun, causing the render pass using this consumer to fail with an
exception instead of treating `fixedColor()` as a safe no-op.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/radiance/client/vertex/PBRVertexConsumer.java
**Line:** 373:373
**Comment:**
*Logic Error: The current implementation of the fixed-color API delegates directly to the per-vertex color method, which enforces that a vertex is already in progress; this breaks the VertexConsumer contract where calling fixedColor() before vertex() should be safe and effectively a no-op, and will cause an IllegalStateException when external code sets a fixed color before starting vertices.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| PBR_ALBEDO_EMISSION = | ||
| VertexFormatElement.register(23, 0, VertexFormatElement.ComponentType.UINT, | ||
| VertexFormatElement.Usage.UV, 1); | ||
| new VertexFormatElement(0, VertexFormatElement.ComponentType.UINT, |
There was a problem hiding this comment.
Suggestion: The vertex format element for the emission value is declared with an unsigned integer component type, but the rest of the pipeline (the albedoEmission field and flushPendingVertex in PBRVertexConsumer) writes and treats this attribute as a float, so the GPU will interpret the attribute with the wrong type and produce incorrect rendering results for emission. [type error]
Severity Level: Major ⚠️
- ❌ PBR_TRIANGLE vertex buffers mis-type AlbedoEmission attribute.
- ⚠️ Entity and block entity exports mis-decode emission channel.
Suggested change
| new VertexFormatElement(0, VertexFormatElement.ComponentType.UINT, | |
| new VertexFormatElement(0, VertexFormatElement.ComponentType.FLOAT, |
Steps of Reproduction ✅
1. During world rendering, the mixin in
`src/main/java/com/radiance/mixins/vulkan_render_integration/WorldRendererMixins.java:250-36`
collects visible entities and calls `EntityProxy.queueEntitiesBuild(camera,
renderedEntities, this.entityRenderDispatcher, tickDelta)` at line 26.
2. Inside `EntityProxy.queueEntitiesBuild` in
`src/main/java/com/radiance/client/proxy/world/EntityProxy.java:137-150`, a
`StorageVertexConsumerProvider` is created for each entity (lines 21-23) and passed to
`entityRenderDispatcher.render(...)` as the `VertexConsumerProvider` (line 29-31), so all
entity model rendering that uses QUADS will obtain vertex consumers from this provider.
3. In `StorageVertexConsumerProvider.getBuffer` at
`src/main/java/com/radiance/client/vertex/StorageVertexConsumerProvider.java:25-37`, when
`renderLayer.getDrawMode()` is `VertexFormat.DrawMode.QUADS` (line 35), the method
constructs a `PBRVertexConsumer` with the given `RenderLayer` (line 36). This
`PBRVertexConsumer` begins a buffer with `PBRVertexFormats.PBR_TRIANGLE` as its vertex
format in its constructor (`PBRVertexConsumer` at
`src/main/java/com/radiance/client/vertex/PBRVertexConsumer.java:74-80`).
4. The `PBR_TRIANGLE` vertex format is defined in
`src/main/java/com/radiance/client/vertex/PBRVertexFormats.java:29-51` and maps the
attribute name `"AlbedoEmission"` to `PBRVertexFormatElements.PBR_ALBEDO_EMISSION` via
`.put("AlbedoEmission", PBR_ALBEDO_EMISSION)` at line 48, so the layout for that attribute
is controlled by `PBR_ALBEDO_EMISSION`.
5. The definition of `PBR_ALBEDO_EMISSION` in
`src/main/java/com/radiance/client/vertex/PBRVertexFormatElements.java:92-95` uses `new
VertexFormatElement(0, VertexFormatElement.ComponentType.UINT,
VertexFormatElement.Type.UV, 1);`, i.e. the GPU-side vertex format declares this attribute
as a single unsigned integer component, not a float.
6. Every vertex flushed by `PBRVertexConsumer` goes through `flushPendingVertex()` in
`src/main/java/com/radiance/client/vertex/PBRVertexConsumer.java:180-245`. Near the end of
this method, it writes the emission value with `this.bufferBuilder.putFloat(0,
this.albedoEmission);` at line 233, so the actual vertex buffer stores this attribute as a
32-bit IEEE float at the offset corresponding to `"AlbedoEmission"`.
7. Callers can set this value as a float via `PBRVertexConsumer.albedoEmission(float
emission)` in `src/main/java/com/radiance/client/vertex/PBRVertexConsumer.java:360-363`,
which calls `ensureVertexStarted()` and then assigns `this.albedoEmission = emission;`.
Once any entity/block/entity-hand render path uses this method on the `VertexConsumer`
obtained in step 3, non-zero float emission values will be written into the vertex buffer.
8. When `EntityProxy.queueBuild` prepares draw parameters for native submission (see
`src/main/java/com/radiance/client/proxy/world/EntityProxy.java:389-66`), it derives a
`vertexFormatId` via `Constants.VertexFormats.getValue(drawParameters.format())` at line
417 based on the `VertexFormat` used by `PBRVertexConsumer`. This ID, together with the
vertex buffer address (lines 32-57), is passed into the native `queueBuild(...)` call at
lines 61-66. Because the Java-side vertex format for `"AlbedoEmission"` uses
`ComponentType.UINT`, the native side (and ultimately the GPU) will interpret that
attribute as an unsigned integer while the buffer actually contains floats written by
`putFloat`.
9. Under these conditions (rendering any entity or block entity on a QUADS render layer,
with client code calling `albedoEmission(float)` on the `PBRVertexConsumer` provided by
`StorageVertexConsumerProvider`), the emission channel will carry float data but be
decoded as integer data by the consumer defined via `PBR_ALBEDO_EMISSION`, leading to
incorrect per-vertex emission values in the PBR pipeline (e.g., emissive surfaces rendered
with wrong intensity or garbage values).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/radiance/client/vertex/PBRVertexFormatElements.java
**Line:** 94:94
**Comment:**
*Type Error: The vertex format element for the emission value is declared with an unsigned integer component type, but the rest of the pipeline (the `albedoEmission` field and `flushPendingVertex` in `PBRVertexConsumer`) writes and treats this attribute as a float, so the GPU will interpret the attribute with the wrong type and produce incorrect rendering results for emission.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+49
to
+50
| .put("PostBase", PBR_POST_BASE) | ||
| .put("Padding", PBR_PADDING) |
There was a problem hiding this comment.
Suggestion: The vertex format currently declares PostBase as a 3-float element (PBR_POST_BASE) plus a separate 4-byte Padding element, but PBRVertexConsumer.flushPendingVertex writes three floats (baseX/Y/Z) and then an additional 4-byte alphaMode value into this block without ever advancing into a distinct padding element. This means the extra 4 bytes are effectively written into a region the format describes as padding, making the declared element layout inconsistent with the actual data written and risking incorrect interpretation of the last 4 bytes by the rendering pipeline. Defining PostBase as a single 4-component float element and removing the separate padding entry aligns the format with the real vertex data layout and avoids writing real data into a padding-only element. [logic error]
Severity Level: Major ⚠️
- ❌ Vertex layout mismatch for all PBR_TRIANGLE-rendered quads.
- ⚠️ AlphaMode bytes stored where format declares padding only.
- ⚠️ Future attribute-based code may misinterpret PostBase/Padding region.
Suggested change
| .put("PostBase", PBR_POST_BASE) | |
| .put("Padding", PBR_PADDING) | |
| .put("PostBase", new VertexFormatElement(0, VertexFormatElement.ComponentType.FLOAT, | |
| VertexFormatElement.Type.GENERIC, 4)) |
Steps of Reproduction ✅
1. Trigger any normal world or entity rendering path that uses quad draw mode, which
creates a `PBRVertexConsumer` via `StorageVertexConsumerProvider.getBuffer(...)` at
`src/main/java/com/radiance/client/vertex/StorageVertexConsumerProvider.java:25-36` when
`drawMode == VertexFormat.DrawMode.QUADS`.
2. `PBRVertexConsumer` is constructed with `PBRVertexFormats.PBR_TRIANGLE` as its
`VertexFormat` (see constructor at
`src/main/java/com/radiance/client/vertex/PBRVertexConsumer.java:65-79`), so every vertex
built by this consumer uses the `PBR_TRIANGLE` layout defined at
`src/main/java/com/radiance/client/vertex/PBRVertexFormats.java:29-51`.
3. When vertices are finalized (e.g. during `endNullable()` calls in rendering),
`PBRVertexConsumer.flushPendingVertex()` at
`src/main/java/com/radiance/client/vertex/PBRVertexConsumer.java:180-245` is invoked: it
writes three floats `baseX/baseY/baseZ` at offsets 0, 4, 8 and then writes the 4‑byte
`alphaMode` at offset 12 before calling `nextElement()` once (lines 236‑240).
4. The vertex format metadata declares `PostBase` as a 3‑float element and introduces a
separate 4‑byte padding element: `PBR_POST_BASE` is defined as `ComponentType.FLOAT` with
count 3 at `src/main/java/com/radiance/client/vertex/PBRVertexFormatElements.java:87-90`,
and `PBR_PADDING` is defined as `ComponentType.BYTE`, `Type.PADDING`, count 4 at
`src/main/java/com/radiance/client/vertex/PBRVertexFormatElements.java:97-100`, then
`PBR_TRIANGLE` maps `"PostBase"` to `PBR_POST_BASE` and `"Padding"` to `PBR_PADDING` at
`src/main/java/com/radiance/client/vertex/PBRVertexFormats.java:49-50`. Because
`flushPendingVertex()` only advances the element once while writing 16 bytes starting at
offset 0, the 4‑byte `alphaMode` value is always written into the region that the vertex
format advertises as a separate padding element, so any downstream code that relies on the
Java `VertexFormat` metadata will treat those last 4 bytes as padding rather than a real
component of `PostBase`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/radiance/client/vertex/PBRVertexFormats.java
**Line:** 49:50
**Comment:**
*Logic Error: The vertex format currently declares `PostBase` as a 3-float element (`PBR_POST_BASE`) plus a separate 4-byte `Padding` element, but `PBRVertexConsumer.flushPendingVertex` writes three floats (baseX/Y/Z) and then an additional 4-byte `alphaMode` value into this block without ever advancing into a distinct padding element. This means the extra 4 bytes are effectively written into a region the format describes as padding, making the declared element layout inconsistent with the actual data written and risking incorrect interpretation of the last 4 bytes by the rendering pipeline. Defining `PostBase` as a single 4-component float element and removing the separate padding entry aligns the format with the real vertex data layout and avoids writing real data into a padding-only element.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
123
to
124
| } | ||
| ci.cancel(); |
There was a problem hiding this comment.
Suggestion: The injected update mixin unconditionally calls ci.cancel() even when dirty is false, meaning it always cancels the original LightmapTextureManager.update implementation even in cases where this mixin performs no work, which can break vanilla behavior and other mixins relying on the original method; ci.cancel() should only be invoked when dirty is true and the mixin has actually replaced the logic. [logic error]
Severity Level: Critical 🚨
- ❌ Cancels vanilla LightmapTextureManager.update body on most frames.
- ⚠️ Vulkan lightmap uniforms use stale block flicker intensity.
- ⚠️ Reduces compatibility with other mixins on update().
Suggested change
| } | |
| ci.cancel(); | |
| ci.cancel(); | |
| } |
Steps of Reproduction ✅
1. Launch the client with the Radiance mod so that the mixin in
`src/main/java/com/radiance/mixins/vulkan_render_integration/LightmapTextureManagerMixins.java:70-125`
is applied to `net.minecraft.client.render.LightmapTextureManager.update(float)`.
2. Join any world and let the game render normally; the vanilla engine calls
`LightmapTextureManager.update(float)` every frame from the rendering loop (GameRenderer),
while the mixin's injected method `redirectUpdate(float, CallbackInfo)` at line 71
executes at `@At("HEAD")`.
3. On a typical frame where the lightmap is not marked dirty (`this.dirty == false`), the
`if (this.dirty)` block at lines 72‑122 is skipped, but the unconditional `ci.cancel();`
call at line 124 still runs, cancelling the original `LightmapTextureManager.update`
method even though this mixin did not recompute lighting or touch the lightmap texture.
4. Because the original method body is cancelled on those non-dirty frames, vanilla
per-frame lightmap behavior (including updates to the shadowed `flickerIntensity` used at
line 102) is skipped, and other logic that relies on `LightmapTextureManager.update` is
affected; for example, `WorldRendererMixins` at
`src/main/java/com/radiance/mixins/vulkan_render_integration/WorldRendererMixins.java:35-44`
reads `radiance$getBlockFactor()` and related values for Vulkan uniforms, but
`blockFactor` is now based on a `flickerIntensity` that no longer follows vanilla's update
semantics once the method is always cancelled.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/radiance/mixins/vulkan_render_integration/LightmapTextureManagerMixins.java
**Line:** 123:124
**Comment:**
*Logic Error: The injected `update` mixin unconditionally calls `ci.cancel()` even when `dirty` is false, meaning it always cancels the original `LightmapTextureManager.update` implementation even in cases where this mixin performs no work, which can break vanilla behavior and other mixins relying on the original method; `ci.cancel()` should only be invoked when `dirty` is true and the mixin has actually replaced the logic.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| public void initRenderer(int debugVerbosity, boolean debugSync) { | ||
| long stackSize = 512 * 1024 * 1024; // 32MB | ||
| Runnable myRunnable = () -> { | ||
| private void initRenderer(int debugVerbosity, boolean debugSync, RunArgs args) { |
There was a problem hiding this comment.
Suggestion: The @reDIrect handler for replacing the call to RenderSystem.initRenderer(IZ)V declares an extra RunArgs parameter that is not part of the original call site, so its signature does not match the target and will cause the mixin to fail at runtime when applying this redirect. [type error]
Severity Level: Critical 🚨
- ❌ Client fails to start due to invalid redirect handler.
- ❌ Vulkan renderer initialization mixin cannot be applied.
- ❌ Radiance mod unusable; Fabric aborts client initialization.
Suggested change
| private void initRenderer(int debugVerbosity, boolean debugSync, RunArgs args) { | |
| private void initRenderer(int debugVerbosity, boolean debugSync) { |
Steps of Reproduction ✅
1. Install and enable the Radiance mod in a Fabric client; Fabric is configured to load
`radiance.mixins.json` via `src/main/resources/fabric.mod.json:27-29` (`"mixins":
["radiance.mixins.json"]`).
2. Start the Minecraft client; during startup, Fabric's Mixin subsystem reads
`src/main/resources/radiance.mixins.json:7-12`, which declares the required client mixin
`"vulkan_render_integration.MinecraftClientMixins"` (line 40) with `"required": true` and
`"injectors": {"defaultRequire": 1}` (lines 1-3, 59-61).
3. While applying `com.radiance.mixins.vulkan_render_integration.MinecraftClientMixins`
from
`src/main/java/com/radiance/mixins/vulkan_render_integration/MinecraftClientMixins.java:25-57`,
Mixin processes the `@Redirect` at lines 37-39 targeting
`Lcom/mojang/blaze3d/systems/RenderSystem;initRenderer(IZ)V`, which is a static method
with parameters `(int, boolean)`.
4. Mixin validates the redirect handler `initRenderer(int debugVerbosity, boolean
debugSync, RunArgs args)` at lines 39-57 and finds that the handler has an extra `RunArgs`
parameter not present in the target signature; this mismatched handler signature causes
Mixin to treat the redirect as invalid and, because the mixin configuration is required
(`required: true` and `defaultRequire: 1` in `radiance.mixins.json`), the client fails to
complete startup with a mixin application error before reaching the main menu.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/radiance/mixins/vulkan_render_integration/MinecraftClientMixins.java
**Line:** 39:39
**Comment:**
*Type Error: The @Redirect handler for replacing the call to RenderSystem.initRenderer(IZ)V declares an extra RunArgs parameter that is not part of the original call site, so its signature does not match the target and will cause the mixin to fail at runtime when applying this redirect.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| if (entity == focusedEntity && !camera.isThirdPerson() | ||
| && (!(focusedEntity instanceof LivingEntity livingEntity) | ||
| || !livingEntity.isSleeping())) { | ||
| renderedEntities.add(entity); |
There was a problem hiding this comment.
Suggestion: The conditional that checks whether the focused entity is being viewed in first-person and not sleeping currently still adds that entity to the render list, making the complex condition effectively a no-op and causing the player model to be rendered even in first-person when not sleeping; this contradicts the apparent intent of the check and typical camera behavior where the self-entity is hidden in that case. [logic error]
Severity Level: Major ⚠️
- ⚠️ First-person view includes full player model unexpectedly.
- ⚠️ Potential double-rendering with separate hand rendering pipeline.
- ⚠️ Visual behavior diverges from vanilla first-person expectations.
Suggested change
| renderedEntities.add(entity); |
Steps of Reproduction ✅
1. Start the client with the Radiance mod so that `WorldRendererMixins` is active via
`src/main/resources/radiance.mixins.json` line 18
(`"vulkan_render_integration.WorldRendererMixins"`), and join any world so that
`WorldRenderer.render(...)` is invoked every frame.
2. In normal gameplay, use the default first-person perspective; at runtime the
`@Inject`ed `redirectRender(...)` method in `WorldRendererMixins.java` lines 68–72 runs
each frame and builds the `renderedEntities` list from `this.world.getEntities()` (lines
26–54 in the excerpt starting at offset 220).
3. When iterating entities, for the local player entity (which is the camera's focused
entity) in first-person and not sleeping, the condition at lines 266–271:
`if (entity == focusedEntity && !camera.isThirdPerson() && (!(focusedEntity instanceof
LivingEntity livingEntity) || !livingEntity.isSleeping())) {
renderedEntities.add(entity); continue; }`
evaluates to `true`, so the player entity is added to `renderedEntities` inside the
`if` block and the loop continues, and for all other entities the unconditional
`renderedEntities.add(entity);` at line 272 also adds them, meaning the focused player
entity is always present in `renderedEntities` even in first-person.
4. The filled list is then passed to `EntityProxy.queueEntitiesBuild(camera,
renderedEntities, this.entityRenderDispatcher, tickDelta)` at lines 275–282 in
`WorldRendererMixins.java`, which in `EntityProxy.queueEntitiesBuild(...)`
(`src/main/java/com/radiance/client/proxy/world/EntityProxy.java` lines 18–55) renders
every entity in the list, while first-person hands are rendered separately by
`EntityProxy.queueHandRebuild(...)` invoked from
`GameRendererMixins.redirectRenderHand(...)`
(`src/main/java/com/radiance/mixins/vulkan_render_integration/GameRendererMixins.java`
lines 10–16), causing the local player model to be rendered in first-person despite the
complex condition that appears intended to suppress it.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/java/com/radiance/mixins/vulkan_render_integration/WorldRendererMixins.java
**Line:** 269:269
**Comment:**
*Logic Error: The conditional that checks whether the focused entity is being viewed in first-person and not sleeping currently still adds that entity to the render list, making the complex condition effectively a no-op and causing the player model to be rendered even in first-person when not sleeping; this contradicts the apparent intent of the check and typical camera behavior where the self-entity is hidden in that case.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the project to Minecraft 1.20.1, requiring extensive refactoring of the rendering pipeline. Key changes include updating Gradle dependencies, migrating from BuiltBuffer to BufferBuilder APIs, and adapting shader handling. Custom UI screens have been simplified, and numerous mixins related to font, banner, particle, block model, fluid, and screen blur rendering have been removed or significantly altered to align with the new Minecraft version. The review identifies several areas for improvement: the build process uses fragile reflection, shader pipeline mapping relies on brittle string matching, there is code duplication in RadianceClient.java, alpha mode determination uses fragile string matching, exception handling in ChunkProxy.java could be cleaner, and the unusual renderer initialization pattern lacks documentation.
Comment on lines
+116
to
+127
| def stripArchiveCopyAction = { task -> | ||
| def filteredActions = task.actions.findAll { action -> | ||
| if (action.class.name != "org.gradle.api.internal.project.taskfactory.StandardTaskAction") { | ||
| return true | ||
| } | ||
|
|
||
| def methodField = action.class.getDeclaredField("method") | ||
| methodField.accessible = true | ||
| methodField.get(action).name != "copy" | ||
| } | ||
| task.setActions(filteredActions) | ||
| } |
There was a problem hiding this comment.
This stripArchiveCopyAction closure uses reflection to access and modify internal Gradle APIs (org.gradle.api.internal.project.taskfactory.StandardTaskAction). This is a very fragile approach that is likely to break with future updates to Gradle or the Fabric Loom plugin.
While this might be a necessary workaround for a current issue, it would be more robust to find an officially supported API to achieve the same goal. If no such API exists, I recommend adding a prominent comment explaining why this reflection is needed and that it's a significant potential point of failure for future builds.
Comment on lines
+83
to
+133
| private static int mapShaderPipeline(String shaderName) { | ||
| String normalized = normalizeShaderName(shaderName); | ||
|
|
||
| return switch (normalized) { | ||
| case "rendertype_glint", "rendertype_glint_direct", "rendertype_glint_translucent", | ||
| "rendertype_entity_glint", "rendertype_entity_glint_direct", | ||
| "rendertype_armor_glint", "rendertype_armor_entity_glint", "position_tex" -> 0; | ||
| case "position_color", "rendertype_gui", "rendertype_gui_overlay", | ||
| "rendertype_gui_text_highlight", "rendertype_gui_ghost_recipe_overlay", | ||
| "rendertype_lines" -> 1; | ||
| case "position_tex_color", "rendertype_lightning" -> 2; | ||
| case "rendertype_text", "rendertype_text_background", | ||
| "rendertype_text_background_see_through", "rendertype_text_intensity", | ||
| "rendertype_text_intensity_see_through", "rendertype_text_see_through", | ||
| "particle" -> 3; | ||
| case "rendertype_entity_cutout", "rendertype_entity_cutout_no_cull", | ||
| "rendertype_entity_cutout_no_cull_z_offset", | ||
| "rendertype_entity_translucent", "rendertype_entity_translucent_cull", | ||
| "rendertype_entity_translucent_emissive", | ||
| "rendertype_item_entity_translucent_cull", "rendertype_entity_solid", | ||
| "rendertype_entity_smooth_cutout", "rendertype_entity_shadow", | ||
| "rendertype_entity_alpha", "rendertype_entity_decal", | ||
| "rendertype_energy_swirl", "rendertype_eyes", "rendertype_beacon_beam" -> 4; | ||
| case "rendertype_entity_no_outline", "rendertype_armor_cutout_no_cull" -> 5; | ||
| case "rendertype_end_portal", "rendertype_end_gateway" -> 6; | ||
| case "position" -> 7; | ||
| default -> -1; | ||
| }; | ||
| } | ||
|
|
||
| private static String normalizeShaderName(String shaderName) { | ||
| if (shaderName == null) { | ||
| return ""; | ||
| } | ||
|
|
||
| String normalized = shaderName.toLowerCase(Locale.ROOT).replace('\\', '/'); | ||
| int namespaceSeparator = normalized.indexOf(':'); | ||
| if (namespaceSeparator >= 0) { | ||
| normalized = normalized.substring(namespaceSeparator + 1); | ||
| } | ||
| if (normalized.startsWith("core/")) { | ||
| normalized = normalized.substring("core/".length()); | ||
| } | ||
| if (normalized.startsWith("minecraft/shaders/core/")) { | ||
| normalized = normalized.substring("minecraft/shaders/core/".length()); | ||
| } | ||
| if (normalized.endsWith(".json")) { | ||
| normalized = normalized.substring(0, normalized.length() - ".json".length()); | ||
| } | ||
| return normalized; | ||
| } |
There was a problem hiding this comment.
The mapShaderPipeline and normalizeShaderName methods rely on hardcoded string matching of shader names to determine the correct overlay pipeline. This is very brittle and will break if Mojang renames shaders in a future version.
To make this more maintainable, consider:
- Adding a prominent comment warning that this is a fragile part of the code that needs checking with every Minecraft version update.
- Exploring a data-driven approach, where these mappings could be loaded from a configuration file, making them easier to update without recompiling the mod.
Comment on lines
+102
to
108
| } catch (AccessDeniedException e) { | ||
| if (Files.exists(targetPath)) { | ||
| LOGGER.warn("Using existing locked native/resource file {}", targetPath); | ||
| return; | ||
| } | ||
| throw new RuntimeException(e); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
This catch (AccessDeniedException e) block is nearly identical to the one in copyOptionalFileFromResource (lines 121-127). To avoid code duplication and improve maintainability, consider extracting this logic into a private helper method.
For example:
private void handleAccessDenied(AccessDeniedException e, Path targetPath, String fileType) {
if (Files.exists(targetPath)) {
LOGGER.warn("Using existing locked {} file {}", fileType, targetPath);
return;
}
throw new RuntimeException(e);
}This could then be called from both catch blocks, like handleAccessDenied(e, targetPath, "native/resource");.
Comment on lines
+168
to
+170
| if (multiPhase.name.contains("translucent") || multiPhase.name.contains("glint") | ||
| || multiPhase.name.contains("lightning") || multiPhase.name.contains("crumbling") | ||
| || multiPhase.name.contains("tripwire")) { |
There was a problem hiding this comment.
The logic to determine if a render layer is transparent relies on string matching against the layer's name (e.g., multiPhase.name.contains("translucent")). This approach is fragile and may break if Mojang renames these render layers in a future update.
The previous implementation used multiPhase.isTranslucent(), which is generally more robust. If that method is no longer available or suitable in 1.20.1, I'd recommend investigating if there's another property or flag on the RenderLayer or its phases that can be used to identify translucent layers more reliably than by name.
Comment on lines
104
to
106
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
It's good practice to re-interrupt the thread when catching an InterruptedException. You've done this correctly here, which is great for ensuring the interrupted status isn't lost. However, you've split the catch block for InterruptedException and ExecutionException. You can handle the InterruptedException within the same block and rethrow a RuntimeException after setting the interrupt flag for cleaner code.
Suggested change
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| throw new RuntimeException(e); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| throw new RuntimeException(e); | |
| } catch (ExecutionException e) { | |
| throw new RuntimeException(e); | |
| } |
Comment on lines
+39
to
53
| private void initRenderer(int debugVerbosity, boolean debugSync, RunArgs args) { | ||
| long stackSize = 512L * 1024L * 1024L; | ||
| Runnable initTask = () -> { | ||
| RendererProxy.initRenderer(window); | ||
| Pipeline.collectNativeModules(); | ||
| }; | ||
|
|
||
| Thread myThread = new Thread(null, myRunnable, "", stackSize); | ||
| myThread.start(); | ||
| Thread initThread = new Thread(null, initTask, "radiance-renderer-init", stackSize); | ||
| initThread.start(); | ||
| try { | ||
| myThread.join(); | ||
| initThread.join(); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
The renderer initialization is performed on a new thread with a large stack size, which is then immediately joined. This pattern is unusual and adds some overhead.
If this is necessary because the native initialization code has deep recursion or large stack allocations, it would be beneficial to add a comment explaining this requirement. This would clarify the rationale for future maintainers and help prevent accidental removal of this threading logic.
Comment on lines
+112
to
+114
| return multiPhase.name.contains("translucent") || multiPhase.name.contains("glint") | ||
| || multiPhase.name.contains("lightning") ? ALPHA_MODE_TRANSPARENT : | ||
| ALPHA_MODE_CUTOUT; |
There was a problem hiding this comment.
This logic for determining the alpha mode relies on string matching against the render layer's name. This is fragile and can break on future Minecraft updates if layer names change. It would be more robust to use a property of the RenderLayer or its phases if a suitable one exists (e.g., checking the transparency phase property).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CodeAnt-AI Description
Port Radiance to Minecraft 1.20.1
What Changed
Impact
✅ 1.20.1 game startup✅ Fewer startup failures from locked files✅ Working settings access on the new game version💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.