Spin train wheels the way the train moves, at its speed - #38
Conversation
- 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>
|
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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughTrain 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. ChangesTrain Wheel Animation
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
Merge Risk: ⚪ Minimal · up to No actionable issue was established that would prevent merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to 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 Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
A rabbit watched the wheels go round, Comment |
Problem
splineTickpicked the forward or backward animation fromAccessPanel.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.Change
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'sIAnimationProperty.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.simple_locomotive1.875,coal_car1.0 andpassenger_car1.0, taken from the models.AnimationHandler.setSpeedandActiveVehicle.setAnimationSpeedare 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-diameterto the live configs.Tests
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.SimpleProperty.updateTimescales time by the speed.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes