JGR's Patch Pack

Forum for technical discussions regarding development. If you have a general suggestion, problem or comment, please use one of the other forums.

Moderator: OpenTTD Developers

Eddi
Tycoon
Tycoon
Posts: 7559
Joined: 17 Jan 2007 00:14

Re: JGR's Patch Pack

Post by Eddi »

Timberwolf wrote: 02 Jan 2021 21:45I 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.
You might not exactly be interested in Ferion, but if you are, have fun :)
Timberwolf
Traffic Manager
Traffic Manager
Posts: 238
Joined: 22 May 2006 18:25
Location: London-ish.
Contact:

Re: JGR's Patch Pack

Post by Timberwolf »

Eddi wrote: 02 Jan 2021 22:41
Timberwolf wrote: 02 Jan 2021 21:45I 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
Tycoon
Tycoon
Posts: 7559
Joined: 17 Jan 2007 00:14

Re: JGR's Patch Pack

Post by Eddi »

Timberwolf wrote: 02 Jan 2021 23:14Does 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]
}
You might not exactly be interested in Ferion, but if you are, have fun :)
User avatar
Snail
Tycoon
Tycoon
Posts: 1252
Joined: 28 Apr 2003 18:52
Contact:

Re: JGR's Patch Pack

Post by Snail »

JGR wrote: 02 Jan 2021 20:15 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! :D Thank you very much.
The French Narrow Gauge Train Set is now released! Get it here
michael blunck
Tycoon
Tycoon
Posts: 5902
Joined: 27 Apr 2005 07:09
Contact:

Re: JGR's Patch Pack

Post by michael blunck »

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

regards
Michael
Image
Argus
Traffic Manager
Traffic Manager
Posts: 182
Joined: 16 Oct 2018 08:31

Re: JGR's Patch Pack

Post by Argus »

Problem with joker patchpack games persist.
Attachments
VRivervalleyauz3joker.sav
(971.74 KiB) Downloaded 69 times
User avatar
JGR
Tycoon
Tycoon
Posts: 2205
Joined: 08 Aug 2005 13:46
Location: Ipswich

Re: JGR's Patch Pack

Post by JGR »

Timberwolf wrote: 02 Jan 2021 21:45 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:

Image

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.
Argus wrote: 03 Jan 2021 11:31 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.
Ex TTDPatch Coder
Patch Pack, Github
Dad-Coder since April 2018

Avatar by MoonsongWolf
Argus
Traffic Manager
Traffic Manager
Posts: 182
Joined: 16 Oct 2018 08:31

Re: JGR's Patch Pack

Post by 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. :)
User avatar
JGR
Tycoon
Tycoon
Posts: 2205
Joined: 08 Aug 2005 13:46
Location: Ipswich

Re: JGR's Patch Pack

Post by JGR »

Timberwolf wrote: 02 Jan 2021 21:45 (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.
Ex TTDPatch Coder
Patch Pack, Github
Dad-Coder since April 2018

Avatar by MoonsongWolf
Eddi
Tycoon
Tycoon
Posts: 7559
Joined: 17 Jan 2007 00:14

Re: JGR's Patch Pack

Post by Eddi »

JGR wrote: 03 Jan 2021 16:18 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
Last edited by Eddi on 04 Jan 2021 13:14, edited 1 time in total.
You might not exactly be interested in Ferion, but if you are, have fun :)
User avatar
JGR
Tycoon
Tycoon
Posts: 2205
Joined: 08 Aug 2005 13:46
Location: Ipswich

Re: JGR's Patch Pack

Post by JGR »

Eddi wrote: 04 Jan 2021 13:04
JGR wrote: 03 Jan 2021 16:18 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.
Ex TTDPatch Coder
Patch Pack, Github
Dad-Coder since April 2018

Avatar by MoonsongWolf
Eddi
Tycoon
Tycoon
Posts: 7559
Joined: 17 Jan 2007 00:14

Re: JGR's Patch Pack

Post by Eddi »

JGR wrote: 04 Jan 2021 13:12 I'm not going to PR it upstream, so you needn't worry about it.
that doesn't sound very reassuring :p
You might not exactly be interested in Ferion, but if you are, have fun :)
michael blunck
Tycoon
Tycoon
Posts: 5902
Joined: 27 Apr 2005 07:09
Contact:

Re: JGR's Patch Pack

Post by 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
Image
User avatar
JGR
Tycoon
Tycoon
Posts: 2205
Joined: 08 Aug 2005 13:46
Location: Ipswich

Re: JGR's Patch Pack

Post by JGR »

Eddi wrote: 04 Jan 2021 13:17
JGR wrote: 04 Jan 2021 13:12 I'm not going to PR it upstream, so you needn't worry about it.
that doesn't sound very reassuring :p
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.
michael blunck wrote: 04 Jan 2021 13:18 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).
Ex TTDPatch Coder
Patch Pack, Github
Dad-Coder since April 2018

Avatar by MoonsongWolf
michael blunck
Tycoon
Tycoon
Posts: 5902
Joined: 27 Apr 2005 07:09
Contact:

Re: JGR's Patch Pack

Post by 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
Image
Eddi
Tycoon
Tycoon
Posts: 7559
Joined: 17 Jan 2007 00:14

Re: JGR's Patch Pack

Post by 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
You might not exactly be interested in Ferion, but if you are, have fun :)
Timberwolf
Traffic Manager
Traffic Manager
Posts: 238
Joined: 22 May 2006 18:25
Location: London-ish.
Contact:

Re: JGR's Patch Pack

Post by 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:

Image

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:

Image

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.
User avatar
Gadg8eer
Traffic Manager
Traffic Manager
Posts: 157
Joined: 14 Dec 2019 14:22

Re: JGR's Patch Pack

Post by 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.
Attachments
crash.txt
(56.63 KiB) Downloaded 12 times
I have Asperger's, please be easy on me about stuff. My apologies if I've been a problem for you in the past.
User avatar
JGR
Tycoon
Tycoon
Posts: 2205
Joined: 08 Aug 2005 13:46
Location: Ipswich

Re: JGR's Patch Pack

Post by JGR »

Timberwolf wrote: 05 Jan 2021 01:03 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:

Image

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:

Image

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.
Gadg8eer wrote: 05 Jan 2021 12:10 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.
Ex TTDPatch Coder
Patch Pack, Github
Dad-Coder since April 2018

Avatar by MoonsongWolf
User avatar
Gadg8eer
Traffic Manager
Traffic Manager
Posts: 157
Joined: 14 Dec 2019 14:22

Re: JGR's Patch Pack

Post by Gadg8eer »

JGR wrote: 05 Jan 2021 12:25
Gadg8eer wrote: 05 Jan 2021 12:10 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!
I have Asperger's, please be easy on me about stuff. My apologies if I've been a problem for you in the past.
Post Reply

Return to “OpenTTD Development”

Who is online

Users browsing this forum: No registered users and 13 guests