Skip to content

Spin train wheels the way the train moves, at its speed - #38

Merged
ryanbarlow97 merged 1 commit into
mainfrom
fix/train-wheel-spin
Sep 27, 2026
Merged

ryanbarlow97 merged 1 commit into
mainfrom
fix/train-wheel-spin

Conversation

@ryanbarlow97

@ryanbarlow97 ryanbarlow97 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • Direction: splineTick picked the forward or backward animation from AccessPanel.isReverse(), which follows the throttle's sign. When you slow down with S, the throttle goes below zero while the train is still rolling forward, so the wheels spin backwards until it stops. Coasting backwards at zero throttle does the opposite.
  • Speed: the move animations always played at their authored rate, one wheel turn a second, whatever the train's speed.

Change

  • The direction now comes from the sign of the train's speed.
  • There's a new optional behaviour.train.wheel-diameter, in blocks. With it set, TrainHandler.animateMove(dir, speed) sets each car's move animation speed to |speed| × 20 / (π × diameter) through ModelEngine's IAnimationProperty.setSpeed, so the wheels roll at the train's speed. This assumes the forward and backward animations turn the wheels once per second, as the TFMC train models do. Cars without the setting keep the authored speed.
  • The bundled trains set it: simple_locomotive 1.875, coal_car 1.0 and passenger_car 1.0, taken from the models.
  • AnimationHandler.setSpeed and ActiveVehicle.setAnimationSpeed are new.

A companion server-assets PR moves the carriage wheels onto the rail gauge, reverses the carriages' wheel spin to match the locomotive, and adds wheel-diameter to the live configs.

Tests

  • New TrainWheelSpinTest: the speed formula, per-car speeds on forward and backward moves, cars without a diameter keeping the authored speed, and stopping leaving the speed alone.
  • TrainReversePlacementTest.wheelsFollowTheWayTheTrainMovesNotTheReverseSwitch: all four combinations of speed sign and reverse switch.
  • mvn verify: 540 tests pass.
  • Lab: tested in the CachyOS lab on a copy of the Main line. A consist was driven with its engine at throttle 30, then braked at −40. I checked in ModelEngine's code that SimpleProperty.updateTime scales time by the speed.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Train wheel animations now scale with train speed and configured wheel diameter, with separate forward and backward rates.
    • Animation speed can be updated for animations that are currently playing.
  • Bug Fixes

    • Wheel animation direction now follows the train’s actual movement, regardless of the reverse switch setting.

- Pick the forward or backward animation from the train's speed, not the
  reverse switch. Braking with the throttle below zero still rolls forward,
  and coasting at zero throttle can roll backwards.
- New behaviour.train.wheel-diameter (blocks). With it, each car's move
  animation plays at the train's speed, so the wheels roll instead of turning
  once a second at every speed. The bundled trains set it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 3c51be64-42c9-42dc-92b2-5633f5da8449

📥 Commits

Reviewing files that changed from the base of the PR and between d1aea02 and dd6f951.

📒 Files selected for processing (8)
  • src/main/java/net/tfminecraft/vehicleframework/vehicles/ActiveVehicle.java
  • src/main/java/net/tfminecraft/vehicleframework/vehicles/handlers/TrainHandler.java
  • src/main/java/net/tfminecraft/vehicleframework/vehicles/handlers/state/AnimationHandler.java
  • src/main/resources/vehicles/coal_car.yml
  • src/main/resources/vehicles/passenger_car.yml
  • src/main/resources/vehicles/simple_locomotive.yml
  • src/test/java/net/tfminecraft/vehicleframework/vehicles/handlers/TrainReversePlacementTest.java
  • src/test/java/net/tfminecraft/vehicleframework/vehicles/handlers/TrainWheelSpinTest.java

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Train movement now sets wheel animation direction and speed from actual movement speed. The speed calculation uses each vehicle’s configured wheel diameter. AnimationHandler applies speed updates to active animation properties.

Changes

Train Wheel Animation

Layer / File(s) Summary
Animation speed updates
src/main/java/net/tfminecraft/vehicleframework/vehicles/ActiveVehicle.java, src/main/java/net/tfminecraft/vehicleframework/vehicles/handlers/state/AnimationHandler.java
ActiveVehicle exposes animation speed updates. AnimationHandler applies the supplied speed to active properties for the requested animation.
Train movement and wheel speed
src/main/java/net/tfminecraft/vehicleframework/vehicles/handlers/TrainHandler.java, src/main/resources/vehicles/*_car.yml, src/main/resources/vehicles/simple_locomotive.yml, src/test/java/net/tfminecraft/vehicleframework/vehicles/handlers/Train*Test.java
TrainHandler selects animation direction from actual speed and calculates wheel speed from wheel diameter. Vehicle configurations set wheel diameters. Tests cover direction, wheel speed, zero diameter, and stopping.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant TrainHandler
  participant ActiveVehicle
  participant AnimationHandler
  participant IAnimationProperty
  TrainHandler->>TrainHandler: splineTick selects direction from actual speed
  TrainHandler->>ActiveVehicle: setAnimationSpeed(animation, wheel turns per second)
  ActiveVehicle->>AnimationHandler: setSpeed(animation, speed)
  AnimationHandler->>IAnimationProperty: set speed on active animation properties
Loading

Merge Risk: ⚪ Minimal · up to dd6f9

No actionable issue was established that would prevent merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to dd6f9

The new behavior appears limited to wheel animation, with no identified change to train authority or access controls. Risk remains low rather than minimal because live configurations and some model-lifecycle behavior are outside the verified scope.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The observed effect is confined to the current train and attached cars' animation state; no additional service, data-store, or privilege boundary was identified in this path.

Trust Boundaries and Controls

  • observed — A positive configured diameter and nonzero movement speed are required before TrainHandler overrides an animation rate; the setter skips unavailable animation properties.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 5 files. (3 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: train wheel animations now follow train movement and speed. It is concise and specific.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 13.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 5 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

A rabbit watched the wheels go round,
As speeds were set and turns were found.
Forward, backward, swift or slow,
The active animations now know.
I twitch my nose and hop along,
While wheels keep time with every song.

Comment @coderabbitai help to get the list of available commands.

@ryanbarlow97
ryanbarlow97 merged commit e35f776 into main Sep 27, 2026
2 checks passed
@ryanbarlow97
ryanbarlow97 deleted the fix/train-wheel-spin branch September 27, 2026 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant