Conversation
WalkthroughThe PR refactors interpolation utilities by moving Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
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/com/paneedah/weaponlib/vehicle/VehicleCustomGUI.java (1)
50-51:⚠️ Potential issue | 🟡 MinorPre-existing bug: key and lock textures appear swapped.
Not introduced by this PR, but worth noting:
keyTexis assignedlockTandlockTexis assignedkeyT. The parameter names strongly suggest these assignments are reversed.- keyTex = lockT; - lockTex = keyT; + keyTex = keyT; + lockTex = lockT;
🧹 Nitpick comments (7)
src/main/java/com/paneedah/weaponlib/EntityBounceable.java (1)
47-51: Instance fields usestatic finalconstant naming convention.
ROTATIONandROTATION_CHANGEare mutable instance fields whose contents change every tick via.add()/.multiply()/ direct field writes. Java convention reservesSCREAMING_CASEforstatic finalconstants — using it here suggests immutability that doesn't exist.Additionally,
MAX_ROTATION_CHANGEdoesn't depend on instance state and can be promoted tostatic final.Suggested rename
- private final Vector3F ROTATION = new Vector3F(); - private final Vector3F ROTATION_CHANGE = new Vector3F(); + private final Vector3F rotation = new Vector3F(); + private final Vector3F rotationChange = new Vector3F(); private float rotationSlowdownFactor = 0.99f; - private final float MAX_ROTATION_CHANGE = 20f; + private static final float MAX_ROTATION_CHANGE = 20f;src/main/java/com/paneedah/mwc/utils/InterpolationUtil.java (2)
8-33: Consider marking the utility class asfinalwith a private constructor.
InterpolationUtilis a stateless utility class with only static methods. Preventing instantiation and subclassing is idiomatic for such classes.Proposed fix
-public class InterpolationUtil { +public final class InterpolationUtil { + + private InterpolationUtil() { + // Utility class + }
11-17:interpolatedEntityPositionhas a hidden dependency onMC.getRenderPartialTicks().Unlike
interpolateValueandinterpolateVector, which acceptpartialTicksas a parameter,interpolatedEntityPositionfetches it internally from the client proxy. This makes the API inconsistent and the method unusable in contexts where a different partial tick value is needed. Consider acceptingpartialTicksas a parameter for consistency and testability.Proposed signature change
- public static Vec3d interpolatedEntityPosition(Entity en) { - return new Vec3d(interpolateValue(en.prevPosX, en.posX, MC.getRenderPartialTicks()), - interpolateValue(en.prevPosY, en.posY, MC.getRenderPartialTicks()), - interpolateValue(en.prevPosZ, en.posZ, MC.getRenderPartialTicks()) + public static Vec3d interpolatedEntityPosition(Entity en, float partialTicks) { + return new Vec3d(interpolateValue(en.prevPosX, en.posX, partialTicks), + interpolateValue(en.prevPosY, en.posY, partialTicks), + interpolateValue(en.prevPosZ, en.posZ, partialTicks) ); - }src/main/java/com/paneedah/weaponlib/vehicle/jimphysics/solver/WheelSolver.java (1)
85-96: Pre-existing:this.solver = solveris a no-op self-assignment.Both constructors assign
this.solver = solver;(lines 91 and 104), butsolveris not a constructor parameter — it refers to the field itself (which isnullat that point). This is dead code. Since this is a cleanup PR, consider removing these lines; the solver is already set viaassignSolver().Proposed cleanup for both constructors
public WheelSolver(TyreSize tyreSize, double mass, boolean isDrive) { this.suspension = new SuspensionSolver(springRate, 1.0); this.tyreSize = tyreSize; - //this.axel = axel; - this.solver = solver; this.wheelMass = mass;Same for the second constructor on lines 98–110.
src/main/java/com/paneedah/weaponlib/vehicle/jimphysics/stability/InertialStabilizer.java (1)
68-68: Pre-existing: DebugSystem.out.printlnleft in production code.Line 68 prints
rotationPitchon every camera update tick. Since this is a cleanup PR, consider removing this debug output.Proposed fix
- System.out.println(rotationPitch);src/main/java/com/paneedah/weaponlib/vehicle/RenderVehicle2.java (1)
88-91: Pre-existing: Profanity in error handler and swallowed exception.Since this is a cleanup PR, consider cleaning up the
catchblock — theSystem.out.println("fuck!")on line 90 is not professional, ande.printStackTrace()should ideally use a proper logger.src/main/java/com/paneedah/weaponlib/vehicle/GearShiftPattern.java (1)
19-22: Naming:PATTERNuses constant-style casing for a mutable instance field.
SCREAMING_CASEin Java conventionally signals astatic finalconstant. SincePATTERNis a non-static, mutableLinkedList(items added viawithBranch), a name likepatternorbrancheswould better communicate its nature.
📝 Description
Continue to work on the
vehiclefolder🎯 Goals
Make the vehicles code better
❌ Non Goals
Make the vehicles achually function better
🚦 Testing
Open game and drive
⏮️ Backwards Compatibility
yes
📚 Related Issues & Documents
N/a
🖼️ Screenshots/Recordings
N/a
📖 Added to documentation?