New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix eeb88e8: Trains reversed while paused do not correctly update sprite bounds #8540
Fix eeb88e8: Trains reversed while paused do not correctly update sprite bounds #8540
Conversation
I had a moment of inspiration about the cause for this and found another case where this can happen.
The reason is that sprite updates because a vehicle is a viewport candidate always run one game tick behind. This is normally not noticeable as a vehicle will be a candidate at least one tick before it becomes visible, and in the cases where it isn't (sharp scrolling, new windows opening) it's hard to spot the missing sprite. Pausing the game causes these ticks not to run, and the incorrect sprite information to become visible. Looking to see if I can find any more cases and if there's a better solution. |
e492655
to
8c86865
Compare
I've pushed a small update to fix the paused case, by always resolving |
Other cases tested:
I think the solution is rather than setting |
a8aec42
to
7659e46
Compare
7659e46
to
3d7ab09
Compare
Fix applied. I had to split up the A useful thing here has been discovering a number of useful corner cases to check when changing sprite code:
This fix handles all cases I've discovered while testing. It also gives a 10% improvement in performance over my original version, as fewer sprites that will not be drawn are resolved. I'd like some pointers on code quality as I ended up having to go back to the mutable vehicle cast in order to update the coordinates. Does it make sense for these to become part of the mutable sprite cache, or is there some other approach we could take? |
Does it also result in a performance improvement compared to current |
Yes, although the difference is small. Running the same save game over the same time window at the same resolution gives the following: master: this branch: |
One last problem case fixed - with this updated version, there can be artifacts if the viewport bounding box update makes a change which should cause viewport hash invalidation. My initial solution caused the check for whether a sprite has entered a new viewport hash to incorrectly identify the sprite as already in the correct hash, as it caused the "old" coordinates to update. This change moves "old" coordinates to the mutable sprite cache and only updates them when the viewport hash is also being recalculated. All of the existing difficult case scenarios still work, so I think this is now at the point it needs code quality review and any improvements thereof. |
481121b
to
67029d7
Compare
67029d7
to
1bb31f9
Compare
src/vehicle.cpp
Outdated
Vehicle* v_mutable = const_cast<Vehicle*>(v); | ||
v_mutable->Vehicle::UpdateBoundingBoxCoordinates(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit irritating that the const_cast has had to be reintroduced here. Removing it was the whole point of the MutableSpriteCache structure :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a short play around with this, see if there's a fix which doesn't result in wholesale code changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest change moves this chain to const, with vehicle coords becoming mutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only mild pedantry. Ultimately it's definitely an improvement over what's there already. It's either this or revert the entire thing :)
b71250d
to
7b575a5
Compare
I've run this through a bunch of testing and all issues noted with "normal" game sprites and typical NewGRF files are resolved to my knowledge. Multiplayer games appear happy with no desyncs noticed even on large games with many vehicles. There is one case still outstanding, which may be worth future attention - if a vehicle changes sprite without an accompanying change in direction to one that is larger than the current dirty rectangle, it will be clipped to the dirty rectangle until the following tick. This is unfortunately by design of the optimisation, by the time we find out the new sprite is bigger we have already established the rectangle it should be drawn in. As you say, I think this is a big improvement on what went before and I'm unaware of any NewGRF files in circulation which alter bounding boxes by large amounts within a single direction. (The ones which do such as CETS and Timberwolf's make small changes well within an existing dirty rectangle). It would be a nice thing to fix but I'm not sure if it's possible to do so without increasing the memory requirements per vehicle or reducing the amount of performance improvement. |
7b575a5
to
5e3eb73
Compare
…er large bounding box changes
5e3eb73
to
29ec751
Compare
It didn't sit well having a known case which didn't work, and while it's only noticeable with extremes I noticed a few situations where vehicles in Timberwolf's Trains (which has quite a large difference in sprite sizes) were coming close to getting 1 or 2 pixel errors at their corners in situations where the --- sprite changed direction to a \ or / track in some places. This latest update fixes even the extreme case of a vehicle changing size by >100 pixels without a change in direction, while not changing existing bounding box behaviour for "well-behaved" sets. Sets which allow large discrepancies between sprite sizes will end up with slightly oversize dirty rectangles but I think this is an acceptable tradeoff to allow these to work correctly, and as the difference is only ~0.1-0.2ms even on a busy viewport at high resolution, it is more than offset by the improvement in train tick time. There is one remaining situation where a vehicle can clip for a single tick, which is if the sprite expands by more than ~32 pixels in X or ~16 pixels in Y in a single step. The very first time this happens it will be unable to expand fully. This is very unlikely to be encountered in a real game situation. (Although if there's a way to work out how much the vehicle's sprites can grow without waiting for it to happen on a viewport, I'd be interested to try that) |
… correct bounding box when drawn on viewport
And now that very last case is handled. I realised the problem could be solved if we knew what was going to be a viewport candidate before we start drawing it and the dirty rectangle can no longer be extended upward from its original dimensions. This gives three places we need to resolve sprites:
Sorry this became rather an odyssey - what looked like a "simple" change turned up a lot of interesting information about how sprite resolution and invalidation works, and I have a bad habit of going to sleep one night then waking up the next morning with an, "ah, but what if a NewGRF author/player did this?" which inevitably uncovers new information. This change reverts the workaround of creating oversize bounding boxes, and the need for vehicle variables to track them. As the same number of |
Motivation / Problem
I found a bug in the code for resolving vehicle sprites only on direction change. Initially this was a flicker briefly observed when vehicles reversed at end of line, but I found a reproducible case:
All conditions need to be true (game paused, train reversed, display region dirty). It is possible to notice single-frame sprite flickering in some circumstances when a vehicle reverses, but not as consistently reproducible.
The result is this:
This is caused because
AdvanceWagonsBeforeSwap()
callsTrainController()
, which in turn callsUpdateViewport()
with the new direction. However, the correct configuration and location of sprites will not be known until afterAdvanceWagonsAfterSwap()
has been called, at which point the incorrect sprite information has already been cached.Description
My proposed fix is to make
ReverseTrainDirection()
invalidate the sprite cache before it requests a viewport update. This means the sprite is correctly set again once all of the vehicles are in their final direction, position and relative orientation.An alternative would be to force updating the viewport information in
DoDrawVehicle()
but that felt less clean as it would force updating bounds even for vehicles where this is not necessary.There are additional
GetImage()
calls introduced by this change. It is expected the number of vehicles reversing is negligible compared to the number that are transitioning through turn states in a typical game.Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.