A mount changes the sprite and nothing else - #2303
Merged
Merged
Conversation
`cond` carries Speed, and Speed is written once at login from the class table and never again. Mounting sets VehicleSpeed, which no packet reads: the player rides at walking pace, and nothing in a log or a packet says so. SpeedCalculationService exists to answer exactly this question and **has no callers at all**. It is also wrong where it matters: PlayerStateComponent declares VehicleSpeed as a plain `byte` and ICharacterEntity widens it to `byte?`, so `VehicleSpeed != null` can never be false. Wired as written it would have returned 0 for everybody on foot. Now it keys off IsVehicled, and the mount and dismount paths call it. The existing test passed only because of that bug: it set VehicleSpeed without IsVehicled, which is a state a mount never produces, and the impossible null check happened to answer it correctly. It now sets both, and a second case covers being on foot with VehicleSpeed at 0. Still missing, and out of scope here: the movement BCards. CalculateSpeed's bonus is a commented-out `bonusSpeed = 0` from upstream, so a speed buff moves nothing.
|
Warning Review limit reachedNext included review available in 26 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
condcarriesSpeed, andSpeedis written once inCreatePlayerfrom the class table and never again. Mounting setsVehicleSpeed, which no packet reads — so the player rides at walking pace, and nothing in a log or a packet says so.The service for this already exists and nobody calls it
SpeedCalculationService.CalculateSpeed(ICharacterEntity)is written to answer exactly this question.grepfinds no caller anywhere insrc/.It is also wrong where it matters:
PlayerStateComponentdeclaresVehicleSpeedas a plainbyte;ICharacterEntitywidens it tobyte?. The null branch is unreachable, so wired as written the service would have returned0for everyone on foot. Nothing reported it because nothing called it.It now keys off
IsVehicled, and the mount and dismount paths call it.The existing test was green for the wrong reason
VehicleSpeedOverridesDefaultSpeedsetVehicleSpeedand leftIsVehicledat its mock default offalse— a state a mount never produces. The impossible null check happened to answer it correctly. It now sets both, and a second case covers being on foot withVehicleSpeedat 0.The transformation tests assert the number the client actually receives (
GenerateCond().Speed), not justVehicleSpeed: asserting only the latter is what let a mount that adds no speed look correct.Not in this PR
CalculateSpeed's bonus is a commented-outbonusSpeed = 0left from upstream, so the movement BCards still do nothing. That is a separate change.