JGR's Patch Pack

[Eddi]

I don't think anything else is affected with the possible exception of some of the old CETS test releases.

could you test that? it might be that you do something different than me. also, there is a parallel universe where DBSetXL 0.9 exists and uses a similar method of splitting vehicles into 3.

[Timberwolf]

I don't think anything else is affected with the possible exception of some of the old CETS test releases.

could you test that? it might be that you do something different than me. also, there is a parallel universe where DBSetXL 0.9 exists and uses a similar method of splitting vehicles into 3.

Just tested - CETS r822 shows the same behaviour. Does it use a 1x1 transparent sprite when the front/rear sections aren't shown? I think this is only an issue when bounding boxes change in a different tick to the one in which the vehicle changes track direction.

[Eddi]

Does it use a 1x1 transparent sprite when the front/rear sections aren't shown?

yeah

Code: Select all

spriteset(inv_dummy_set, "src/gfx/dummy.png") {
        [ 0, 0, 1, 1, 0, 0]
        [ 0, 0, 1, 1, 0, 0]
        [ 0, 0, 1, 1, 0, 0]
        [ 0, 0, 1, 1, 0, 0]
}

[Snail]

Hmm, I think I'd have to spend some time resolving it myself, it is not as straightforward as I had presumed.

On the other hand, michi_cc's PR might remove the problem instead https://github.com/OpenTTD/OpenTTD/pull/8479, that will also require some resolving from me to integrate.

In the meantime, I've added a one-liner to my branch which should hopefully allow you to make progress.

It works! Thank you very much.

[michael blunck]

As Eddi said. It's the same problem over here.

regards
Michael

[Argus]

Problem with joker patchpack games persist.

[JGR]

I've found a tiny bug in the optimised sprite rendering... mainly because I've been looking at similar things in regular OpenTTD and encountered the same problem myself.

The current code assumes that all sprites for a given direction have the same bounding box. Which is usually valid, but some sets get particularly abusive, for example Timberwolf's Trains which has a tendency to swap between 1x1 "invisible" sprites and larger vehicle sections depending on how it feels like rendering long vehicles at that point. This interacts unfortunately with the optimised renderer, which takes the bounding box for a sprite the first time a vehicle enters a new direction. On Timberwolf's Trains, this is still using one of the intermediate turn sprites and thus the front and rear sections of the vehicle have their 1x1 "invisible" sprites in play. This means JGRPP will then cache this size for determining whether a vehicle is in a viewport, and you get a slightly jumpy effect on straight tracks once Trains has swapped to its alternate scheme of splitting the vehicle into three sprites.

When this strikes, you get this happening at the very edge of the viewport:



I noted you're not using this optimisation for ships/aircraft, where this kind of internal mucking about with sprite geometry is more common. At any rate, it's a tiny thing you have to be looking hard to notice, and the correct response may indeed be "Closed: Timberwolf is a bad person and should not do the unnatural things his train set attempts to inflict upon the world."

