Repository navigation
Some big rendering changes - #628
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:
WalkthroughReplaces NewScreenshakingManager with a new ScreenshakingManager and updates Interceptors and call sites; adds runtime GL init and GLImplementation enum; registers two data serializers via an event subscriber; moves and renames several utilities; removes multiple legacy rendering helpers; applies Lombok for generated accessors. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (render loop)
participant Screenshake as Interceptors.SCREENSHAKING_MANAGER
participant Spring as SpringVector
participant Weapon as MainHeldWeapon / ScreenShakeParams
Renderer->>Screenshake: update(partialTicks)
Screenshake->>Spring: advance fixed-timestep (dt) using accumulator
Spring-->>Screenshake: previous & current state
Weapon->>Screenshake: (on fire) impulse(mag)
Screenshake->>Spring: apply impulse
Renderer->>Screenshake: applyWorld()
Screenshake->>Weapon: read screenShake parameters (if present)
Screenshake->>Renderer: GlStateManager.rotate(...) using interpolated shake
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/main/java/com/paneedah/weaponlib/render/shells/ShellParticleSimulator.java (1)
87-89:⚠️ Potential issue | 🟡 MinorHand-written
shouldDie()coexists with Lombok-generatedisShouldDie()— ambiguous API.The
public boolean shouldDiefield (line 38) causes Lombok to generateisShouldDie()(the standardisprefix for primitive booleans). Meanwhile, the hand-writtenshouldDie()method here does the same thing. Callers now have two ways to check this —shell.shouldDie()andshell.isShouldDie()— which is confusing.Either remove the hand-written method and update
Shell::shouldDieon line 104 toShell::isShouldDie, or annotate theshouldDiefield with@Getter(AccessLevel.NONE)to suppress Lombok's generation for it.Option A: Remove hand-written method, use Lombok's accessor
- public boolean shouldDie() { - return this.shouldDie; - }And on line 104:
- shells.removeIf(Shell::shouldDie); + shells.removeIf(Shell::isShouldDie);Option B: Suppress Lombok getter for `shouldDie`
+ `@Getter`(AccessLevel.NONE) public boolean shouldDie = false;src/main/java/com/paneedah/weaponlib/render/bgl/math/AngleKit.java (2)
420-424:⚠️ Potential issue | 🟠 MajorPre-existing bug:
zis divided by2instead ofs.Lines 421–422 normalize
xandybys, but line 423 divideszby the literal2. This is almost certainly a typo — the standard quaternion-to-axis-angle conversion requires all three axis components to be divided by the same sine factor. This will produce incorrect axis directions for any rotation with a non-trivial z-component.Since you're already touching this file, consider fixing it now.
🐛 Proposed fix
} else { x = q0.x / s; y = q0.y / s; - z = q0.z / 2; + z = q0.z / s; }
378-390:⚠️ Potential issue | 🟠 MajorPre-existing bug: default axis
(1,0,0)is then divided by near-zerolength.When
length < 0.001, the code setsx = 1, y = 0, z = 0(a sensible default for a near-zero rotation), butlengthitself is never updated. The subsequentx /= lengthon line 388 divides by the original tiny value, producing an astronomically largex.🐛 Proposed fix — set `length = 1` so the normalization pass is a no-op
if (length < 0.001) { x = 1; y = 0; z = 0; + length = 1; } else { length = (float) Math.sqrt(length); }src/main/java/com/paneedah/weaponlib/numerical/SpringValue.java (1)
108-111:⚠️ Potential issue | 🟡 Minor
muparameter is ignored —MC.getRenderPartialTicks()is always used instead.
getLerpedPosition(double mu)accepts amuinterpolation factor but never uses it, always substitutingMC.getRenderPartialTicks(). This makes the overload misleading and functionally identical to the zero-arg version.Proposed fix
public double getLerpedPosition(double mu) { return MatrixHelper.solveLerp((float) this.prevPosition, (float) this.position, - MC.getRenderPartialTicks()); + (float) mu); }
🤖 Fix all issues with AI agents
In `@src/main/java/com/paneedah/mwc/init/MWCDataSerializers.java`:
- Around line 10-11: The class annotation for MWCDataSerializers is missing the
mod id; update the `@Mod.EventBusSubscriber` annotation on the MWCDataSerializers
class to include modid = ID so it matches other subscribers (MWCRecipes,
MWCItems, MWCBlocks), ensuring the subscriber is registered under the same mod
identifier.
In `@src/main/java/com/paneedah/weaponlib/numerical/SpringValue.java`:
- Line 18: In SpringValue (class SpringValue) the method
getLerpedPosition(double mu) ignores its mu parameter and instead calls
MC.getRenderPartialTicks(); change the implementation to use the passed-in mu
value wherever MC.getRenderPartialTicks() is currently used so the method
actually lerps by the provided mu, and keep the signature as
getLerpedPosition(double mu); if there are call sites that relied on the old
behavior, ensure they pass the desired partial tick value into getLerpedPosition
instead of relying on MC.getRenderPartialTicks().
In
`@src/main/java/com/paneedah/weaponlib/render/bgl/instancing/ModelInstancedObject.java`:
- Around line 22-24: The error message is misleading and execution continues
into invalid VAO usage: in ModelInstancedObject, when model.usesVAO() is false,
change the LOGGER.error text to clearly state the model does not provide a VAO
(e.g., "Model does not provide a VAO") and prevent further VAO access by either
throwing an exception (IllegalStateException) or returning early before calling
model.getVAOID(); update the check around model.usesVAO(), LOGGER.error(...) and
the subsequent model.getVAOID() usage accordingly to ensure no VAO methods are
invoked when usesVAO() is false.
In `@src/main/java/com/paneedah/weaponlib/render/ScreenshakingManager.java`:
- Around line 46-66: The update method currently interpolates using partialTicks
which is wrong; change interpolation to use the leftover physics fraction
(accumulator / dt) and capture prev as the position immediately before the final
springVector.update step. Specifically, in update(float partialTicks) use
accumulator/dt as the lerp factor when calling MatrixHelper.lerpVectors(prev,
springVector.getPosition(), ...), and move the assignment of Vec3d prev =
springVector.getPosition() so it is set just before the last
springVector.update(dt) call (or update prev inside the loop to the previous
state) to ensure prev represents the position before the final step; keep using
springVector, accumulator, dt, interpolated and lastUpdateTime accordingly.
🧹 Nitpick comments (7)
src/main/java/com/paneedah/weaponlib/render/shells/ShellRegistry.java (1)
67-69: Consider returning an unmodifiable view of the internal map.
getShellMappings()exposes the mutable backing map, allowing callers to modify registry state. Wrapping withCollections.unmodifiableMap(shellRegistrar)would prevent accidental mutation.♻️ Suggested change
public static HashMap<Shell.Type, Pair<WavefrontModel, ResourceLocation>> getShellMappings() { - return shellRegistrar; + return java.util.Collections.unmodifiableMap(shellRegistrar); }Note: the return type would need to widen to
Map<...>accordingly.src/main/java/com/paneedah/weaponlib/numerical/CyclicList.java (1)
20-30: Pre-existing:current()on an empty list will throwIndexOutOfBoundsException.Not introduced by this PR, but worth noting since the class is being moved and given fresh documentation: both
current()andnext()will throw if the list is empty. Callers (e.g.,ReflexScreen.renderReticleat line 136,ItemScope.Builderat line 111) guard withisEmpty()or null-checks inconsistently —ReflexScreen.renderat line 201 callsreticleList.current()without any guard.Consider adding a defensive check or documenting the precondition in the new Javadoc.
src/main/java/com/paneedah/weaponlib/render/shells/ShellParticleSimulator.java (1)
25-47: Public fields with@Getter@Setter`` creates an inconsistent access pattern.The class has a mix of
privatefields (sleeping,type,height) andpublicfields (age,onGround,shouldDie,pos,prevPos,velocity,prevRot,rot). Applying@Getter@Setter`` at the class level generates accessors for all fields, but the public ones are still accessed directly throughoutupdate()(e.g., `sh.pos.x`, `sh.age`). This leads to two coexisting access patterns for the same data.Consider making all fields
privateand using the Lombok-generated accessors consistently, or scope@Getter@Setter`` to only the fields that need it.src/main/java/com/paneedah/weaponlib/render/gui/ModificationGUI.java (1)
367-375: Documentation improvement looks good!The updated Javadoc is more specific and accurate. The term "TexturedRect bounds" is clearer than "GUI boundings".
One minor suggestion: For boolean return types, the standard Javadoc pattern is typically
@return true if the mouse is within the TexturedRect, false otherwiseor simply@return whether the mouse is within the TexturedRect. The current wording is acceptable but slightly less conventional.📝 Optional Javadoc refinement
- * `@return` true or false if the mouse is within the TexturedRect + * `@return` true if the mouse is within the TexturedRect, false otherwisesrc/main/java/com/paneedah/weaponlib/render/ScreenshakingManager.java (1)
15-24: Consider making internal state fieldsprivate.
springVector,interpolated,dt,lastUpdateTime, andaccumulatorare allpublic, allowing any external code to mutate the simulation state arbitrarily (e.g., resettinglastUpdateTimeoraccumulator). Onlyimpulse(),update(), andapplyWorld()should be public API; the rest is internal state. The Javadoc suggests this is a focused manager class — encapsulation would protect invariants.src/main/java/com/paneedah/weaponlib/render/cam/NaturalCamera.java (1)
44-48: The comment documents existing dead code — consider removing the dead method body instead.The
if (true) { return; }block makes lines 49–147 unreachable. Rather than documenting this with a comment, consider either removing the dead code entirely or extracting it behind a feature flag if it's intended for future use. Leaving ~100 lines of unreachable code increases maintenance burden.src/main/java/com/paneedah/weaponlib/ClientEventHandler.java (1)
248-250: The reconcile condition is still quite dense — consider extracting to a helper method.This single-line condition has 4 boolean conjuncts with timestamp arithmetic and mixed field access patterns (
syncStartTimestampis accessed directly while other fields use getters). The static analysis also flags this as a complex conditional. Extracting to ashouldReconcile(PlayerWeaponInstance)helper would improve readability.That said, the simplification from the prior version is an improvement.
| */ | ||
| @Getter @Setter | ||
| public class SpringValue { | ||
| public class SpringValue implements ISimulator { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'ISimulator.java' --type fRepository: Cubed-Development/Modern-Warfare-Cubed
Length of output: 144
🏁 Script executed:
cat -n src/main/java/com/paneedah/weaponlib/numerical/ISimulator.javaRepository: Cubed-Development/Modern-Warfare-Cubed
Length of output: 243
🏁 Script executed:
cat -n src/main/java/com/paneedah/weaponlib/numerical/SpringValue.javaRepository: Cubed-Development/Modern-Warfare-Cubed
Length of output: 4140
Fix unused parameter in getLerpedPosition(double mu).
The method at lines 108-111 accepts a mu parameter but never uses it, instead calling MC.getRenderPartialTicks(). Either use the parameter or remove it from the signature.
🤖 Prompt for AI Agents
In `@src/main/java/com/paneedah/weaponlib/numerical/SpringValue.java` at line 18,
In SpringValue (class SpringValue) the method getLerpedPosition(double mu)
ignores its mu parameter and instead calls MC.getRenderPartialTicks(); change
the implementation to use the passed-in mu value wherever
MC.getRenderPartialTicks() is currently used so the method actually lerps by the
provided mu, and keep the signature as getLerpedPosition(double mu); if there
are call sites that relied on the old behavior, ensure they pass the desired
partial tick value into getLerpedPosition instead of relying on
MC.getRenderPartialTicks().
| public void update(float partialTicks) { | ||
| // Calculate frame time | ||
| long now = System.currentTimeMillis(); | ||
| double frameTime = (now - lastUpdateTime) / 1000.0; | ||
| lastUpdateTime = now; | ||
|
|
||
| // Clamp to avoid huge jumps | ||
| if (frameTime > 0.25) frameTime = 0.25; | ||
| accumulator += frameTime; | ||
|
|
||
| Vec3d prev = springVector.getPosition(); | ||
|
|
||
| // Step the spring fixed timestep until we catch up | ||
| while (accumulator >= dt) { | ||
| springVector.update(dt); | ||
| accumulator -= dt; | ||
| } | ||
|
|
||
| // Interpolate within this frame using partialTicks | ||
| interpolated = MatrixHelper.lerpVectors(prev, springVector.getPosition(), partialTicks); | ||
| } |
There was a problem hiding this comment.
Incorrect interpolation factor: partialTicks vs accumulator / dt.
The fixed-timestep accumulator pattern requires interpolating by the leftover fraction (accumulator / dt), not by Minecraft's partialTicks. These represent fundamentally different things:
partialTicks: fraction of the current Minecraft tick (~50ms) that has elapsedaccumulator / dt: fraction of one physics step (~8.3ms) remaining after the loop
Using partialTicks here will produce jittery or visually incorrect interpolation because it doesn't correlate with the physics step boundary.
Proposed fix
public void update(float partialTicks) {
// Calculate frame time
long now = System.currentTimeMillis();
double frameTime = (now - lastUpdateTime) / 1000.0;
lastUpdateTime = now;
// Clamp to avoid huge jumps
if (frameTime > 0.25) frameTime = 0.25;
accumulator += frameTime;
Vec3d prev = springVector.getPosition();
// Step the spring fixed timestep until we catch up
while (accumulator >= dt) {
+ prev = springVector.getPosition();
springVector.update(dt);
accumulator -= dt;
}
- // Interpolate within this frame using partialTicks
- interpolated = MatrixHelper.lerpVectors(prev, springVector.getPosition(), partialTicks);
+ // Interpolate using the leftover accumulator fraction
+ double alpha = accumulator / dt;
+ interpolated = MatrixHelper.lerpVectors(prev, springVector.getPosition(), (float) alpha);
}Note: prev must also be captured as the position before the last step (not before all steps), so it should be updated inside the loop.
🤖 Prompt for AI Agents
In `@src/main/java/com/paneedah/weaponlib/render/ScreenshakingManager.java` around
lines 46 - 66, The update method currently interpolates using partialTicks which
is wrong; change interpolation to use the leftover physics fraction (accumulator
/ dt) and capture prev as the position immediately before the final
springVector.update step. Specifically, in update(float partialTicks) use
accumulator/dt as the lerp factor when calling MatrixHelper.lerpVectors(prev,
springVector.getPosition(), ...), and move the assignment of Vec3d prev =
springVector.getPosition() so it is set just before the last
springVector.update(dt) call (or update prev inside the loop to the previous
state) to ensure prev represents the position before the final step; keep using
springVector, accumulator, dt, interpolated and lastUpdateTime accordingly.
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/render/bgl/GLCompatible.java (1)
265-273:⚠️ Potential issue | 🟠 MajorLine 268: Replace
GL_RGB16F_ARBwithGL_RGBA16F_ARB.This assigns a 3-channel format to a field intended for 4-channel RGBA. The correct constant
ARBTextureFloat.GL_RGBA16F_ARBis available in LWJGL 2 (unlike the pattern in other branches at lines 264, 266, 270, which all use 4-channel formats). Consumers expect an alpha channel based on theGL_RGBAformat flag passed toglTexImage2D, so using the RGB variant will cause silent rendering errors on systems using the ARB extension.
🧹 Nitpick comments (1)
src/main/java/com/paneedah/weaponlib/render/bgl/GLCompatible.java (1)
213-216: Mac instancing override may clobber a previously detectedattribDivisorType.On macOS,
instancingTypeis forced toAPPLE(which hassupportsInstancing=false), effectively disabling instancing. However,attribDivisorType— set a few lines above — is left at whatever was detected (potentiallyNORMALorARB). If instancing is disabled, having a validattribDivisorTypeis harmless but semantically inconsistent. Consider whetherattribDivisorTypeshould also be overridden on Mac for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/com/paneedah/weaponlib/render/cam/NaturalCamera.java`:
- Around line 24-28: matrixStack is being pushed to unconditionally in
addToMatrixStack which causes unbounded growth when enableNaturalCamera is false
and can still grow if addToMatrixStack is called more than twice between
update() calls; modify addToMatrixStack to only push when the feature is active
(check enableNaturalCamera or equivalent flag) and/or enforce a bounded policy
(e.g., drop oldest entries or cap size) so the stack cannot grow indefinitely,
and adjust update() to consume all pending matrices or a sensible capped number
rather than only two to prevent backlog; reference matrixStack,
addToMatrixStack, update(), and enableNaturalCamera when making the change.
🧹 Nitpick comments (4)
src/main/java/com/paneedah/mwc/asm/Interceptors.java (1)
563-563: Consider relocating this field declaration near the other static fields at the top of the class (lines 45–55).The field itself is fine —
public static finalwith eager initialization is appropriate for a stateless or thread-safe manager singleton. Moving it next tonc,thirdPersonCameraStabilizer, etc. would improve readability.src/main/java/com/paneedah/weaponlib/render/cam/NaturalCamera.java (3)
35-54: Misleading variable names:previousandcurrentappear swapped.
LinkedList.pushadds to the head, sopop()returns the most recently pushed element first. That means line 37 assigns the newest matrix topreviousand line 38 assigns the older matrix tocurrent— the opposite of what the names suggest.The math may still produce a usable result (the sign inversion is absorbed by the negative
scale), but this is confusing and fragile for future maintainers. Consider swapping the names or adding a clarifying comment.Also,
thres = 0.0on line 45 makes all three threshold checks (Math.abs(...) > 0.0) effectively no-ops — they pass for any non-identical float values. Either use a meaningful epsilon or remove the dead conditionals.Proposed clarification
- Matrix4f previous = matrixStack.pop(); - Matrix4f current = matrixStack.pop(); + Matrix4f current = matrixStack.pop(); // most recent (head) + Matrix4f previous = matrixStack.pop(); // older double forceX = 0; double forceY = 0; double forceZ = 0; - double scale = -0.04; - double thres = 0.0; - if (Math.abs(current.m30 - previous.m30) > thres) { - forceX = (current.m30 - previous.m30) * scale; - } - if (Math.abs(current.m31 - previous.m31) > thres) { - forceY = (current.m31 - previous.m31) * scale; - } - if (Math.abs(current.m32 - previous.m32) > thres) { - forceZ = (current.m32 - previous.m32) * scale; - } + double scale = 0.04; + forceX = (current.m30 - previous.m30) * scale; + forceY = (current.m31 - previous.m31) * scale; + forceZ = (current.m32 - previous.m32) * scale;
74-80:xrgrows without bound, risking floating-point precision loss over long sessions.
xris incremented by0.01every update and is never wrapped or reset. It's used as a phase parameter (xr * 2) in the Lissajous curve. After extended play sessions (hours), the value becomes large enough that adding0.01loses precision due to IEEE 754 representation, causing the animation to stutter or freeze.Consider wrapping it modulo
2π(orMath.PI * 2) since trigonometric functions are periodic:xr += 0.01; + xr %= (Math.PI * 2);
59-61: Spring physics uses a fixed timestep, causing frame-rate-dependent behavior.
shockVector.update(0.05)is called once per frame with a constant delta. At higher frame rates the spring simulation advances more real-time per second than at lower frame rates, resulting in noticeably different camera sway intensity. Consider scaling the timestep by actual frame delta, or accumulating a fixed-step loop using the real elapsed time.Additionally,
shockVector.configure(500, 800, 50)is called every frame — if those are constant parameters, consider moving the configuration to initialization.
2 participants
📝 Description
Larger changes to the
weaponlib/renderfolderNotable Changes:
NewScreenshakingManager(now justScreenshakingManager)DataSerializers.registerSerializerwith the correct version that using the eventbusCyclicListwith its other kindimplement ISimulatordo soModelInstancedObjecterror messageShellRegistryboolet9mmto match its modelInstancedRenderclassModelRenderToolclassModernModelclassModernSkyRendererclassMWCFrameTimerclassTriangleclassScreenRenderertoScreenModelRendererbecause thats what it doesGLCompatiblebetter by using enums❌ Non Goals
Completely change lifecycle
🚦 Testing
Launch game, fly around, shoot guns at mobs
⏮️ Backwards Compatibility
yep
📚 Related Issues & Documents
n/a
🖼️ Screenshots/Recordings
n/a
📖 Added to documentation?