Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUnifies and extends 3D rendering across components (Image, Item, Line, Rect, Text, RenderElement), adds Surface lighting controls and propagation, fixes Draw3D/primitive bugs, adds mixin accessors for item rendering internals, removes Fabric/Forge frame-pass hooks, and adds automated and visual Draw3D test suites. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Surface
participant RenderElement
participant PoseStack
participant MultiBufferSource
participant RenderPipelines
Client->>Surface: render(poseStack, consumers, packedLight, seeThrough)
Surface->>RenderElement: for each element: render3D(poseStack, consumers, light, seeThrough, delta)
RenderElement->>PoseStack: push / translate / rotate / scale
RenderElement->>MultiBufferSource: request VertexConsumer
alt seeThrough requires toggle
RenderElement->>RenderPipelines: (reflection) set depthTestFunction(DEPTH_ALWAYS)
end
RenderElement->>MultiBufferSource: emit vertices via VertexConsumer
alt consumers is BufferSource
MultiBufferSource->>MultiBufferSource: endBatch()
end
alt depthTest toggled
RenderElement->>RenderPipelines: restore original depthTestFunction
end
RenderElement->>PoseStack: pop
Surface-->>Client: render complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java (1)
858-874:⚠️ Potential issue | 🟠 MajorBuilder's zIndexScale is not applied to the built Surface.
The Builder's
zIndexScalefield (set via thezIndex()method) is never transferred to the constructed Surface instance in thebuild()method. This means any custom zIndexScale value set on the Builder is silently ignored, and the Surface will always use its default value of0.001.🐛 Proposed fix
.setRotateCenter(rotateCenter) .setRotateToPlayer(rotateToPlayer) .bindToEntity(boundEntity) .setBoundOffset(boundOffset); + surface.zIndexScale = zIndexScale; surface.lightMode = lightMode; surface.customLight = customLight; return surface;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 858 - 874, The Builder's zIndexScale value (set by zIndex()) isn't copied into the Surface when building; in build() (class Builder, method build) assign the Builder's zIndexScale to the constructed Surface (e.g., set surface.zIndexScale = this.zIndexScale) before returning so the custom zIndexScale is applied to the Surface instance.
🧹 Nitpick comments (7)
common/src/main/java/com/jsmacrosce/jsmacros/client/gui/overlays/EventChooser.java (1)
74-82: LGTM - Translation fallback logic is correct.The implementation properly checks for translation key existence and falls back to the raw event name when a translation isn't available. This ensures events without localized names still display correctly.
Optional: Consider extracting this logic into a shared utility method since
MacroContainer.getTranslatedEventName(lines 61-68) uses identical logic. This would reduce duplication and ensure consistent behavior if the translation key format ever changes.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/gui/overlays/EventChooser.java` around lines 74 - 82, Duplicate translation lookup logic appears in EventChooser (constructing translatedName) and MacroContainer.getTranslatedEventName; extract this into a single utility method (e.g., TranslationUtils.getTranslatedEventName or a static method on MacroContainer) that takes the raw eventName and returns a Component using the existing Locale and Language checks, then replace the inline logic in EventChooser and the existing block in MacroContainer.getTranslatedEventName to call that new method so behavior is consistent and duplication is removed.common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Box.java (1)
128-130: Fix looks correct; consider aligning Builder behavior.The
fixAlphacall here correctly ensures visibility when only color is provided (no explicit alpha). However,Builder.fillColor(int fillColor)at line 611 still assigns directly withoutfixAlpha, creating an inconsistency between instantiation paths.,
♻️ Optional: Apply same normalization in Builder
public Builder fillColor(int fillColor) { - this.fillColor = fillColor; + this.fillColor = ColorUtil.fixAlpha(fillColor); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Box.java` around lines 128 - 130, setFillColor(int) normalizes alpha via ColorUtil.fixAlpha, but Builder.fillColor(int fillColor) still assigns raw value causing inconsistent behavior; update the Builder.fillColor(int) method to assign the normalized color (use ColorUtil.fixAlpha(fillColor)) so both construction paths behave identically, and ensure any Builder chaining returns the Builder as before.docs/testing/draw3d_visual_tests.js (1)
150-154: Note: Entity tests require operator permissions.Tests V23-V25 use
/summon zombiewhich requires OP permissions. Consider documenting this prerequisite at the top of the file.📝 Suggested documentation addition
/* * draw3d_visual_tests.js * * Interactive Draw3D visual test suite. + * Prerequisites: + * - Must be in a world (singleplayer or server with OP) + * - Tests V23-V25 require /summon command permissions * Use chat: * !pass * !fail <reason> */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/testing/draw3d_visual_tests.js` around lines 150 - 154, Add a short note at the top of the test file clarifying that entity-based tests (e.g., those using summonEntityNearPlayer and the `/summon zombie` command used by tests V23–V25) require operator (OP) permissions; update the file header or a prominent README section to state the prerequisite (OP) and any recommended steps to grant OP for test runs so maintainers know to enable operator privileges before running these visual/entity tests.docs/testing/draw3d_auto_tests.js (2)
351-365: Document the known bug more prominently or track it for resolution.Test A24 explicitly validates a known bug where
Box.Builder.color(int,int)assigns tofillColorinstead ofcolor. While it's good to have a regression test for current behavior, consider:
- Adding a comment linking to a bug ticket if one exists
- Adding a
TODOto remove/update this test when the bug is fixedThe log note on line 362-364 helps, but this approach may cause confusion if the bug gets fixed and this test starts failing unexpectedly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/testing/draw3d_auto_tests.js` around lines 351 - 365, Add explicit documentation and a follow-up marker for the known Box.Builder.color(int,int) bug in this test: insert a short comment above the run('A24', ...) block referencing the bug ticket or issue ID (if available) and add a TODO note that the test should be removed or updated when the bug is fixed; keep the references to the specific API in the comment (Box.Builder.color(int,int), fillColor, color) so future readers know why the assertion expects fillColor behavior and can find the test for resolution tracking.
523-526: Test A40 has limited value and potential side effects.This test sets a static field to
falseand immediately asserts it'sfalse, which will always pass. The test doesn't verify any meaningful behavior. Additionally, modifying a static field could interfere with the actual rendering system if tests run while the game is active.Consider either:
- Testing actual dirty flag behavior (e.g., verifying it becomes
trueafter a relevant mutation)- Removing this test if the goal is just to verify field accessibility
🔧 Suggested improvement
run('A40', 'entityTraceLine_dirty_flag_access', () => { + // Store original value to restore after test + var original = EntityTraceLineClass.dirty; EntityTraceLineClass.dirty = false; assertTrue(EntityTraceLineClass.dirty === false, 'failed to set dirty=false'); + EntityTraceLineClass.dirty = original; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/testing/draw3d_auto_tests.js` around lines 523 - 526, The A40 test is a no-op that only sets EntityTraceLineClass.dirty = false and asserts it, which is meaningless and risky; either replace it with a meaningful behavior-driven test (e.g., perform the mutation that should mark the trace dirty and assert EntityTraceLineClass.dirty === true after calling the method that mutates state) or remove the test entirely; if you keep a test, use the existing run('A40', 'entityTraceLine_dirty_flag_access', ...) wrapper to 1) capture the original value of EntityTraceLineClass.dirty, 2) perform the action that should toggle the dirty flag (call the method or change the property that real code uses), 3) assert the expected dirty state, and 4) restore the original dirty value in a finally/teardown to avoid side effects during active rendering.common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java (2)
257-261: Consider validating light level inputs.The
setLightmethod acceptsblockLightandskyLightparameters documented as 0–15, but there's no validation. Values outside this range passed toLightTexture.pack()could produce unexpected packed light values.🛡️ Proposed validation
public Surface setLight(int blockLight, int skyLight) { + if (blockLight < 0 || blockLight > 15 || skyLight < 0 || skyLight > 15) { + throw new IllegalArgumentException("Light levels must be between 0 and 15"); + } this.lightMode = LightMode.CUSTOM; this.customLight = LightTexture.pack(blockLight, skyLight); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 257 - 261, The setLight method accepts blockLight and skyLight but doesn't validate they are within 0–15; update Surface.setLight to validate both inputs (blockLight and skyLight) before calling LightTexture.pack — if either value is outside the 0–15 range, throw an IllegalArgumentException (or optionally clamp to 0/15) with a clear message referencing the parameter and accepted range; keep the existing behavior of setting lightMode to LightMode.CUSTOM and assigning customLight via LightTexture.pack after validation.
835-839: Same validation concern applies to Builder.light().Like
Surface.setLight(), the Builder'slight()method should validate thatblockLightandskyLightare within 0–15.🛡️ Proposed validation
public Builder light(int blockLight, int skyLight) { + if (blockLight < 0 || blockLight > 15 || skyLight < 0 || skyLight > 15) { + throw new IllegalArgumentException("Light levels must be between 0 and 15"); + } this.lightMode = LightMode.CUSTOM; this.customLight = LightTexture.pack(blockLight, skyLight); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 835 - 839, Builder.light(int blockLight, int skyLight) currently sets customLight without validating inputs; add the same range checks used in Surface.setLight() to ensure blockLight and skyLight are within 0–15 and throw an IllegalArgumentException with a clear message if out of range, then set this.lightMode = LightMode.CUSTOM and this.customLight = LightTexture.pack(blockLight, skyLight) as before; reference the Builder.light method and LightTexture.pack (and mirror validation logic from Surface.setLight) so the builder enforces valid light values early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Image.java`:
- Around line 449-452: The instanceof check is using the wrong variable (vc) and
will never match; replace the check so it tests the MultiBufferSource parameter
(consumers) against MultiBufferSource.BufferSource and cast that to call
endBatch(); specifically, change the conditional that currently reads "vc
instanceof MultiBufferSource.BufferSource" to "consumers instanceof
MultiBufferSource.BufferSource" (binding to a variable like immediate) and call
immediate.endBatch(); apply the same fix pattern to the analogous checks in
Line.java (near the use of vc/consumers at the endBatch call) and Rect.java to
ensure endBatch() is invoked on the actual BufferSource instance.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.java`:
- Around line 295-335: The new >=1.21.10 3D rendering branch omits the
post-render overlay step (the same work done by render(..., is3dRender) via
drawContext.renderItemDecorations), causing overlays to be lost; after the layer
rendering loop in the block that uses ItemStackRenderState (the code that
iterates layerCount and calls itemRenderer.jsmacros$renderItem), add a shared
post-render overlay pass that invokes the same overlay-drawing logic used by the
fallback branch (i.e., call the drawContext.renderItemDecorations-equivalent
path or extract that overlay/text-drawing code into a helper and call it here)
so both branches render identical decorations and text labels; apply the same
change to the similar section referenced around the other occurrence (the
345-353 region) to keep parity.
- Around line 299-306: The mixin classes used in Item.java are not registered,
causing runtime cast failures; add the three mixin class names
MixinItemRenderer, MixinItemStackRenderState, and MixinItemStackRenderStateLayer
to the client array of the mixin configuration (jsmacrosce-common.mixins.json5)
so the mixins are applied at runtime and the casts in the Item rendering code
succeed.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Line.java`:
- Around line 379-381: The instanceof check is wrong: vc is a VertexConsumer and
will never be a MultiBufferSource.BufferSource, so replace the check to test the
actual buffer-source object (the MultiBufferSource used to obtain vc) rather
than vc, and call endBatch() on that buffer-source; in other words, where you
currently have "if (seeThrough && vc instanceof MultiBufferSource.BufferSource
immediate) { immediate.endBatch(); }" change the condition to inspect the
MultiBufferSource/BufferSource variable that produced vc and invoke endBatch()
on that variable (preserving the seeThrough guard).
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Rect.java`:
- Around line 407-409: The instanceof check uses vc (a VertexConsumer) but
should test the vertex-consumer-provider variable like in Image.java/Line.java;
replace the condition to check the provider (e.g., the variable named vb or the
VertexConsumerProvider/vertexConsumers instance) for
MultiBufferSource.BufferSource and call endBatch() on that casted immediate;
keep the seeThrough guard and the immediate.endBatch() call but change vc to the
provider variable and import/use MultiBufferSource.BufferSource as in the other
classes.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 269-271: The recomputeScale() method currently computes scale =
Math.min(sizes.x, sizes.y) / minSubdivisions which can divide by zero; update
recomputeScale() to guard against minSubdivisions == 0 (or negative) by first
validating minSubdivisions and using a safe fallback (e.g., treat zero as 1 or
skip recomputation) and/or clamp minSubdivisions to a minimum of 1 in
setMinSubdivisions(String)setMinSubdivisions(int) so that recomputeScale()
always divides by a positive non-zero value; reference the recomputeScale()
method, the minSubdivisions field, the scale field and the setter
setMinSubdivisions when making the change.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/mixin/access/MixinItemStackRenderState.java`:
- Around line 3-4: The unused org.spongepowered.asm.mixin.Mixin and
org.spongepowered.asm.mixin.gen.Accessor imports cause compiler warnings in the
fallback empty interface branch of MixinItemStackRenderState; to fix, either
move those two import statements into the version-conditional block that
declares the actual mixin (so they are only present when the mixin is compiled)
or annotate/suppress the unused-import warning for the fallback branch, ensuring
the change targets the MixinItemStackRenderState file and references the empty
fallback interface and the mixin-containing branch where the imports are
actually needed.
- Around line 6-23: Add the mixin classes
com.jsmacrosce.jsmacros.client.mixin.access.MixinItemStackRenderState and
com.jsmacrosce.jsmacros.client.mixin.access.MixinItemStackRenderStateLayer to
the client mixins array in the project's mixin config so the accessor interfaces
(e.g., jsmacros$getActiveLayerCount, jsmacros$getLayers) are generated at
runtime and the cast in Item.java to MixinItemStackRenderState no longer throws
ClassCastException.
---
Outside diff comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 858-874: The Builder's zIndexScale value (set by zIndex()) isn't
copied into the Surface when building; in build() (class Builder, method build)
assign the Builder's zIndexScale to the constructed Surface (e.g., set
surface.zIndexScale = this.zIndexScale) before returning so the custom
zIndexScale is applied to the Surface instance.
---
Nitpick comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Box.java`:
- Around line 128-130: setFillColor(int) normalizes alpha via
ColorUtil.fixAlpha, but Builder.fillColor(int fillColor) still assigns raw value
causing inconsistent behavior; update the Builder.fillColor(int) method to
assign the normalized color (use ColorUtil.fixAlpha(fillColor)) so both
construction paths behave identically, and ensure any Builder chaining returns
the Builder as before.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 257-261: The setLight method accepts blockLight and skyLight but
doesn't validate they are within 0–15; update Surface.setLight to validate both
inputs (blockLight and skyLight) before calling LightTexture.pack — if either
value is outside the 0–15 range, throw an IllegalArgumentException (or
optionally clamp to 0/15) with a clear message referencing the parameter and
accepted range; keep the existing behavior of setting lightMode to
LightMode.CUSTOM and assigning customLight via LightTexture.pack after
validation.
- Around line 835-839: Builder.light(int blockLight, int skyLight) currently
sets customLight without validating inputs; add the same range checks used in
Surface.setLight() to ensure blockLight and skyLight are within 0–15 and throw
an IllegalArgumentException with a clear message if out of range, then set
this.lightMode = LightMode.CUSTOM and this.customLight =
LightTexture.pack(blockLight, skyLight) as before; reference the Builder.light
method and LightTexture.pack (and mirror validation logic from Surface.setLight)
so the builder enforces valid light values early.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/gui/overlays/EventChooser.java`:
- Around line 74-82: Duplicate translation lookup logic appears in EventChooser
(constructing translatedName) and MacroContainer.getTranslatedEventName; extract
this into a single utility method (e.g., TranslationUtils.getTranslatedEventName
or a static method on MacroContainer) that takes the raw eventName and returns a
Component using the existing Locale and Language checks, then replace the inline
logic in EventChooser and the existing block in
MacroContainer.getTranslatedEventName to call that new method so behavior is
consistent and duplication is removed.
In `@docs/testing/draw3d_auto_tests.js`:
- Around line 351-365: Add explicit documentation and a follow-up marker for the
known Box.Builder.color(int,int) bug in this test: insert a short comment above
the run('A24', ...) block referencing the bug ticket or issue ID (if available)
and add a TODO note that the test should be removed or updated when the bug is
fixed; keep the references to the specific API in the comment
(Box.Builder.color(int,int), fillColor, color) so future readers know why the
assertion expects fillColor behavior and can find the test for resolution
tracking.
- Around line 523-526: The A40 test is a no-op that only sets
EntityTraceLineClass.dirty = false and asserts it, which is meaningless and
risky; either replace it with a meaningful behavior-driven test (e.g., perform
the mutation that should mark the trace dirty and assert
EntityTraceLineClass.dirty === true after calling the method that mutates state)
or remove the test entirely; if you keep a test, use the existing run('A40',
'entityTraceLine_dirty_flag_access', ...) wrapper to 1) capture the original
value of EntityTraceLineClass.dirty, 2) perform the action that should toggle
the dirty flag (call the method or change the property that real code uses), 3)
assert the expected dirty state, and 4) restore the original dirty value in a
finally/teardown to avoid side effects during active rendering.
In `@docs/testing/draw3d_visual_tests.js`:
- Around line 150-154: Add a short note at the top of the test file clarifying
that entity-based tests (e.g., those using summonEntityNearPlayer and the
`/summon zombie` command used by tests V23–V25) require operator (OP)
permissions; update the file header or a prominent README section to state the
prerequisite (OP) and any recommended steps to grant OP for test runs so
maintainers know to enable operator privileges before running these
visual/entity tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d802c2e-c9a9-4307-b559-ea0d6fdf2d82
📒 Files selected for processing (19)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/Draw3D.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Image.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Line.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Rect.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/RenderElement.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Text.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Box.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/containers/MacroContainer.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/gui/overlays/EventChooser.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/mixin/access/MixinItemRenderer.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/mixin/access/MixinItemStackRenderState.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/mixin/access/MixinItemStackRenderStateLayer.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/mixin/access/MixinScreen.javadocs/testing/draw3d_auto_tests.jsdocs/testing/draw3d_visual_tests.jsfabric/src/main/java/com/jsmacrosce/jsmacros/fabric/client/mixins/access/MixinLevelRenderer.javaneoforge/src/main/java/com/jsmacrosce/jsmacros/forge/client/forgeevents/ForgeEvents.java
💤 Files with no reviewable changes (2)
- fabric/src/main/java/com/jsmacrosce/jsmacros/fabric/client/mixins/access/MixinLevelRenderer.java
- neoforge/src/main/java/com/jsmacrosce/jsmacros/forge/client/forgeevents/ForgeEvents.java
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Image.java
Show resolved
Hide resolved
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.java
Show resolved
Hide resolved
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Line.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Rect.java
Outdated
Show resolved
Hide resolved
...on/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java
Show resolved
Hide resolved
common/src/main/java/com/jsmacrosce/jsmacros/client/mixin/access/MixinItemStackRenderState.java
Show resolved
Hide resolved
common/src/main/java/com/jsmacrosce/jsmacros/client/mixin/access/MixinItemStackRenderState.java
Show resolved
Hide resolved
Signed-off-by: Jack Manning <eastarctica@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java (1)
863-879:⚠️ Potential issue | 🟠 MajorBuilder's
zIndexScaleis never assigned to the created Surface.The Builder has a
zIndexScalefield (line 479) and setterzIndex()(lines 795-798), butbuild()never assigns it to the Surface. The Surface will always use its defaultzIndexScale = 0.001regardless of Builder configuration.🐛 Proposed fix
.setRotateCenter(rotateCenter) .setRotateToPlayer(rotateToPlayer) .bindToEntity(boundEntity) .setBoundOffset(boundOffset); + surface.zIndexScale = zIndexScale; surface.lightMode = lightMode; surface.customLight = customLight; return surface;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 863 - 879, The Builder's zIndexScale field is never copied into the created Surface, so set the built Surface's zIndexScale from the Builder before returning in Surface.build(); locate the Builder's zIndexScale field and zIndex() setter and add an assignment like surface.zIndexScale = this.zIndexScale (or use an existing Surface setter if preferred) just after creating the Surface and before returning it.
♻️ Duplicate comments (1)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java (1)
192-195:⚠️ Potential issue | 🟠 Major
setMinSubdivisionslacks the same validation as the constructor.The constructor (lines 64-68) clamps
minSubdivisionsto at least 1 with a warning, butsetMinSubdivisions()accepts any value directly. This allows callers to setminSubdivisionsto 0 or negative after construction, causing division by zero inrecomputeScale().🛡️ Proposed fix to add validation
public void setMinSubdivisions(int minSubdivisions) { - this.minSubdivisions = minSubdivisions; + int clamped = Math.max(minSubdivisions, 1); + if (minSubdivisions != clamped) { + JsMacros.LOGGER.warn("Invalid minSubdivisions: {}, clamping to 1", minSubdivisions); + } + this.minSubdivisions = clamped; recomputeScale(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 192 - 195, setMinSubdivisions currently assigns any value directly and calls recomputeScale(), allowing zero/negative values that will later cause division-by-zero; update setMinSubdivisions(int minSubdivisions) to validate and clamp the input exactly like the constructor does (ensure minSubdivisions >= 1, log a warning when clamping) before assigning to this.minSubdivisions and calling recomputeScale(), referencing the setMinSubdivisions method and the existing constructor validation logic to match behavior.
🧹 Nitpick comments (2)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java (2)
740-743: Builder'sminSubdivisions()silently accepts invalid values.While the
Surfaceconstructor clamps invalid values with a warning, the Builder accepts any value viaminSubdivisions(). Users configuring via Builder won't see the warning untilbuild()is called. Consider adding the same validation in the Builder for earlier feedback.💡 Optional: Add validation in Builder
public Builder minSubdivisions(int minSubdivisions) { + if (minSubdivisions < 1) { + JsMacros.LOGGER.warn("Invalid minSubdivisions: {}, will be clamped to 1", minSubdivisions); + } this.minSubdivisions = minSubdivisions; return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 740 - 743, The Builder.minSubdivisions(int) currently accepts any value silently; add the same validation/clamping and warning logic that's in the Surface constructor into Builder.minSubdivisions so invalid values are normalized and a warning logged immediately (instead of only at build()), then return this; ensure the check uses the same limits and message as the Surface constructor and keep build() behavior consistent with the validated field.
262-266: Consider documenting behavior for out-of-range light values.
setLight()accepts any int values forblockLightandskyLight, but the Javadoc states "0–15".LightTexture.pack()will mask values, so out-of-range inputs won't crash but may produce unexpected results.This is acceptable behavior, but you could either add input validation or clarify in the Javadoc that values are masked to 4 bits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 262 - 266, The setLight method accepts any ints but the Javadoc says "0–15" while LightTexture.pack masks values to 4 bits; update the Surface.setLight Javadoc to explicitly state that blockLight and skyLight are masked to their lower 4 bits (effectively modulo 16) by LightTexture.pack and thus out‑of‑range values will be wrapped, or alternatively implement input validation in Surface.setLight (e.g., clamp to 0–15 or throw IllegalArgumentException) before calling LightTexture.pack to enforce the documented range.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.java`:
- Around line 351-363: The 3D overlay branch (when overlay is true in
Item.render) draws only overlay text (using ovText, font, matrixStack and
font.drawInBatch) but omits the durability/damage bar that
drawContext.renderItemDecorations() produces in the 2D path; to fix, add
rendering of the item's durability bar in the overlay path by computing the same
bar parameters (based on item.getItem().isDamageableItem() / item.getMaxDamage()
and item.getDamageValue()) and drawing the two rectangles (background and
foreground) at the same relative position/scale before popping matrixStack, or
call the same helper used by drawContext.renderItemDecorations() if available,
ensuring to use the same light/consumers/matrixStack state so the damage bar
appears for damaged items just like in the 2D path.
- Around line 297-338: The code casts to MixinItemRenderer,
MixinItemStackRenderState, and MixinItemStackRenderStateLayer but those mixin
classes aren’t registered, causing ClassCastException at runtime; fix it by
adding the fully-qualified mixin class names (the classes under
./common/src/main/java/com/jsmacrosce/jsmacros/client/mixin/access/) to the
appropriate mixin config (e.g., add entries to the "client" array in
com.jsmacrosce.jsmacros.client.mixin — not the empty "client"/"mixins" arrays
currently present) so that MixinItemRenderer, MixinItemStackRenderState, and
MixinItemStackRenderStateLayer are loaded and available when the >=1.21.10
branch executes.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Line.java`:
- Around line 371-378: The pre-1.21.11 branch that uses
consumers.getBuffer(RenderType.lines()) doesn't apply the width field (unlike
the newer branch that calls setLineWidth(width)); update the older path to set
the line width via the legacy API (e.g., call RenderSystem.lineWidth(width)
before adding vertices and restore the default after) around the block that uses
PoseStack.Pose pose = matrixStack.last() and vc.addVertex(...), and if that
legacy API isn't available in some targets, add a runtime warning/log when width
!= 1.0 so users know custom widths aren't applied; reference the existing
symbols: consumers.getBuffer(RenderType.lines()), lineDepthTestFunction /
RenderPipelines.LINES, seeThrough, and the newer call setLineWidth(width).
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 371-384: resolveLightValue() currently samples WORLD light at
pos.toRawBlockPos(), causing moving entity-bound surfaces to use the static
position; change it to use the interpolated render position by accepting the
render position (e.g., pass Vec3 renderPos into resolveLightValue or compute
BlockPos from renderPos) and replace pos.toRawBlockPos() with a BlockPos derived
from renderPos before calling level.getBrightness(LightLayer.*, bp); update all
callers (places that invoked resolveLightValue()) to pass the current renderPos
used at render time so entity-interpolated surfaces sample lighting at their
actual rendered coordinates.
- Around line 314-320: Replace the manual interpolation against
boundEntity.getPos() with the entity's interpolated position: call
boundEntity.getPosition(tickDelta) (or expose it via EntityHelper) to obtain the
interpolated Vec3, convert it to Pos3D, add boundOffset, and then use that
resulting entityPos in the existing renderPos adjustment (the same place where
renderPos.x/y/z are updated). Update references around renderPos, boundEntity,
boundOffset, and Pos3D so the code uses the interpolated position like
EntityTraceLine.render does.
---
Outside diff comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 863-879: The Builder's zIndexScale field is never copied into the
created Surface, so set the built Surface's zIndexScale from the Builder before
returning in Surface.build(); locate the Builder's zIndexScale field and
zIndex() setter and add an assignment like surface.zIndexScale =
this.zIndexScale (or use an existing Surface setter if preferred) just after
creating the Surface and before returning it.
---
Duplicate comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 192-195: setMinSubdivisions currently assigns any value directly
and calls recomputeScale(), allowing zero/negative values that will later cause
division-by-zero; update setMinSubdivisions(int minSubdivisions) to validate and
clamp the input exactly like the constructor does (ensure minSubdivisions >= 1,
log a warning when clamping) before assigning to this.minSubdivisions and
calling recomputeScale(), referencing the setMinSubdivisions method and the
existing constructor validation logic to match behavior.
---
Nitpick comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 740-743: The Builder.minSubdivisions(int) currently accepts any
value silently; add the same validation/clamping and warning logic that's in the
Surface constructor into Builder.minSubdivisions so invalid values are
normalized and a warning logged immediately (instead of only at build()), then
return this; ensure the check uses the same limits and message as the Surface
constructor and keep build() behavior consistent with the validated field.
- Around line 262-266: The setLight method accepts any ints but the Javadoc says
"0–15" while LightTexture.pack masks values to 4 bits; update the
Surface.setLight Javadoc to explicitly state that blockLight and skyLight are
masked to their lower 4 bits (effectively modulo 16) by LightTexture.pack and
thus out‑of‑range values will be wrapped, or alternatively implement input
validation in Surface.setLight (e.g., clamp to 0–15 or throw
IllegalArgumentException) before calling LightTexture.pack to enforce the
documented range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f032a721-8cd4-48ca-abe3-ccd3033a202a
📒 Files selected for processing (6)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Image.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Line.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Rect.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Line3D.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.java
Show resolved
Hide resolved
Comment on lines
+351
to
+363
| if (overlay) { | ||
| String text = ovText != null ? ovText : (item.getCount() > 1 ? String.valueOf(item.getCount()) : null); | ||
| if (text != null) { | ||
| Font font = mc.font; | ||
| // Text offset | ||
| float tx = DEFAULT_ITEM_SIZE + 1 - font.width(text); | ||
| float ty = 9; | ||
| matrixStack.pushPose(); | ||
| matrixStack.translate(0, 0, OVERLAY_TEXT_Z_OFFSET); | ||
| font.drawInBatch(text, tx, ty, 0xFFFFFF, true, matrixStack.last().pose(), consumers, Font.DisplayMode.POLYGON_OFFSET, 0, light); | ||
| matrixStack.popPose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing durability bar rendering in overlay path.
The 3D rendering path only draws the count/overlay text but does not render the durability bar that drawContext.renderItemDecorations() provides in the 2D path (line 414). Items with durability will appear without their damage indicator when rendered on Surfaces with overlay=true.
Consider whether this is intentional or if the durability bar should be rendered as well for full parity with the 2D path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.java`
around lines 351 - 363, The 3D overlay branch (when overlay is true in
Item.render) draws only overlay text (using ovText, font, matrixStack and
font.drawInBatch) but omits the durability/damage bar that
drawContext.renderItemDecorations() produces in the 2D path; to fix, add
rendering of the item's durability bar in the overlay path by computing the same
bar parameters (based on item.getItem().isDamageableItem() / item.getMaxDamage()
and item.getDamageValue()) and drawing the two rectangles (background and
foreground) at the same relative position/scale before popping matrixStack, or
call the same helper used by drawContext.renderItemDecorations() if available,
ensuring to use the same light/consumers/matrixStack state so the damage bar
appears for damaged items just like in the 2D path.
Comment on lines
+371
to
+378
| VertexConsumer vc = consumers.getBuffer(RenderType.lines()); | ||
| try { | ||
| if (seeThrough) { | ||
| lineDepthTestFunction.set(RenderPipelines.LINES, DepthTestFunction.NO_DEPTH_TEST); | ||
| } | ||
| PoseStack.Pose pose = matrixStack.last(); | ||
| vc.addVertex(pose, x1, y1, 0).setColor(r, g, b, a).setNormal(pose, nx, ny, 0); | ||
| vc.addVertex(pose, x2, y2, 0).setColor(r, g, b, a).setNormal(pose, nx, ny, 0); |
There was a problem hiding this comment.
The width field is not applied in the pre-1.21.11 path.
The >=1.21.11 branch calls setLineWidth(width) on each vertex (line 368-369), but the <1.21.11 branch (lines 377-378) does not set line width at all. Lines will render at default width regardless of the width field value.
This appears to be a limitation of the older rendering API. If custom line widths are not possible in pre-1.21.11, consider documenting this limitation or logging a warning when a non-default width is used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Line.java`
around lines 371 - 378, The pre-1.21.11 branch that uses
consumers.getBuffer(RenderType.lines()) doesn't apply the width field (unlike
the newer branch that calls setLineWidth(width)); update the older path to set
the line width via the legacy API (e.g., call RenderSystem.lineWidth(width)
before adding vertices and restore the default after) around the block that uses
PoseStack.Pose pose = matrixStack.last() and vc.addVertex(...), and if that
legacy API isn't available in some targets, add a runtime warning/log when width
!= 1.0 so users know custom widths aren't applied; reference the existing
symbols: consumers.getBuffer(RenderType.lines()), lineDepthTestFunction /
RenderPipelines.LINES, seeThrough, and the newer call setLineWidth(width).
...on/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java
Outdated
Show resolved
Hide resolved
...on/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java
Show resolved
Hide resolved
Signed-off-by: Jack Manning <eastarctica@gmail.com>
Signed-off-by: Jack Manning <eastarctica@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.java`:
- Around line 282-295: The render3D method in Item.java takes a seeThrough
parameter but never uses it; update render3D to toggle depth testing based on
seeThrough (disable depth testing when seeThrough is true, re-enable it after
rendering) surrounding the item render calls so items render correctly on
surfaces with cull=false. Locate the render3D method (class Item) and add the
depth test enable/disable calls immediately after matrixStack.pushPose() (before
any drawing) and restore the depth test state just before matrixStack.popPose(),
ensuring the push/pop and rotation/scale logic (including rotateCenter and
Quaternionf usage) remain unchanged. Use the same depth toggling
approach/pattern used by Text/Image/Rect/Line components in the codebase to keep
behavior consistent. Ensure no other rendering state is left modified after the
method returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28b0ce06-a8a4-4b00-8a1b-3d5b15952a27
📒 Files selected for processing (1)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.java
Comment on lines
+282
to
+295
| public void render3D(PoseStack matrixStack, MultiBufferSource consumers, int light, boolean seeThrough, float delta) { | ||
| if (item == null) { | ||
| return; | ||
| } | ||
| matrixStack.pushPose(); | ||
| matrixStack.translate(x, y, 0); | ||
| matrixStack.scale((float) scale, (float) scale, 1); | ||
| if (rotateCenter) { | ||
| matrixStack.translate(DEFAULT_ITEM_SIZE / 2d, DEFAULT_ITEM_SIZE / 2d, 0); | ||
| matrixStack.mulPose(new Quaternionf().rotateLocalZ((float) Math.toRadians(rotation))); | ||
| matrixStack.translate(-DEFAULT_ITEM_SIZE / 2d, -DEFAULT_ITEM_SIZE / 2d, 0); | ||
| } else { | ||
| matrixStack.mulPose(new Quaternionf().rotateLocalZ((float) Math.toRadians(rotation))); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for seeThrough usage in other render components
rg -n -A5 'seeThrough' --type java -g '*/render/components/*.java'Repository: JsMacrosCE/JsMacros
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Find the render components directory and list files
find . -type d -name "components" | grep render | head -20Repository: JsMacrosCE/JsMacros
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Search for seeThrough usage more broadly across the codebase
rg -n 'seeThrough' --type javaRepository: JsMacrosCE/JsMacros
Length of output: 7050
🏁 Script executed:
#!/bin/bash
# Get context from Text.java render3D method
sed -n '282,310p' common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Text.javaRepository: JsMacrosCE/JsMacros
Length of output: 1092
🏁 Script executed:
#!/bin/bash
# Get context from Image.java render3D method where seeThrough is used
sed -n '381,460p' common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Image.javaRepository: JsMacrosCE/JsMacros
Length of output: 3867
🏁 Script executed:
#!/bin/bash
# Get the full Item.java render3D method to see current implementation
sed -n '282,320p' common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.javaRepository: JsMacrosCE/JsMacros
Length of output: 2195
🏁 Script executed:
#!/bin/bash
# Get more of the Item.java render3D method to see the actual item rendering call
sed -n '282,350p' common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.javaRepository: JsMacrosCE/JsMacros
Length of output: 3730
seeThrough parameter is unused.
The seeThrough parameter is received but never used in this method. Other render components (Text, Image, Rect, Line) actively use this parameter to control depth testing when rendering on Surfaces with cull=false. Without handling this parameter, items will render with depth testing enabled even when they should be visible through other surfaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components/Item.java`
around lines 282 - 295, The render3D method in Item.java takes a seeThrough
parameter but never uses it; update render3D to toggle depth testing based on
seeThrough (disable depth testing when seeThrough is true, re-enable it after
rendering) surrounding the item render calls so items render correctly on
surfaces with cull=false. Locate the render3D method (class Item) and add the
depth test enable/disable calls immediately after matrixStack.pushPose() (before
any drawing) and restore the depth test state just before matrixStack.popPose(),
ensuring the push/pop and rotation/scale logic (including rotateCenter and
Quaternionf usage) remain unchanged. Use the same depth toggling
approach/pattern used by Text/Image/Rect/Line components in the codebase to keep
behavior consistent. Ensure no other rendering state is left modified after the
method returns.
… use builtin getPos(partialTick)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java (1)
192-195:⚠️ Potential issue | 🟡 MinorClamp
minSubdivisionsin the setter too.The constructor now protects against invalid values, but
setMinSubdivisions()still allows0or negatives intorecomputeScale(). That can produce an invalidscaleand break width/height calculations and z-offset rendering.Suggested fix
public void setMinSubdivisions(int minSubdivisions) { - this.minSubdivisions = minSubdivisions; + this.minSubdivisions = Math.max(minSubdivisions, 1); + if (minSubdivisions != this.minSubdivisions) { + JsMacros.LOGGER.warn("Surface set with invalid minSubdivisions: {}, defaulting to 1", + minSubdivisions); + } recomputeScale(); }Also applies to: 274-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 192 - 195, setMinSubdivisions currently assigns values directly and calls recomputeScale(), allowing 0 or negative values to produce invalid scale; change setMinSubdivisions(int minSubdivisions) to clamp the incoming value to a valid range (e.g., Math.max(1, Math.min(minSubdivisions, this.maxSubdivisions))) before assigning and calling recomputeScale(), and apply the same clamping logic in the corresponding setter referenced at lines ~274-276 (e.g., setMaxSubdivisions) to ensure min ≤ max and both are ≥ 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 314-316: When computing renderPos for boundEntity, the code
currently uses the raw interpolated entity position and ignores boundOffset;
update the tracking branch so renderPos includes the configured boundOffset
(e.g., add boundOffset to the interpolated Pos3D) before calling
matrices.translate, and make the symmetric change at the other occurrence (the
block around the render path at 359-363) so both tracking and non-tracking
branches apply boundOffset consistently; reference symbols: boundOffset,
boundEntity, renderPos, Pos3D, matrices.translate.
- Around line 262-265: The method Surface.setLight(int blockLight, int skyLight)
currently calls LightTexture.pack(...) without validating that blockLight and
skyLight are within the documented 0–15 range; add an explicit precondition
check in this method (and the other Surface setLight overload that also calls
LightTexture.pack) to validate each component is between 0 and 15 and throw an
IllegalArgumentException with a clear message if not, before setting
lightMode/customLight so callers fail fast on invalid values.
---
Duplicate comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 192-195: setMinSubdivisions currently assigns values directly and
calls recomputeScale(), allowing 0 or negative values to produce invalid scale;
change setMinSubdivisions(int minSubdivisions) to clamp the incoming value to a
valid range (e.g., Math.max(1, Math.min(minSubdivisions, this.maxSubdivisions)))
before assigning and calling recomputeScale(), and apply the same clamping logic
in the corresponding setter referenced at lines ~274-276 (e.g.,
setMaxSubdivisions) to ensure min ≤ max and both are ≥ 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84ed1646-986e-4b15-95a6-4b82c47a324d
📒 Files selected for processing (2)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/helper/world/entity/EntityHelper.java
Comment on lines
+262
to
+265
| public Surface setLight(int blockLight, int skyLight) { | ||
| this.lightMode = LightMode.CUSTOM; | ||
| this.customLight = LightTexture.pack(blockLight, skyLight); | ||
| return this; |
There was a problem hiding this comment.
Validate light components before packing them.
Both public entry points document blockLight and skyLight as 0–15, but neither enforces that contract before calling LightTexture.pack(...). Out-of-range values will encode invalid packed-light data instead of failing fast.
Suggested fix
+ private static int validateLightComponent(String name, int value) {
+ if (value < 0 || value > 15) {
+ throw new IllegalArgumentException(name + " must be between 0 and 15");
+ }
+ return value;
+ }
+
public Surface setLight(int blockLight, int skyLight) {
this.lightMode = LightMode.CUSTOM;
- this.customLight = LightTexture.pack(blockLight, skyLight);
+ this.customLight = LightTexture.pack(
+ validateLightComponent("blockLight", blockLight),
+ validateLightComponent("skyLight", skyLight)
+ );
return this;
} public Builder light(int blockLight, int skyLight) {
this.lightMode = LightMode.CUSTOM;
- this.customLight = LightTexture.pack(blockLight, skyLight);
+ this.customLight = LightTexture.pack(
+ Surface.validateLightComponent("blockLight", blockLight),
+ Surface.validateLightComponent("skyLight", skyLight)
+ );
return this;
}Also applies to: 842-845
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`
around lines 262 - 265, The method Surface.setLight(int blockLight, int
skyLight) currently calls LightTexture.pack(...) without validating that
blockLight and skyLight are within the documented 0–15 range; add an explicit
precondition check in this method (and the other Surface setLight overload that
also calls LightTexture.pack) to validate each component is between 0 and 15 and
throw an IllegalArgumentException with a clear message if not, before setting
lightMode/customLight so callers fail fast on invalid values.
Comment on lines
+314
to
+316
| boolean isTrackingEntity = boundEntity != null && boundEntity.isAlive(); | ||
| Pos3D renderPos = isTrackingEntity ? new Pos3D(boundEntity.getRaw().getPosition(tickDelta)) : pos; | ||
| matrices.translate(renderPos.x, renderPos.y, renderPos.z); |
There was a problem hiding this comment.
Reapply boundOffset when following an entity.
The new interpolated position path ignores the configured offset entirely, so bound surfaces now render at the entity origin. As a downstream effect, WORLD lighting is also sampled from the wrong block for offset surfaces.
Suggested fix
boolean isTrackingEntity = boundEntity != null && boundEntity.isAlive();
- Pos3D renderPos = isTrackingEntity ? new Pos3D(boundEntity.getRaw().getPosition(tickDelta)) : pos;
+ Pos3D offset = boundOffset == null ? Pos3D.ZERO : boundOffset;
+ Pos3D renderPos = isTrackingEntity
+ ? new Pos3D(boundEntity.getRaw().getPosition(tickDelta)).add(offset)
+ : pos;
matrices.translate(renderPos.x, renderPos.y, renderPos.z);Also applies to: 359-363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`
around lines 314 - 316, When computing renderPos for boundEntity, the code
currently uses the raw interpolated entity position and ignores boundOffset;
update the tracking branch so renderPos includes the configured boundOffset
(e.g., add boundOffset to the interpolated Pos3D) before calling
matrices.translate, and make the symmetric change at the other occurrence (the
block around the render path at 359-363) so both tracking and non-tracking
branches apply boundOffset consistently; reference symbols: boundOffset,
boundEntity, renderPos, Pos3D, matrices.translate.
EastArctica
force-pushed
the
surface-fixes-clean
branch
from
f77d622 to
030e4e2
Compare
Collaborator
Author
|
abdabdad wtf did I do |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java (3)
192-195:⚠️ Potential issue | 🟠 MajorClamp or reject non-positive
minSubdivisionsin the setter.The constructor now guards invalid values, but
setMinSubdivisions()still allows0/negative input and immediately callsrecomputeScale(), which divides by that value. A single bad setter call can still drivescaletoInfinity/NaNor negative and break rendering.Suggested fix
public void setMinSubdivisions(int minSubdivisions) { - this.minSubdivisions = minSubdivisions; + if (minSubdivisions <= 0) { + JsMacros.LOGGER.warn("Surface setMinSubdivisions called with invalid value: {}, defaulting to 1", + minSubdivisions); + minSubdivisions = 1; + } + this.minSubdivisions = minSubdivisions; recomputeScale(); }Also applies to: 274-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 192 - 195, In setMinSubdivisions(int minSubdivisions) (and the similar setter at the other location), validate the input and prevent zero/negative values before assigning and calling recomputeScale(): either clamp minSubdivisions to a sensible minimum (e.g., 1) or throw an IllegalArgumentException for non-positive inputs; update the assignment to use the validated value (this.minSubdivisions = validatedValue) and only call recomputeScale() after validation to avoid division by zero/NaN in recomputeScale().
314-316:⚠️ Potential issue | 🟠 MajorReapply
boundOffsetin the interpolated entity-following path.
renderPosnow tracks the entity viagetPos(partialTicks), but it ignoresboundOffset, so bound surfaces render at the entity origin instead of the configured offset. Downstream, WORLD lighting is also sampled from the wrong block for offset surfaces because Line 363 uses this samerenderPos.Suggested fix
boolean isTrackingEntity = boundEntity != null && boundEntity.isAlive(); - Pos3D renderPos = isTrackingEntity ? new Pos3D(boundEntity.getPos(partialTicks)) : pos; + Pos3D offset = boundOffset == null ? Pos3D.ZERO : boundOffset; + Pos3D renderPos = isTrackingEntity + ? new Pos3D(boundEntity.getPos(partialTicks)).add(offset) + : pos; matrices.translate(renderPos.x, renderPos.y, renderPos.z);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 314 - 316, The interpolated entity-following path creates renderPos from boundEntity.getPos(partialTicks) but never applies the configured boundOffset, causing bound surfaces and subsequent WORLD lighting sampling to use the entity origin; update the logic in Surface (use the same conditional that builds renderPos) so that when isTrackingEntity is true you add/apply boundOffset to the interpolated position (e.g., compute renderPos = new Pos3D(boundEntity.getPos(partialTicks)) and then add boundOffset or create Pos3D.fromOffset) before calling matrices.translate(renderPos.x, renderPos.y, renderPos.z) so both rendering and downstream WORLD lighting use the offset position.
262-265:⚠️ Potential issue | 🟡 MinorValidate
blockLightandskyLightbefore packing.Both APIs document
0-15, but they currently accept anyintand forward it toLightTexture.pack(...). That silently produces invalid packed-light values instead of failing fast on bad input.Suggested fix
+ private static int validateLightComponent(String name, int value) { + if (value < 0 || value > 15) { + throw new IllegalArgumentException(name + " must be between 0 and 15"); + } + return value; + } + public Surface setLight(int blockLight, int skyLight) { this.lightMode = LightMode.CUSTOM; - this.customLight = LightTexture.pack(blockLight, skyLight); + this.customLight = LightTexture.pack( + validateLightComponent("blockLight", blockLight), + validateLightComponent("skyLight", skyLight) + ); return this; }public Builder light(int blockLight, int skyLight) { this.lightMode = LightMode.CUSTOM; - this.customLight = LightTexture.pack(blockLight, skyLight); + this.customLight = LightTexture.pack( + Surface.validateLightComponent("blockLight", blockLight), + Surface.validateLightComponent("skyLight", skyLight) + ); return this; }Also applies to: 842-845
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java` around lines 262 - 265, In Surface.setLight(int blockLight, int skyLight) (and the other similar overload around the 842–845 region), validate that blockLight and skyLight are within 0..15 before calling LightTexture.pack; if either value is outside that range, throw an IllegalArgumentException with a clear message (e.g., indicating which param and its invalid value) so the method fails fast instead of producing an invalid packed-light value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.java`:
- Around line 192-195: In setMinSubdivisions(int minSubdivisions) (and the
similar setter at the other location), validate the input and prevent
zero/negative values before assigning and calling recomputeScale(): either clamp
minSubdivisions to a sensible minimum (e.g., 1) or throw an
IllegalArgumentException for non-positive inputs; update the assignment to use
the validated value (this.minSubdivisions = validatedValue) and only call
recomputeScale() after validation to avoid division by zero/NaN in
recomputeScale().
- Around line 314-316: The interpolated entity-following path creates renderPos
from boundEntity.getPos(partialTicks) but never applies the configured
boundOffset, causing bound surfaces and subsequent WORLD lighting sampling to
use the entity origin; update the logic in Surface (use the same conditional
that builds renderPos) so that when isTrackingEntity is true you add/apply
boundOffset to the interpolated position (e.g., compute renderPos = new
Pos3D(boundEntity.getPos(partialTicks)) and then add boundOffset or create
Pos3D.fromOffset) before calling matrices.translate(renderPos.x, renderPos.y,
renderPos.z) so both rendering and downstream WORLD lighting use the offset
position.
- Around line 262-265: In Surface.setLight(int blockLight, int skyLight) (and
the other similar overload around the 842–845 region), validate that blockLight
and skyLight are within 0..15 before calling LightTexture.pack; if either value
is outside that range, throw an IllegalArgumentException with a clear message
(e.g., indicating which param and its invalid value) so the method fails fast
instead of producing an invalid packed-light value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22e28e61-8844-4862-9d89-6157bc0b2e8a
📒 Files selected for processing (2)
common/src/main/java/com/jsmacrosce/jsmacros/client/api/classes/render/components3d/Surface.javacommon/src/main/java/com/jsmacrosce/jsmacros/client/api/helper/world/entity/EntityHelper.java
Signed-off-by: Jack Manning <eastarctica@gmail.com>
Signed-off-by: Jack Manning <eastarctica@gmail.com>
Signed-off-by: Jack Manning <eastarctica@gmail.com>
Signed-off-by: Jack Manning <eastarctica@gmail.com>
Signed-off-by: Jack Manning <eastarctica@gmail.com>
Signed-off-by: Jack Manning <eastarctica@gmail.com>
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.
Box.setFillColoralphaText.widthprivate volatile, width should be gotten throughText.getWidth()docs/testing/