26.2refactor#138
Merged
Merged
Conversation
WalkthroughThe PR refactors the cape rendering pipeline by introducing a ChangesCape Rendering Pipeline Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/dev/tr7zw/waveycapes/support/EarsSupport.java (1)
80-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent entity access between
shouldBeUsedandgetCapeInfo.In
shouldBeUsed(line 44), the entity UUID is obtained viacapeRenderInfo.getAvatar()for version >= 1.21.9, but heregetCapeInfounconditionally usescapeRenderInfo.getEntity().getUUID(). IfgetAvatar()andgetEntity()return different objects with different UUIDs, theEarsFeatureslookup will be inconsistent between the two methods.🐛 Suggested fix: apply the same version-conditional pattern
`@Override` public CapeInfos getCapeInfo(PlayerWrapper capeRenderInfo) { - EarsFeatures playerFeatures = EarsFeatures.getById(capeRenderInfo.getEntity().getUUID()); + //? if >= 1.21.9 { + var entity = capeRenderInfo.getAvatar(); + //? } else { + /*var entity = capeRenderInfo.getEntity(); + *///? } + EarsFeatures playerFeatures = EarsFeatures.getById(entity.getUUID()); if (playerFeatures != null && playerFeatures.capeEnabled) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/tr7zw/waveycapes/support/EarsSupport.java` around lines 80 - 98, The getCapeInfo method has an inconsistency with the shouldBeUsed method in how it obtains the entity UUID for EarsFeatures lookup. Currently, getCapeInfo unconditionally uses capeRenderInfo.getEntity().getUUID() to fetch player features, but shouldBeUsed uses capeRenderInfo.getAvatar() for version 1.21.9 or later. Apply the same version-conditional pattern in the getCapeInfo method where you call EarsFeatures.getById() so that it uses getAvatar() for version >= 1.21.9 and getEntity() for earlier versions, matching the logic in shouldBeUsed to ensure consistent entity UUID lookups across both methods.
🧹 Nitpick comments (1)
src/main/java/dev/tr7zw/waveycapes/render/CapeRenderer.java (1)
17-17: ⚡ Quick winMake the nullable
getCapeInfocontract explicit.
getCapeInfo(...)is treated as nullable by callers and implementations, but the interface doesn’t state that. Please document this directly in the API to avoid misuse by future renderers/callers.Proposed contract clarification
public interface CapeRenderer { @@ - CapeInfos getCapeInfo(PlayerWrapper capeRenderInfo); + /** + * `@return` cape render info, or null when this renderer should not render for the given player. + */ + CapeInfos getCapeInfo(PlayerWrapper capeRenderInfo);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/tr7zw/waveycapes/render/CapeRenderer.java` at line 17, The getCapeInfo method signature does not explicitly document that it can return null, even though callers and implementations treat it as nullable. Add a nullable annotation (such as `@Nullable` from the appropriate null-safety library being used in the project) to the return type of the getCapeInfo method to make the nullable contract explicit in the API and prevent potential misuse by future implementations or callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/dev/tr7zw/waveycapes/render/CapeNodeCollector.java`:
- Around line 39-56: The code is using the preselected `renderer` parameter when
calling `customCapeRenderer.render()`, but it should use the renderer from
`capeInfo.capeRenderer()` instead. This mismatch causes the geometry and UV
behavior to desynchronize from the chosen render type and glint path. Replace
all occurrences of `renderer` with `capeInfo.capeRenderer()` in both the glint
rendering submission (within the `if (capeInfo.isGlint())` block) and the
regular cape rendering submission to ensure consistency between the render type
applied and the actual renderer used.
---
Outside diff comments:
In `@src/main/java/dev/tr7zw/waveycapes/support/EarsSupport.java`:
- Around line 80-98: The getCapeInfo method has an inconsistency with the
shouldBeUsed method in how it obtains the entity UUID for EarsFeatures lookup.
Currently, getCapeInfo unconditionally uses capeRenderInfo.getEntity().getUUID()
to fetch player features, but shouldBeUsed uses capeRenderInfo.getAvatar() for
version 1.21.9 or later. Apply the same version-conditional pattern in the
getCapeInfo method where you call EarsFeatures.getById() so that it uses
getAvatar() for version >= 1.21.9 and getEntity() for earlier versions, matching
the logic in shouldBeUsed to ensure consistent entity UUID lookups across both
methods.
---
Nitpick comments:
In `@src/main/java/dev/tr7zw/waveycapes/render/CapeRenderer.java`:
- Line 17: The getCapeInfo method signature does not explicitly document that it
can return null, even though callers and implementations treat it as nullable.
Add a nullable annotation (such as `@Nullable` from the appropriate null-safety
library being used in the project) to the return type of the getCapeInfo method
to make the nullable contract explicit in the API and prevent potential misuse
by future implementations or callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe96ab50-7a60-4a0b-b48e-53726a2df878
📒 Files selected for processing (24)
gradle-compose.ymlmodern.jsonreplacements.gradlesrc/main/java/dev/tr7zw/waveycapes/CapeNodeCollector.javasrc/main/java/dev/tr7zw/waveycapes/VanillaCapeRenderer.javasrc/main/java/dev/tr7zw/waveycapes/WaveyCapesBase.javasrc/main/java/dev/tr7zw/waveycapes/WaveyCapesConfigScreen.javasrc/main/java/dev/tr7zw/waveycapes/delegate/PlayerDelegate.javasrc/main/java/dev/tr7zw/waveycapes/mixin/FeatureRenderDispatcherMixin.javasrc/main/java/dev/tr7zw/waveycapes/mixin/LivingEntityRendererMixin.javasrc/main/java/dev/tr7zw/waveycapes/mixin/PlayerMixin.javasrc/main/java/dev/tr7zw/waveycapes/mixin/PlayerRendererMixin.javasrc/main/java/dev/tr7zw/waveycapes/render/CapeInfos.javasrc/main/java/dev/tr7zw/waveycapes/render/CapeNodeCollector.javasrc/main/java/dev/tr7zw/waveycapes/render/CapeRenderer.javasrc/main/java/dev/tr7zw/waveycapes/render/CustomCapeRenderer.javasrc/main/java/dev/tr7zw/waveycapes/render/VanillaCapeRenderer.javasrc/main/java/dev/tr7zw/waveycapes/renderlayers/CustomCapeRenderLayer.javasrc/main/java/dev/tr7zw/waveycapes/support/EarsSupport.javasrc/main/java/dev/tr7zw/waveycapes/support/MinecraftCapesSupport.javasrc/main/java/dev/tr7zw/waveycapes/support/ModSupport.javasrc/main/resources/waveycapes.mixins.jsonsrc/test/java/dev/tr7zw/tests/MixinTests.javaversions/mainProject
💤 Files with no reviewable changes (4)
- src/main/java/dev/tr7zw/waveycapes/mixin/FeatureRenderDispatcherMixin.java
- src/main/java/dev/tr7zw/waveycapes/CapeNodeCollector.java
- src/main/resources/waveycapes.mixins.json
- src/main/java/dev/tr7zw/waveycapes/VanillaCapeRenderer.java
Comment on lines
+39
to
+56
| var capeInfo = renderer.getCapeInfo(playerWrapper); | ||
| if (capeInfo == null) { | ||
| return; | ||
| } | ||
| //? if >= 1.21.9 { | ||
| if (capeInfo.isGlint()) { | ||
| submitNodeCollector.submitCustomGeometry(stack, RenderTypes.entityGlint(), (pose, vertexConsumer) -> { | ||
| PoseStack sharedStack = new PoseStack(); | ||
| sharedStack.last().set(pose); | ||
| customCapeRenderer.render(playerWrapper, renderer, vertexConsumer, sharedStack, packedLight, delta); | ||
| }); | ||
| } | ||
|
|
||
| submitNodeCollector.submitCustomGeometry(stack, capeInfo.renderType(), (pose, vertexConsumer) -> { | ||
| PoseStack sharedStack = new PoseStack(); | ||
| sharedStack.last().set(pose); | ||
| customCapeRenderer.render(playerWrapper, renderer, vertexConsumer, sharedStack, packedLight, delta); | ||
| }); |
There was a problem hiding this comment.
Use capeInfo.capeRenderer() for rendering, not the preselected renderer.
submitCape(...) applies capeInfo.renderType() but still calls customCapeRenderer.render(...) with the earlier renderer. If CapeInfos carries a different renderer, geometry/UV behavior can desync from the chosen render type/glint path.
Proposed fix
var capeInfo = renderer.getCapeInfo(playerWrapper);
if (capeInfo == null) {
return;
}
+ var activeRenderer = capeInfo.capeRenderer() != null ? capeInfo.capeRenderer() : renderer;
@@
if (capeInfo.isGlint()) {
submitNodeCollector.submitCustomGeometry(stack, RenderTypes.entityGlint(), (pose, vertexConsumer) -> {
PoseStack sharedStack = new PoseStack();
sharedStack.last().set(pose);
- customCapeRenderer.render(playerWrapper, renderer, vertexConsumer, sharedStack, packedLight, delta);
+ customCapeRenderer.render(playerWrapper, activeRenderer, vertexConsumer, sharedStack, packedLight, delta);
});
}
@@
submitNodeCollector.submitCustomGeometry(stack, capeInfo.renderType(), (pose, vertexConsumer) -> {
PoseStack sharedStack = new PoseStack();
sharedStack.last().set(pose);
- customCapeRenderer.render(playerWrapper, renderer, vertexConsumer, sharedStack, packedLight, delta);
+ customCapeRenderer.render(playerWrapper, activeRenderer, vertexConsumer, sharedStack, packedLight, delta);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/dev/tr7zw/waveycapes/render/CapeNodeCollector.java` around
lines 39 - 56, The code is using the preselected `renderer` parameter when
calling `customCapeRenderer.render()`, but it should use the renderer from
`capeInfo.capeRenderer()` instead. This mismatch causes the geometry and UV
behavior to desynchronize from the chosen render type and glint path. Replace
all occurrences of `renderer` with `capeInfo.capeRenderer()` in both the glint
rendering submission (within the `if (capeInfo.isGlint())` block) and the
regular cape rendering submission to ensure consistency between the render type
applied and the actual renderer used.
|
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.
No description provided.