(If I figure out a fix which still keeps the massive performance improvement from sprite direction caching I'll let you know. And if it's a massive, massive pain I might look at making my train set use more consistent bounding boxes as I don't think anything else is affected with the possible exception of some of the old CETS test releases. Edit: https://github.com/OpenTTD/OpenTTD/pull/8485 is my variant, which reduces the number of cases in which this happens by considering vehicles to be "viewport candidates" and reverting to checking every tick. There may be a more efficient and reliable approach than this still out there.)

I think that this is a valid use case, just a bit untraditional, "Closed: Timberwolf is a bad person and should not do the unnatural things his train set attempts to inflict upon the world." would seem wrong to me.

I'll do some experimenting a bit later.

Problem with joker patchpack games persist.

The savegame seems fine here, what problem are you seeing?

In general though, I can't do anything about the fundamental NRT incompatibility issues between the current version, and the early versions used in JokerPP, etc.

[Argus]

Vehicles can't find the way, reports invalid stations. However, if there is an error in the collision between the NRT versions, then I probably have no choice but to fix everything manually. I don't want to download the joker patchpack again, which will be obsolete in a while .... Apart from influencing the generation of the desert, I also don't miss anything from it, rather the opposite.

[JGR]

(If I figure out a fix which still keeps the massive performance improvement from sprite direction caching I'll let you know. And if it's a massive, massive pain I might look at making my train set use more consistent bounding boxes as I don't think anything else is affected with the possible exception of some of the old CETS test releases. Edit: https://github.com/OpenTTD/OpenTTD/pull/8485 is my variant, which reduces the number of cases in which this happens by considering vehicles to be "viewport candidates" and reverting to checking every tick. There may be a more efficient and reliable approach than this still out there.)

I've added a fix to my branch which seems to resolve this, and the performance hit is only modest (and more importantly it's basically 0 for normal vehicles).

If curvature variables are accessed when getting the image, cache the directions of the 4 nearest neighbour vehicles and check those along with the vehicle's own direction when deciding whether to refresh the image (and bounds) in UpdateViewport.

[Eddi]

If curvature variables are accessed when getting the image, cache the directions of the 4 nearest neighbour vehicles and check those along with the vehicle's own direction when deciding whether to refresh the image (and bounds) in UpdateViewport.

dunno, before actually looking at your code, this sounds like a giant hack that's going to haunt us in 5 years.

edit: i think it would be much safer and more future-proof to just update the whole vehicle chain

[JGR]

If curvature variables are accessed when getting the image, cache the directions of the 4 nearest neighbour vehicles and check those along with the vehicle's own direction when deciding whether to refresh the image (and bounds) in UpdateViewport.

dunno, before actually looking at your code, this sounds like a giant hack that's going to haunt us in 5 years.

I'm not going to PR it upstream, so you needn't worry about it.

[Eddi]

I'm not going to PR it upstream, so you needn't worry about it.

that doesn't sound very reassuring

[michael blunck]

With regards to this optimisatuon, I still don't get the implications on vehicle bounding boxes. Are they, or should they, (more) dependent on sprite dimensions or on vehicle sizes (prop 21)?

regards
Michael

[JGR]

I'm not going to PR it upstream, so you needn't worry about it.

that doesn't sound very reassuring

These optimisations are invisible to NewGRFs, so there's no risk of anything being broken. Flicker and poor performance are permanent issues which exist in varying amounts with or without these optimisations.
It would be trivial to modify or remove it later if it turns out there's some sort of problem with it.
In the meantime, this is a reasonable point on the trade-off between acceptable run-time performance and the absence of visual artefacts.

With regards to this optimisatuon, I still don't get the implications on vehicle bounding boxes. Are they, or should they, (more) dependent on sprite dimensions or on vehicle sizes (prop 21)?

regards
Michael

Vehicle sizing (prop 21) is just about how long vehicles are in terms of game simulation (movement, vehicle spacing, etc.), it affects the positioning of the centre-point of the sprite but does not otherwise affect drawing.

This optimisation is really about the dimensions of the sprite and whether or not it intersects with the viewport(s).

[michael blunck]

So the problem will go away if we'd supply a properly dimensioned transparent sprite instead of a 1×1 transparent sprite? This would be easy enough.

regards
Michael

[Eddi]

if i'm understanding the problem right, it's not just the transparent sprite, but also the sliced middle sprite (in case of split drawing) must be the same size as the combined sprite

[Timberwolf]

With the original bug (which JGR has now added a fix for), the dimensions for each sprite section need to match from the point each vehicle section enters a new direction. I think all three of us use Eddi's movement scheme, which means for the 3 vehicles that are part of the chain:

* Vehicle 1 enters a new direction in its "invisible" state.
* Vehicle 2 enters a new direction as a long vehicle, with the second of its intermediate turn sprites.
* Vehicle 3 enters a new direction with the same "section" sprite it will keep until Vehicle 1 changes direction.

To be most correct, vehicles 1 and 2 would need to have the same bounding box and offset for invisible/straight and combined/sectioned respectively. In practice I suspect the second vehicle does not matter for this specific instance as the "combined" sprite is larger than the "section" sprite, so will always be correctly identified as being part of the viewport.

There might be another (rarer) instance where it does matter, which is when the vehicle enters a curve right on the edge of the viewport. In this situation only the first vehicle has changed direction, and the second is still using the smaller bounding box of the "section" sprite. There you would see a brief flicker until Vehicle 2 changes direction and also to the second intermediate turn sprite.

Vehicle 3 doesn't matter in either case, as the only situation in which it could use the wrong bounding box is also one where it is invisible and not displaying it doesn't make any difference.

Now to be very annoying in the context of a fix... at least two of these sets have another situation in which the vehicle bounding box can change (and for long enough vehicles, the same swapping in and out of transparent and combined sprites). And it's not triggered by a curvature change either. On slopes, they also swap to different bounding boxes. Timberwolf's Trains shows this most visibly:



Note if the vehicle is long enough the CETS sprite is also a "combined" sprite at this point, to prevent the vehicle from visibly splitting into 3.

The good news is that the bounding boxes used for these "hill" sprites are similar enough to the intermediate curve sprites you don't see this issue - the vehicle is not split to three at this point. However this behaviour might affect any curvature-based bug fix if you have this situation:



Here the front and rear vehicles 1 and 3 would still be invisible even once all of the directions of the vehicle chain have updated. (I haven't been able to test this against the fixed code, it's only a theory this might present a problem even with the new curvature fix.)

For my own fix I used the idea of a "viewport candidate" - a sprite whose bounding box could potentially overlap the viewport and should receive a fresh `GetImage()` call. I don't think I trust NewGRF authors not to come up with some new and inventive way to break a scheme that is based on people using callbacks in the expected way. But this is not perfect; it can still break if a sprite has extreme differences in offsets, or if we are dealing with a set whose sprites exceed MAX_VEHICLE_PIXEL_X / MAX_VEHICLE_PIXEL_Y - assuming such a thing is possible, I haven't tried to see if OpenTTD will outright prevent you from doing this.

[Gadg8eer]

Had a crash while trying to convert a road vehicle depot after cheating time back to 1900 from 2200. Not sure if I just shouldn't be using time cheats too much but here's the crash report in case you think it should be fixed.

[JGR]

Now to be very annoying in the context of a fix... at least two of these sets have another situation in which the vehicle bounding box can change (and for long enough vehicles, the same swapping in and out of transparent and combined sprites). And it's not triggered by a curvature change either. On slopes, they also swap to different bounding boxes. Timberwolf's Trains shows this most visibly:



Note if the vehicle is long enough the CETS sprite is also a "combined" sprite at this point, to prevent the vehicle from visibly splitting into 3.

The good news is that the bounding boxes used for these "hill" sprites are similar enough to the intermediate curve sprites you don't see this issue - the vehicle is not split to three at this point. However this behaviour might affect any curvature-based bug fix if you have this situation:



Here the front and rear vehicles 1 and 3 would still be invisible even once all of the directions of the vehicle chain have updated. (I haven't been able to test this against the fixed code, it's only a theory this might present a problem even with the new curvature fix.)

For my own fix I used the idea of a "viewport candidate" - a sprite whose bounding box could potentially overlap the viewport and should receive a fresh `GetImage()` call. I don't think I trust NewGRF authors not to come up with some new and inventive way to break a scheme that is based on people using callbacks in the expected way. But this is not perfect; it can still break if a sprite has extreme differences in offsets, or if we are dealing with a set whose sprites exceed MAX_VEHICLE_PIXEL_X / MAX_VEHICLE_PIXEL_Y - assuming such a thing is possible, I haven't tried to see if OpenTTD will outright prevent you from doing this.

I managed to reproduce the rendering issue on a slope to flat transition (with no curve) with my my recent fix. I'll have to have a bit of a rethink.

I'll look into your version (it's now upstream, after all).
MAX_VEHICLE_PIXEL_xy seems like quite a large margin given how most viewport update regions are quite small. Large enough that it may not be necessary to invalidate the sprite bounds on direction changes any more?
I will have to do some more experiments/measurements.

Had a crash while trying to convert a road vehicle depot after cheating time back to 1900 from 2200. Not sure if I just shouldn't be using time cheats too much but here's the crash report in case you think it should be fixed.

This looks like a crash in the NewGRF debug window, presumably you had it open when doing something to the tile it was referring to.
I should be able to fix this later.
Edit: should be fixed now.

[Gadg8eer]

Had a crash while trying to convert a road vehicle depot after cheating time back to 1900 from 2200. Not sure if I just shouldn't be using time cheats too much but here's the crash report in case you think it should be fixed.

This looks like a crash in the NewGRF debug window, presumably you had it open when doing something to the tile it was referring to.
I should be able to fix this later.
Edit: should be fixed now.

Thank you! Good work!