Animations & CustomFOV: much more customization #857
Conversation
bypasses are temporary; " visuals are eternal
📝 Walkthrough📝 WalkthroughWalkthroughThe Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)src/main/java/keystrokesmod/module/impl/render/Animations.java (1)🔇 Additional comments (6)src/main/java/keystrokesmod/module/impl/render/Animations.java (6)
Currently,
The variables Consider renaming them as follows: - private static final double staticscalemultiplier_x = 1;
- private static final double staticscalemultiplier_y = 1;
- private static final double staticscalemultiplier_z = 1;
+ private static final double STATIC_SCALE_MULTIPLIER_X = 1;
+ private static final double STATIC_SCALE_MULTIPLIER_Y = 1;
+ private static final double STATIC_SCALE_MULTIPLIER_Z = 1;And update their usage accordingly.
Casting the result to Consider removing the cast to - event.setAnimationEnd((int) (event.getAnimationEnd() * ((-swingSpeed.getInput() / 100) + 1)));
+ event.setAnimationEnd(event.getAnimationEnd() * ((-swingSpeed.getInput() / 100f) + 1));Apply similar changes to the other instance involving
Rename the variable The variable name Apply this diff to rename the variable: - final float racism = MathHelper.sin(swingProgress * swingProgress / 64 * (float) Math.PI);
+ final float swingFactor = MathHelper.sin(swingProgress * swingProgress / 64 * (float) Math.PI);Also, update any references to this variable within the code.
Pass scaling inputs directly to the Currently, the Apply this diff to modify the method call: - this.scale(1, 1, 1);
+ this.scale(scalex.getInput(), scaley.getInput(), scalez.getInput());And adjust the private void scale(double scaleX, double scaleY, double scaleZ) {
GlStateManager.scale(
- scaleX * this.scalex.getInput(),
- scaleY * this.scaley.getInput(),
- scaleZ * this.scalez.getInput()
+ scaleX,
+ scaleY,
+ scaleZ
);
}
Modify The Apply this diff to update the private void rotate(float rotateX, float rotateY, float rotateZ) {
// x rotation
GlStateManager.rotate(
- (float) this.rotatex.getInput(),
+ rotateX,
1,
0,
0
);
// y rotation
GlStateManager.rotate(
- (float) this.rotatey.getInput(),
+ rotateY,
0,
1,
0
);
// z rotation
GlStateManager.rotate(
- (float) this.rotatez.getInput(),
+ rotateZ,
0,
0,
1
);
}Ensure that when calling
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range comments (2)
src/main/java/keystrokesmod/module/impl/render/Animations.java (2)
Line range hint
260-262: Rename the variableracismto a more appropriate nameUsing
racismas a variable name is inappropriate and may cause offense or misunderstanding. Consider renaming it to something that reflects its purpose in the code, such asprogressSinValueorswingSinValue.Apply this diff to rename the variable:
- final float racism = MathHelper.sin(swingProgress * swingProgress / 64 * (float) Math.PI); + final float progressSinValue = MathHelper.sin(swingProgress * swingProgress / 64 * (float) Math.PI);
Line range hint
145-307: Refactor duplicate code in switch statements for better maintainabilitySeveral cases within the
itemActionswitch statements contain similar code patterns with minor variations. Refactoring these into helper methods or using data structures to manage the variations can improve readability and maintainability.
| @@ -280,19 +366,63 @@ public void onPreMotion(PreMotionEvent event) { | |||
|
|
|||
| @SubscribeEvent | |||
| public void onSwingAnimation(@NotNull SwingAnimationEvent event) { | |||
| event.setAnimationEnd(event.getAnimationEnd() * (int) ((-swingSpeed.getInput() / 100) + 1)); | |||
| if(modifyAnimations.isToggled()) { | |||
| event.setAnimationEnd(event.getAnimationEnd() * (int) ((-swingSpeed.getInput() / 100) + 1)); | |||
There was a problem hiding this comment.
Avoid integer truncation when setting animation end in onSwingAnimation
In the onSwingAnimation method, casting the result to int may cause unintended truncation:
event.setAnimationEnd(event.getAnimationEnd() * (int) ((-swingSpeed.getInput() / 100) + 1));Consider removing the cast to int and using appropriate floating-point arithmetic to ensure correct animation timing.
Apply this diff to correct the calculation:
- event.setAnimationEnd(event.getAnimationEnd() * (int) ((-swingSpeed.getInput() / 100) + 1));
+ event.setAnimationEnd(event.getAnimationEnd() * ((-swingSpeed.getInput() / 100f) + 1));|
|
||
| private void translate(double x, double y, double z) { | ||
| GlStateManager.translate( | ||
| x + this.x.getInput(), | ||
| y + this.y.getInput(), | ||
| z + this.z.getInput() | ||
| x + this.translatex.getInput(), | ||
| y + this.translatey.getInput(), | ||
| z + this.translatez.getInput() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix double addition of translation values in translate() method
The translate() method adds the parameters x, y, z to the settings this.translatex.getInput(), this.translatey.getInput(), this.translatez.getInput(). When you call translate() with translatex.getInput(), etc., it results in doubling the translation values.
Consider changing the method to use only the parameters or the settings to avoid unintended behavior.
Apply this diff to fix the issue:
private void translate(double x, double y, double z) {
GlStateManager.translate(
- x + this.translatex.getInput(),
- y + this.translatey.getInput(),
- z + this.translatez.getInput()
+ x,
+ y,
+ z
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void translate(double x, double y, double z) { | |
| GlStateManager.translate( | |
| x + this.x.getInput(), | |
| y + this.y.getInput(), | |
| z + this.z.getInput() | |
| x + this.translatex.getInput(), | |
| y + this.translatey.getInput(), | |
| z + this.translatez.getInput() | |
| ); | |
| } | |
| private void translate(double x, double y, double z) { | |
| GlStateManager.translate( | |
| x, | |
| y, | |
| z | |
| ); | |
| } |
There was a problem hiding this comment.
translate is not only used to perform the base translation of an object but is also used in block animations to adjust the item to the correct position, its not double addition most of the time
There was a problem hiding this comment.
@Kefpull, understood. Thank you for the clarification about the translate() method and its role in adjusting the item's position during block animations.
✏️ Learnings added
Learnt from: Kefpull
PR: xia-mc/Raven-XD#857
File: src/main/java/keystrokesmod/module/impl/render/Animations.java:374-381
Timestamp: 2024-12-02T01:55:30.892Z
Learning: In `Animations.java`, the `translate()` method intentionally adds the parameters `x`, `y`, `z` to the settings `translatex.getInput()`, `translatey.getInput()`, and `translatez.getInput()` to adjust the item's position during block animations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| private void pretranslate(double x, double y, double z) { | ||
| GlStateManager.translate( | ||
| x + this.pretranslatex.getInput(), | ||
| y + this.pretranslatey.getInput(), | ||
| z + this.pretranslatez.getInput() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix double addition of translation values in pretranslate() method
Similar to the translate() method, the pretranslate() method adds the parameters to the settings values, which may cause translation values to be applied twice.
Apply this diff to fix the issue:
private void pretranslate(double x, double y, double z) {
GlStateManager.translate(
- x + this.pretranslatex.getInput(),
- y + this.pretranslatey.getInput(),
- z + this.pretranslatez.getInput()
+ x,
+ y,
+ z
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void pretranslate(double x, double y, double z) { | |
| GlStateManager.translate( | |
| x + this.pretranslatex.getInput(), | |
| y + this.pretranslatey.getInput(), | |
| z + this.pretranslatez.getInput() | |
| ); | |
| } | |
| private void pretranslate(double x, double y, double z) { | |
| GlStateManager.translate( | |
| x, | |
| y, | |
| z | |
| ); | |
| } |
| private static final double staticscalemultiplier_x = 1; | ||
| private static final double staticscalemultiplier_y = 1; | ||
| private static final double staticscalemultiplier_z = 1; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use camelCase for variable names to adhere to Java conventions
The variables staticscalemultiplier_x, staticscalemultiplier_y, and staticscalemultiplier_z should use camelCase naming conventions in Java.
Apply this diff to rename the variables:
- private static final double staticscalemultiplier_x = 1;
- private static final double staticscalemultiplier_y = 1;
- private static final double staticscalemultiplier_z = 1;
+ private static final double staticScaleMultiplierX = 1;
+ private static final double staticScaleMultiplierY = 1;
+ private static final double staticScaleMultiplierZ = 1;Also, update their usage in the scale() method.
Committable suggestion skipped: line range outside the PR's diff.
|
maybe it would be good to rename translate() to translateWithOffset() and just use the base function from gl11 to do the pre translation |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (3)
src/main/java/keystrokesmod/mixins/impl/client/MixinEntityRenderer.java(2 hunks)src/main/java/keystrokesmod/module/ModuleManager.java(2 hunks)src/main/java/keystrokesmod/module/impl/render/CustomFOV.java(1 hunks)
🔇 Additional comments (2)
src/main/java/keystrokesmod/module/impl/render/CustomFOV.java (1)
34-47: LGTM! Clean and focused FOV management.
The methods handle FOV state management appropriately with proper user feedback.
src/main/java/keystrokesmod/module/ModuleManager.java (1)
63-63: LGTM! Module registration follows existing patterns.
The CustomFOV module is properly declared and registered in the render category.
Also applies to: 276-276
| /* public static class DesiredFOV { | ||
| public final float DesiredFOV = (float) baseFOV.getInput(); | ||
|
|
||
| } | ||
|
|
||
|
|
||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented-out code.
The commented-out DesiredFOV class appears to be unused and can be safely removed.
| public void onFOVChange(@NotNull FOVUpdateEvent event) { | ||
| Utils.sendMessage("FOV Change"); | ||
| if(forceStatic.isToggled()){ | ||
| Utils.sendMessage("FOV Change"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove duplicate message in FOV change handler.
The same message is being sent twice when static mode is enabled.
public void onFOVChange(@NotNull FOVUpdateEvent event) {
Utils.sendMessage("FOV Change");
if(forceStatic.isToggled()){
- Utils.sendMessage("FOV Change");
+ Utils.sendMessage("Static mode is active");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void onFOVChange(@NotNull FOVUpdateEvent event) { | |
| Utils.sendMessage("FOV Change"); | |
| if(forceStatic.isToggled()){ | |
| Utils.sendMessage("FOV Change"); | |
| } | |
| } | |
| public void onFOVChange(@NotNull FOVUpdateEvent event) { | |
| Utils.sendMessage("FOV Change"); | |
| if(forceStatic.isToggled()){ | |
| Utils.sendMessage("Static mode is active"); | |
| } | |
| } |
| private static final SliderSetting baseFOV = new SliderSetting("FOV: ", 70, 1, 179, 1); | ||
| public final ButtonSetting forceStatic = new ButtonSetting("Static", false); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider extracting magic numbers as constants.
The settings are well-structured with appropriate ranges. Consider extracting the FOV values into named constants for better maintainability.
+ private static final float DEFAULT_FOV = 70.0F;
+ private static final float MIN_FOV = 1.0F;
+ private static final float MAX_FOV = 179.0F;
- private static final SliderSetting baseFOV = new SliderSetting("FOV: ", 70, 1, 179, 1);
+ private static final SliderSetting baseFOV = new SliderSetting("FOV: ", DEFAULT_FOV, MIN_FOV, MAX_FOV, 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static final SliderSetting baseFOV = new SliderSetting("FOV: ", 70, 1, 179, 1); | |
| public final ButtonSetting forceStatic = new ButtonSetting("Static", false); | |
| private static final float DEFAULT_FOV = 70.0F; | |
| private static final float MIN_FOV = 1.0F; | |
| private static final float MAX_FOV = 179.0F; | |
| private static final SliderSetting baseFOV = new SliderSetting("FOV: ", DEFAULT_FOV, MIN_FOV, MAX_FOV, 1); | |
| public final ButtonSetting forceStatic = new ButtonSetting("Static", false); |
| @SubscribeEvent(priority = EventPriority.LOWEST) | ||
| public void onUpdate() { | ||
|
|
||
| mc.gameSettings.fovSetting = (float) baseFOV.getInput(); | ||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider optimizing FOV updates.
The continuous FOV update in every tick might be inefficient. Consider updating only when the slider value changes.
@SubscribeEvent(priority = EventPriority.LOWEST)
public void onUpdate() {
+ float currentFOV = mc.gameSettings.fovSetting;
+ float desiredFOV = (float) baseFOV.getInput();
+ if (currentFOV != desiredFOV) {
mc.gameSettings.fovSetting = (float) baseFOV.getInput();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @SubscribeEvent(priority = EventPriority.LOWEST) | |
| public void onUpdate() { | |
| mc.gameSettings.fovSetting = (float) baseFOV.getInput(); | |
| } | |
| @SubscribeEvent(priority = EventPriority.LOWEST) | |
| public void onUpdate() { | |
| float currentFOV = mc.gameSettings.fovSetting; | |
| float desiredFOV = (float) baseFOV.getInput(); | |
| if (currentFOV != desiredFOV) { | |
| mc.gameSettings.fovSetting = (float) baseFOV.getInput(); | |
| } | |
| } |
| if (ModuleManager.customFOV != null && ModuleManager.customFOV.forceStatic != null) { | ||
|
|
||
| if (ModuleManager.customFOV.isEnabled() && ModuleManager.customFOV.forceStatic.isToggled()) { | ||
| cir.setReturnValue(CustomFOV.getDesiredFOV()); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve null safety in FOV modifier.
The null checks can be simplified using the null-safe operator if available in your Java version, or by extracting the conditions.
- if (ModuleManager.customFOV != null && ModuleManager.customFOV.forceStatic != null) {
- if (ModuleManager.customFOV.isEnabled() && ModuleManager.customFOV.forceStatic.isToggled()) {
- cir.setReturnValue(CustomFOV.getDesiredFOV());
- }
- }
+ CustomFOV fovModule = ModuleManager.customFOV;
+ if (fovModule != null && fovModule.forceStatic != null &&
+ fovModule.isEnabled() && fovModule.forceStatic.isToggled()) {
+ cir.setReturnValue(CustomFOV.getDesiredFOV());
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ModuleManager.customFOV != null && ModuleManager.customFOV.forceStatic != null) { | |
| if (ModuleManager.customFOV.isEnabled() && ModuleManager.customFOV.forceStatic.isToggled()) { | |
| cir.setReturnValue(CustomFOV.getDesiredFOV()); | |
| } | |
| } | |
| CustomFOV fovModule = ModuleManager.customFOV; | |
| if (fovModule != null && fovModule.forceStatic != null && | |
| fovModule.isEnabled() && fovModule.forceStatic.isToggled()) { | |
| cir.setReturnValue(CustomFOV.getDesiredFOV()); | |
| } |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (3)
src/main/java/keystrokesmod/event/FOVUpdateEvent.java(1 hunks)src/main/java/keystrokesmod/mixins/impl/client/MixinEntityRenderer.java(2 hunks)src/main/java/keystrokesmod/module/impl/render/CustomFOV.java(1 hunks)
🔇 Additional comments (2)
src/main/java/keystrokesmod/module/impl/render/CustomFOV.java (1)
23-23: 🧹 Nitpick (assertive)
Remove commented-out code.
The commented-out DescriptionSetting line should be removed as it's no longer needed.
src/main/java/keystrokesmod/mixins/impl/client/MixinEntityRenderer.java (1)
133-141: 🧹 Nitpick (assertive)
Improve null safety in FOV modifier.
The nested null checks and conditions can be simplified for better readability.
| @Cancelable | ||
| public class FOVUpdateEvent extends Event { | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding FOV-related fields to the event.
The event class should carry FOV-related data to allow handlers to access or modify the FOV value. This follows the common pattern in Forge events.
Consider adding these fields:
@Cancelable
public class FOVUpdateEvent extends Event {
+ private float fov;
+
+ public FOVUpdateEvent(float fov) {
+ this.fov = fov;
+ }
+
+ public float getFov() {
+ return fov;
+ }
+
+ public void setFov(float fov) {
+ this.fov = fov;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Cancelable | |
| public class FOVUpdateEvent extends Event { | |
| } | |
| @Cancelable | |
| public class FOVUpdateEvent extends Event { | |
| private float fov; | |
| public FOVUpdateEvent(float fov) { | |
| this.fov = fov; | |
| } | |
| public float getFov() { | |
| return fov; | |
| } | |
| public void setFov(float fov) { | |
| this.fov = fov; | |
| } | |
| } |
| public void onWorldChangeEvent(WorldChangeEvent event) { | ||
| mc.gameSettings.fovSetting = (float) setFOV.getInput(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize FOV update handlers.
Both event handlers perform the same operation without any synchronization. This could lead to race conditions and redundant updates.
Consider consolidating the FOV update logic:
+ private synchronized void updateFOV() {
+ float newFOV = (float) setFOV.getInput();
+ if (mc.gameSettings.fovSetting != newFOV) {
+ mc.gameSettings.fovSetting = newFOV;
+ }
+ }
+
@SubscribeEvent
public void onWorldChangeEvent(WorldChangeEvent event) {
- mc.gameSettings.fovSetting = (float) setFOV.getInput();
+ updateFOV();
}
@SubscribeEvent
public void onFOVUpdateEvent(FOVUpdateEvent event) {
- mc.gameSettings.fovSetting = (float) setFOV.getInput();
+ updateFOV();
}Also applies to: 45-48
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unnecessary empty lines.
Multiple consecutive empty lines at the end of the file should be removed.
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unnecessary empty lines.
Multiple consecutive empty lines at the end of the file should be removed.
| @Inject(method = "getFOVModifier", at = @At("RETURN"), cancellable = true) | ||
| public void onGetFOVModifier(@NotNull CallbackInfoReturnable<Float> cir) { | ||
|
|
||
| if (ModuleManager.customFOV != null && ModuleManager.customFOV.forceStaticFOV != null) { | ||
|
|
||
| if (ModuleManager.customFOV.isEnabled() && ModuleManager.customFOV.forceStaticFOV.isToggled()) { | ||
|
|
||
| cir.setReturnValue(CustomFOV.getDesiredFOV()); | ||
|
|
||
| } | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Fire FOVUpdateEvent before modifying FOV.
The FOV is being modified without firing the FOVUpdateEvent, which could lead to inconsistencies with other mods.
Consider firing the event:
@Inject(method = "getFOVModifier", at = @At("RETURN"), cancellable = true)
public void onGetFOVModifier(@NotNull CallbackInfoReturnable<Float> cir) {
+ FOVUpdateEvent event = new FOVUpdateEvent(cir.getReturnValue());
+ MinecraftForge.EVENT_BUS.post(event);
+ if (event.isCanceled()) {
+ return;
+ }
if (ModuleManager.customFOV != null && ModuleManager.customFOV.forceStaticFOV != null) {
if (ModuleManager.customFOV.isEnabled() && ModuleManager.customFOV.forceStaticFOV.isToggled()) {
cir.setReturnValue(CustomFOV.getDesiredFOV());
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Inject(method = "getFOVModifier", at = @At("RETURN"), cancellable = true) | |
| public void onGetFOVModifier(@NotNull CallbackInfoReturnable<Float> cir) { | |
| if (ModuleManager.customFOV != null && ModuleManager.customFOV.forceStaticFOV != null) { | |
| if (ModuleManager.customFOV.isEnabled() && ModuleManager.customFOV.forceStaticFOV.isToggled()) { | |
| cir.setReturnValue(CustomFOV.getDesiredFOV()); | |
| } | |
| } | |
| } | |
| @Inject(method = "getFOVModifier", at = @At("RETURN"), cancellable = true) | |
| public void onGetFOVModifier(@NotNull CallbackInfoReturnable<Float> cir) { | |
| FOVUpdateEvent event = new FOVUpdateEvent(cir.getReturnValue()); | |
| MinecraftForge.EVENT_BUS.post(event); | |
| if (event.isCanceled()) { | |
| return; | |
| } | |
| if (ModuleManager.customFOV != null && ModuleManager.customFOV.forceStaticFOV != null) { | |
| if (ModuleManager.customFOV.isEnabled() && ModuleManager.customFOV.forceStaticFOV.isToggled()) { | |
| cir.setReturnValue(CustomFOV.getDesiredFOV()); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
src/main/java/keystrokesmod/event/FOVUpdateEvent.java(1 hunks)src/main/java/keystrokesmod/mixins/impl/client/MixinEntityRenderer.java(2 hunks)src/main/java/keystrokesmod/module/ModuleManager.java(2 hunks)src/main/java/keystrokesmod/module/impl/render/Animations.java(14 hunks)src/main/java/keystrokesmod/module/impl/render/CustomFOV.java(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/main/java/keystrokesmod/module/impl/render/Animations.java (1)
Learnt from: Kefpull
PR: xia-mc/Raven-XD#857
File: src/main/java/keystrokesmod/module/impl/render/Animations.java:374-381
Timestamp: 2024-12-02T01:55:31.118Z
Learning: In `Animations.java`, the `translate()` method intentionally adds the parameters `x`, `y`, `z` to the settings `translatex.getInput()`, `translatey.getInput()`, and `translatez.getInput()` to adjust the item's position during block animations.
🔇 Additional comments (9)
src/main/java/keystrokesmod/module/impl/render/Animations.java (5)
374-381: Acknowledged intentional design in translate() method
The translate() method adds the parameters x, y, z to the settings translatex.getInput(), translatey.getInput(), and translatez.getInput(). Based on previous clarification, this design is intentional to adjust the item's position during block animations.
383-389: Acknowledged intentional design in pretranslate() method
Similarly, in the pretranslate() method, adding the parameters to the settings values is intentional to achieve the desired translation effect in block animations.
373-381: Potential duplication in translation values
While it's understood that adding parameters to settings in translate() is intentional, ensure that when calling translate(), you are not unintentionally doubling the translation values if the parameters already include the settings.
To confirm, you can review the calls to translate() and ensure that the parameters are correctly set.
59-61: 🧹 Nitpick (assertive)
Use CamelCase for constant variable names
The constants staticscalemultiplier_x, staticscalemultiplier_y, and staticscalemultiplier_z should follow Java naming conventions for constants, which typically use uppercase letters with underscores.
Consider renaming them as follows:
- private static final double staticscalemultiplier_x = 1;
- private static final double staticscalemultiplier_y = 1;
- private static final double staticscalemultiplier_z = 1;
+ private static final double STATIC_SCALE_MULTIPLIER_X = 1;
+ private static final double STATIC_SCALE_MULTIPLIER_Y = 1;
+ private static final double STATIC_SCALE_MULTIPLIER_Z = 1;Also, update their usage throughout the class accordingly.
370-370:
Avoid casting to int to prevent unintended truncation
In the onSwingAnimation method, casting the result to int may cause unintended truncation, leading to incorrect animation timing. It's better to use floating-point arithmetic to ensure smooth animations.
Consider modifying the code as follows:
- event.setAnimationEnd(event.getAnimationEnd() * (int) ((-swingSpeed.getInput() / 100) + 1));
+ event.setAnimationEnd(event.getAnimationEnd() * ((-swingSpeed.getInput() / 100f) + 1));src/main/java/keystrokesmod/event/FOVUpdateEvent.java (1)
6-8: 🛠️ Refactor suggestion
Consider adding FOV-related fields to the event
Currently, FOVUpdateEvent is an empty class. To make this event more useful, consider adding fields to hold the FOV value, allowing event handlers to access and modify it.
You can modify the class as follows:
@Cancelable
public class FOVUpdateEvent extends Event {
+ private float fov;
+
+ public FOVUpdateEvent(float fov) {
+ this.fov = fov;
+ }
+
+ public float getFov() {
+ return fov;
+ }
+
+ public void setFov(float fov) {
+ this.fov = fov;
+ }
}This will enable handlers to modify the FOV value as needed.
src/main/java/keystrokesmod/module/impl/render/CustomFOV.java (1)
47-48: 🧹 Nitpick (assertive)
Unnecessary modification of mc.gameSettings.fovSetting in event handler
In the onFOVUpdateEvent, you are directly modifying mc.gameSettings.fovSetting, which might not be necessary or could cause issues.
Consider setting the FOV value in the event instead:
@SubscribeEvent
public void onFOVUpdateEvent(FOVUpdateEvent event) {
- mc.gameSettings.fovSetting = (float) setFOV.getInput();
+ event.setFov((float) setFOV.getInput());
}This way, the FOV adjustment is handled through the event system.
Likely invalid or redundant comment.
src/main/java/keystrokesmod/mixins/impl/client/MixinEntityRenderer.java (1)
130-142:
Fire FOVUpdateEvent before modifying FOV
Currently, the onGetFOVModifier method modifies the FOV without firing the FOVUpdateEvent. To ensure compatibility and allow other modules to respond, you should fire the event and use its result.
Modify the method as follows:
@Inject(method = "getFOVModifier", at = @At("RETURN"), cancellable = true)
public void onGetFOVModifier(@NotNull CallbackInfoReturnable<Float> cir) {
+ FOVUpdateEvent event = new FOVUpdateEvent(cir.getReturnValue());
+ MinecraftForge.EVENT_BUS.post(event);
+ if (event.isCanceled()) {
+ cir.cancel();
+ return;
+ }
+ cir.setReturnValue(event.getFov());
if (ModuleManager.customFOV != null && ModuleManager.customFOV.forceStaticFOV != null) {
if (ModuleManager.customFOV.isEnabled() && ModuleManager.customFOV.forceStaticFOV.isToggled()) {
cir.setReturnValue(CustomFOV.getDesiredFOV());
}
}
}This ensures that the event is properly fired and handled.
src/main/java/keystrokesmod/module/ModuleManager.java (1)
63-63: Ensure correct instantiation of CustomFOV module
Make sure that customFOV is properly initialized and added to the module list to avoid any NullPointerException.
Confirm that customFOV is initialized before usage in other classes.
| this.rotate((float) rotatex.getInput(), (float) rotatey.getInput(), (float) rotatez.getInput()); | ||
|
|
||
| } |
There was a problem hiding this comment.
Rotate method may not apply rotations as expected
In the onRenderItem method, when customrotation is toggled, the rotate() method is called with parameters cast to float, but inside rotate(), the settings are retrieved again. This might lead to applying rotations twice or unexpected behavior.
Consider adjusting the rotate() method to use the parameters passed instead of retrieving the settings again:
private void rotate(float rotatex, float rotatey, float rotatez) {
- GlStateManager.rotate((float) this.rotatex.getInput(), 1, 0, 0);
- GlStateManager.rotate((float) this.rotatey.getInput(), 0, 1, 0);
- GlStateManager.rotate((float) this.rotatez.getInput(), 0, 0, 1);
+ GlStateManager.rotate(rotatex, 1, 0, 0);
+ GlStateManager.rotate(rotatey, 0, 1, 0);
+ GlStateManager.rotate(rotatez, 0, 0, 1);
}And ensure that you are passing the correct values when calling rotate().
Committable suggestion skipped: line range outside the PR's diff.
| staticStartSwingProgressFloat = ((float) staticStartSwingProgress.getInput()); | ||
| } else{ | ||
| staticStartSwingProgressFloat = 0.0F; | ||
| } |
There was a problem hiding this comment.
Initialize staticStartSwingProgressFloat outside the condition
Currently, staticStartSwingProgressFloat is conditionally initialized, which may lead to uninitialized usage if modifyAnimations is not toggled. It's advisable to initialize it outside the condition to ensure it always has a defined value.
Consider modifying the code as follows:
- if(modifyAnimations.isToggled()) {
- staticStartSwingProgressFloat = ((float) staticStartSwingProgress.getInput());
- } else{
- staticStartSwingProgressFloat = 0.0F;
- }
+ staticStartSwingProgressFloat = 0.0F;
+ if(modifyAnimations.isToggled()) {
+ staticStartSwingProgressFloat = ((float) staticStartSwingProgress.getInput());
+ }This ensures that staticStartSwingProgressFloat is always initialized before use.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| staticStartSwingProgressFloat = ((float) staticStartSwingProgress.getInput()); | |
| } else{ | |
| staticStartSwingProgressFloat = 0.0F; | |
| } | |
| staticStartSwingProgressFloat = 0.0F; | |
| if(modifyAnimations.isToggled()) { | |
| staticStartSwingProgressFloat = ((float) staticStartSwingProgress.getInput()); | |
| } |
| this.scale(1, 1, 1); | ||
| } |
There was a problem hiding this comment.
Scaling method uses hard-coded values
In the onRenderItem method, when customscaling is toggled, the scale() method is called with parameters (1, 1, 1), which may not reflect the intended custom scaling based on user input.
Consider passing the scaling inputs to the scale() method:
- this.scale(1, 1, 1);
+ this.scale(scalex.getInput(), scaley.getInput(), scalez.getInput());And adjust the scale() method accordingly to avoid multiplying the settings twice.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.scale(1, 1, 1); | |
| } | |
| this.scale(scalex.getInput(), scaley.getInput(), scalez.getInput()); | |
| } |
| savedFOVSetting = mc.gameSettings.fovSetting; | ||
|
|
||
| mc.gameSettings.fovSetting = (float) setFOV.getInput(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid directly modifying mc.gameSettings.fovSetting
Directly setting mc.gameSettings.fovSetting can lead to conflicts with other mods or Minecraft's internal handling. Instead, consider handling FOV adjustments via events or more controlled methods.
Since you have an FOVUpdateEvent, you can adjust the FOV in response to that event.
| private float savedFOVSetting; | ||
|
|
||
| private static final SliderSetting setFOV = new SliderSetting("FOV:", 70, 1, 179, 1); | ||
| public final ButtonSetting forceStaticFOV = new ButtonSetting("Static", false); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make setFOV a non-static member variable
The setFOV slider setting is declared as static, which might lead to unintended sharing of the setting across multiple instances if the class is ever instantiated more than once. Since settings are typically instance-specific, consider removing the static modifier.
- private static final SliderSetting setFOV = new SliderSetting("FOV:", 70, 1, 179, 1);
+ private final SliderSetting setFOV = new SliderSetting("FOV:", 70, 1, 179, 1);Also, update any static references to setFOV accordingly.
Committable suggestion skipped: line range outside the PR's diff.
Kefpull
changed the title
|
@xia-mc updated animations with swingspeedwhileblocking for merge |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/main/java/keystrokesmod/module/impl/render/Animations.java(14 hunks)
🧰 Additional context used
📓 Learnings (1)
src/main/java/keystrokesmod/module/impl/render/Animations.java (1)
Learnt from: Kefpull
PR: xia-mc/Raven-XD#857
File: src/main/java/keystrokesmod/module/impl/render/Animations.java:374-381
Timestamp: 2024-12-02T01:55:31.118Z
Learning: In `Animations.java`, the `translate()` method intentionally adds the parameters `x`, `y`, `z` to the settings `translatex.getInput()`, `translatey.getInput()`, and `translatez.getInput()` to adjust the item's position during block animations.
🔇 Additional comments (6)
src/main/java/keystrokesmod/module/impl/render/Animations.java (6)
60-62: 🧹 Nitpick (assertive)
Use camelCase naming conventions for variables
The variables staticscalemultiplier_x, staticscalemultiplier_y, and staticscalemultiplier_z should follow Java naming conventions by using camelCase. This enhances readability and maintains consistency throughout the codebase.
Apply this diff to rename the variables:
- private static final double staticscalemultiplier_x = 1;
- private static final double staticscalemultiplier_y = 1;
- private static final double staticscalemultiplier_z = 1;
+ private static final double staticScaleMultiplierX = 1;
+ private static final double staticScaleMultiplierY = 1;
+ private static final double staticScaleMultiplierZ = 1;Also, update their usage in the scale() method accordingly.
121-124:
Initialize staticStartSwingProgressFloat outside the condition
Currently, staticStartSwingProgressFloat is conditionally initialized, which may lead to unexpected behavior if it's accessed elsewhere without being initialized. It's advisable to initialize it before the condition to ensure it always has a defined value.
Consider modifying the code as follows:
+ staticStartSwingProgressFloat = 0.0F;
if(modifyAnimations.isToggled()) {
staticStartSwingProgressFloat = ((float) staticStartSwingProgress.getInput());
- } else{
- staticStartSwingProgressFloat = 0.0F;
}This ensures that staticStartSwingProgressFloat is always initialized before use.
137-137: 🛠️ Refactor suggestion
Scaling method uses hard-coded values
In the onRenderItem method, when customscaling is toggled, the scale() method is called with parameters (1, 1, 1), which may not reflect the intended custom scaling based on user input.
Consider passing the scaling inputs to the scale() method:
- this.scale(1, 1, 1);
+ this.scale(scalex.getInput(), scaley.getInput(), scalez.getInput());And adjust the scale() method accordingly to avoid multiplying the settings twice.
369-372:
Avoid integer truncation in animation timing calculations
In the onSwingAnimation method, casting the result to int may cause unintended truncation and loss of precision in the animation timing:
event.setAnimationEnd((int) (event.getAnimationEnd() * ((-swingSpeed.getInput() / 100) + 1)));Consider performing the multiplication with floating-point arithmetic and then casting the final result:
- event.setAnimationEnd((int) (event.getAnimationEnd() * ((-swingSpeed.getInput() / 100) + 1)));
+ event.setAnimationEnd((int)(event.getAnimationEnd() * ((-swingSpeed.getInput() / 100f) + 1f)));Similarly, adjust the calculation for swingSpeedWhileBlocking.getInput().
401-425:
Parameters in rotate() method are not utilized
In the rotate() method, the parameters rotatex, rotatey, and rotatez are not used. Instead, the method accesses the settings directly, which could lead to unexpected behavior and makes the parameters redundant.
Consider modifying the rotate() method to use the parameters passed:
private void rotate(float rotatex, float rotatey, float rotatez) {
// x rotation
GlStateManager.rotate(
- (float) this.rotatex.getInput(),
+ rotatex,
1,
0,
0
);
// y rotation
GlStateManager.rotate(
- (float) this.rotatey.getInput(),
+ rotatey,
0,
1,
0
);
// z rotation
GlStateManager.rotate(
- (float) this.rotatez.getInput(),
+ rotatez,
0,
0,
1
);
}Ensure that when calling rotate(), you pass the correct values and remove unnecessary casts.
385-391:
Potential double addition of translation values in pretranslate() method
The pretranslate() method adds the parameters x, y, z to the settings pretranslatex.getInput(), etc. This might cause the translation values to be applied twice if the settings are also passed as parameters.
Consider adjusting the pretranslate() method to either use the parameters or the settings, but not both:
private void pretranslate(double x, double y, double z) {
GlStateManager.translate(
- x + this.pretranslatex.getInput(),
- y + this.pretranslatey.getInput(),
- z + this.pretranslatez.getInput()
+ x,
+ y,
+ z
);
}This change ensures that the translation values are applied as intended.
Likely invalid or redundant comment.
Thanks |
Lots of changes to Animations.java
Description
Testing
References
Summary by CodeRabbit
New Features
modifyAnimationssetting for enhanced animation customization.CustomFOVmodule for customizable Field of View settings.Improvements
These updates provide users with greater flexibility and control over animation and FOV settings, significantly improving the overall user experience.