Skip to content
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

Merged

Conversation

mattkimber
Copy link
Contributor

@mattkimber mattkimber commented Jan 9, 2021

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:

  • Create a game using Timberwolf's Trains (or other set with articulated rail vehicles that change sprites in relation to curvature information - while this feels like a vehicle length issue I couldn't reproduce it with sets that only have variable vehicle lengths)
  • Build a train with a long vehicle such as a Class 55 or Mk3 carriage.
  • Position the train on a length of straight track.
  • Pause the game.
  • Reverse the train.
  • Scroll the display or drag a dialogue box over the train.

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:

image

This is caused because AdvanceWagonsBeforeSwap() calls TrainController(), which in turn calls UpdateViewport() with the new direction. However, the correct configuration and location of sprites will not be known until after AdvanceWagonsAfterSwap() 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

  • This fixes a single situation. I don't think there are any others that I've observed from playing games in nightly builds, although the player base for Nightly+Timberwolf's Trains is small.
  • There may be a better or more holistic solution available, this seeks to patch the observed bug without too much effect elsewhere.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 9, 2021
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged needs review: NewGRF Review requested from a NewGRF expert size: trivial This Pull Request is trivial labels Jan 9, 2021
@mattkimber
Copy link
Contributor Author

mattkimber commented Jan 9, 2021

I had a moment of inspiration about the cause for this and found another case where this can happen.

  • Allow a vehicle to make a turn transition offscreen.
  • Pause the game once it is in the new orientation.
  • Scroll to the vehicle.

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.

@mattkimber mattkimber force-pushed the fix-sprite-error-on-vehicle-reverse branch from e492655 to 8c86865 Compare January 9, 2021 13:47
@mattkimber
Copy link
Contributor Author

I've pushed a small update to fix the paused case, by always resolving GetSprite() if the game is paused. I think there is one other case where flicker could occur, which is opening a brand new viewport on a vehicle after a direction change, where it may appear incompletely for a single game tick if the game is not paused. (This will be an interesting one to test... some screen capture may be necessary)

@mattkimber
Copy link
Contributor Author

Other cases tested:

  • New viewports: these appear to be fine. I can't find any situation where the new viewport opens with a partial vehicle sprite.
  • Whole map screenshot: this is affected. (Good news is this makes a relatively easy test case)

image

I think the solution is rather than setting is_viewport_candidate and relying on the next vehicle tick to update the sprite, this should be done before the viewport is rendered.

@mattkimber mattkimber force-pushed the fix-sprite-error-on-vehicle-reverse branch 3 times, most recently from a8aec42 to 7659e46 Compare January 9, 2021 15:56
@mattkimber mattkimber force-pushed the fix-sprite-error-on-vehicle-reverse branch from 7659e46 to 3d7ab09 Compare January 9, 2021 15:57
@mattkimber
Copy link
Contributor Author

Fix applied. I had to split up the UpdateViewport() method into "update viewport" and "update bounding box" components, as updating the viewport hash while iterating it causes problems with sprites not being correctly displayed (single frame flicker). The changes between bounding boxes of sprites within a single direction should be small enough this does not matter - indeed for most sets, it will be zero.

A useful thing here has been discovering a number of useful corner cases to check when changing sprite code:

  • Manipulate vehicle while paused (reverse, etc.)
  • Scroll screen while paused
  • Combine manipulating vehicles and scrolling screen while paused
  • Take a screenshot which includes currently off-screen areas
  • Observe long vehicle chains transitioning through curves/hills

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?

@James103
Copy link
Contributor

James103 commented Jan 9, 2021

It also gives a 10% improvement in performance over my original version, as fewer sprites that will not be drawn are resolved.

Does it also result in a performance improvement compared to current master (2020-01-08 or -09)?

@mattkimber
Copy link
Contributor Author

It also gives a 10% improvement in performance over my original version, as fewer sprites that will not be drawn are resolved.

Does it also result in a performance improvement compared to current master (2020-01-08 or -09)?

Yes, although the difference is small. Running the same save game over the same time window at the same resolution gives the following:

master:

image

this branch:

image

@mattkimber
Copy link
Contributor Author

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.

src/vehicle.cpp Outdated Show resolved Hide resolved
@mattkimber mattkimber force-pushed the fix-sprite-error-on-vehicle-reverse branch from 481121b to 67029d7 Compare January 9, 2021 22:36
@mattkimber mattkimber force-pushed the fix-sprite-error-on-vehicle-reverse branch from 67029d7 to 1bb31f9 Compare January 10, 2021 00:21
@TrueBrain TrueBrain added size: large This Pull Request is large in size; review will take a while and removed size: trivial This Pull Request is trivial labels Jan 11, 2021
src/vehicle.cpp Outdated
Comment on lines 1172 to 1173
Vehicle* v_mutable = const_cast<Vehicle*>(v);
v_mutable->Vehicle::UpdateBoundingBoxCoordinates(false);
Copy link
Member

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 :)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

@LordAro LordAro left a 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 :)

src/vehicle.cpp Outdated Show resolved Hide resolved
src/vehicle.cpp Outdated Show resolved Hide resolved
src/vehicle.cpp Outdated Show resolved Hide resolved
@mattkimber mattkimber force-pushed the fix-sprite-error-on-vehicle-reverse branch 4 times, most recently from b71250d to 7b575a5 Compare January 13, 2021 19:41
@mattkimber
Copy link
Contributor Author

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.

src/vehicle.cpp Outdated Show resolved Hide resolved
@mattkimber mattkimber force-pushed the fix-sprite-error-on-vehicle-reverse branch from 7b575a5 to 5e3eb73 Compare January 13, 2021 20:59
@mattkimber mattkimber force-pushed the fix-sprite-error-on-vehicle-reverse branch from 5e3eb73 to 29ec751 Compare January 14, 2021 01:46
@mattkimber
Copy link
Contributor Author

mattkimber commented Jan 14, 2021

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
@mattkimber
Copy link
Contributor Author

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:

  • When the vehicle direction changes, which is known to change bounding boxes even for default vehicles.
  • When the vehicle is on a viewport, as the bounding box may change by enough to require recalculating the dirty blocks.
  • Before a vehicle is drawn, as the player may have moved the viewport while the game is paused and the other two cases require a game tick to happen. In this case it is only necessary to update the sprite, since the newly uncovered viewport region will be considered dirty by the game.

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 GetImage() calls are made (just earlier in the chain for most cases) there's no noticeable performance penalty for this version of the fix.

@michicc michicc merged commit 40d5fe1 into OpenTTD:master Jan 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged needs review: NewGRF Review requested from a NewGRF expert size: large This Pull Request is large in size; review will take a while
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants