At a first glance, code looks very neat and clean. One single exception: our coding style is int *a, not int* a (because int* a, b; is int *a; int b; and not int *a; int *b;).

I'll let somebody else decide whether this makes sense from a NewGRF perspective ❤️

Does this preserve backwards compatibility with existing NewGRF bridge sets and savegames created using them?

Does this preserve backwards compatibility with existing NewGRF bridge sets and savegames created using them?

Yes, older bridge sets are not affected by this patch, and they work just like they used to.

At a first glance, code looks very neat and clean. One single exception: our coding style is int *a, not int* a (because int* a, b; is int *a; int b; and not int *a; int *b;).

fixed 👍

Also, maybe should just have put it here, but I opened a discussion #9162, basically this PR as-is is mostly the minimum to support current features, but I think there are several more features that can be added without too much effort.

Please try running regression test locally before force pushing, it seems to trigger an infinite loop.

Edit: And now master should "detect" it sooner, please rebase :)

Please try running regression test locally before force pushing, it seems to trigger an infinite loop.

Edit: And now master should "detect" it sooner, please rebase :)

It is fixed now, it was due to overflow in 'script_bridgelist.cpp'.

Storing the coordinates of the bridge in the pool object could also lead to another optimisation opportunity, and in the future (with #9043) open up for bridges over bridges on the same row. Use a multimap to keep an index of all bridges on a row/column of the map, to avoid having to search the landscape array. The multimap could also be a basic map of interval trees, each interval being a bridge. An interval tree would also allow overlapping intervals, and that way make it efficient to search for overlapping bridges on a tile.

Using a data structure like that may also make it unnecessary to store bits in the map array about bridges crossing a tile, freeing up some space. The structure can be generated on load and should not need to be stored.

Storing the coordinates of the bridge in the pool object could also lead to another optimisation opportunity, and in the future (with #9043) open up for bridges over bridges on the same row. Use a multimap to keep an index of all bridges on a row/column of the map, to avoid having to search the landscape array. The multimap could also be a basic map of interval trees, each interval being a bridge. An interval tree would also allow overlapping intervals, and that way make it efficient to search for overlapping bridges on a tile.

Using a data structure like that may also make it unnecessary to store bits in the map array about bridges crossing a tile, freeing up some space. The structure can be generated on load and should not need to be stored.

At the moment there are only two bits allocated per tile for whether there is a bridge above the tile.
With an index like you suggested, that could easily become just one bit, but I think that it would be a good idea to retain at least one bit per tile.
By far the most common case is that a tile has 0 bridges over it. A single bit check for "there are no bridges here" means that any index scanning can be omitted in the most common case. This would probably make a measurable impact on larger maps where scanning the index may involve a fair bit of pointer chasing, or when zoomed out and and rendering a larger area of the map at once.

While everyone is talking about the code, I'll comment on the specs for a bit. Please correct me, if I misread the diff :)

• In this PR drawing custom bridges works very similar to stations:
  • An action 0 defines which sprites to draw in which position.
  • An action 3-2-1 chain then selected a single spriteset to provide the sprites for the layout.
• The action 0 enforces a "traditional" bridge design:
  • Separate sprites for road and the 3 original railtypes (rail, monorail, maglev).
  • Separate sprites for repetition patterns for bridges with length 2, 3, 2N, 4N+1, 4N+3
• The action 3-2-1 may check various variables, but then has to give a single result that provides sprites for
  • all combinations from the action 0 table
  • bridge heads
  • pillars
• I think the latter implies, that the action 3-2-1 chain has to return the same result for all tiles on a bridge.

I like the approach of starting with a minimal implementation, and later adding more parts.
But I think that the action 0 layout bakes too much "traditional" things into the behavior, that make no sense anymore:

• We have road, tram and railtypes. The original road, rail, monorail, maglev distinction makes no sense.
• I would like it a lot, if NewGRF could use different tile combinations for bridge lengths, which repeat every 3, 4, 5 or whatever tiles. Not just the original patterns.

So, instead I would like to propose a slightly different implementation:

• Deprecate action 0 property D (the sprite layout). If an action3 is present, just ignore the property, and always call the action 3.
• Also ignore property E bit 0 (drawing of far pillars).
• VarAction2 gains two variables that return the bridge length and the position along the bridge. So the NewGRF can choose different sprites for different bridge lengths and implement its own repetition patterns.
• VarAction2 gains some variables to detect the type of the bridge: maybe rail, road, tram, road+tram; but I would avoid exposing specific rail/road/tram types for now.
• The action 3-2-1 chain results in a spriteset with just a few entries:
  • Sprite below/behind the vehicle.
  • Sprite in front of the vehicle.
  • Pillars (not sure whether 1, 2 or 4 sprites are best here, see below).
• This would still result in a single sprite evaluation per tile.
  Alternatively the action 3 could be split into multiple "cargo types" similar to how rail/road/tram types do it:
  • 00: GUI icons (currently the PR uses type 0xFF for this)
  • 01: Front sprite
  • 02: Back sprite
  • 03: Pillars
• While this requires multiple sprite evaluations per tile, it allows changing the meaning of the results later on.
  This may become very handy, if the future comes up with better ideas for pillars.
• Note: resolving all sprites via action 3 implies that you cannot use original sprites in the bridge. But I do not consider that a particular useful goal.

Pillar rambling:
Pillars are currently very weird.

• There is only 1 pillar sprite.
• The sprite can be drawn once (near pillar only) or twice (near and far pillar) per tile.
• However, sometimes a bridge has 4 pillars per tile, so the sprite actually includes two pillars.
• Since the terrain can have different heights at all 4 corners of the tile, OpenTTD splits the pillar sprite algorithmically, and actually draws it as 4 sprites.
• It would be very nice, if we could get rid of this magic :) However, I have thought about bridge pillars for years, and how to draw fancy pillars with different sprites on different height levels (so a pillars could contain arcs spanning multiple height levels), and I never came up with a good idea.
• So the pillar-drawing topic is pretty much out-of-scope of this PR. But I consider it important to leave room for changes, so that future extensions could have multiple pillar sprites, instead of just one.
• It's probably also possible to extend this while keeping the "single sprite evaluation", but using "cargo types" leaves a lot more room.

Storing the coordinates of the bridge in the pool object could also lead to another optimisation opportunity, and in the future (with #9043) open up for bridges over bridges on the same row. Use a multimap to keep an index of all bridges on a row/column of the map, to avoid having to search the landscape array. The multimap could also be a basic map of interval trees, each interval being a bridge. An interval tree would also allow overlapping intervals, and that way make it efficient to search for overlapping bridges on a tile.

Using a data structure like that may also make it unnecessary to store bits in the map array about bridges crossing a tile, freeing up some space. The structure can be generated on load and should not need to be stored.

I implemented it by using multimap.

@frosch123 I implemented your specs.
Variable querying railtype is needed because monorail and maglev don't have overlay sprites.

Added Variable 60, querying bridge type of nearby tiles.

newbridge.zip
sample newgrf for this patch

Rebased to current master, and increased number of bridges per newgrf to 64000

Looks really interesting. I thought that some (unsolicited) feedback from a GRF author would be useful...

Core functionality

It wasn't clear if this uses an ID pool or fixed IDs? ID pool would obviously be nicer.
If it is an ID pool, then a query for nearby tile bridge GRFID is needed for utility of nearby bridge ID.

Bridge pillars

I think default behaviour is fine, ie. auto-generation of sprites for different ground slopes.
My preferred implementation for custom pillars would be to have a standard (vertical repeating) pillar sprite, and a flag for if to use automatic ground level pillar sprite generation. If not auto-generated, then the GRF author should provide custom sprites for the ground level, using a tile slope bitmask variable. This is analogous to houses disabling default foundations and then providing their own.

New variables

Height above ground tile.

Allows nice graphical variation, eg. thicker bridge deck if far enough above the ground.

Ground tile slope bitmask

Particularly for custom bridge pillars based on ground slope where they reach the ground.

Tile type and tile water type.

Would be very cool to customise bridge appearance bases on what is under it. Could add height warning if a road is under it, could make an embankment with cutout arches for road/rail, etc.

Nearby tile type and water type

Tile and water type of nearby tile could allow multi-tile spans based on what is actually under the bridge. ie. by querying the tile up and down the bridge by one position.

Bridge orientation (ne-sw vs nw-se)

Necessary for utility of nearby tile queries, ie. a GRF needs to know whether to look to the north west vs north east when looking for a neighbouring bridge to the north.

Length and Position along bridge of a nearby tile

Bridge ID of a nearby tile is useful for eg. joined bridge decks. But to have aligned pillars in neighbouring bridges , eg. for a suspension bridge, you also need length and position information.

Nearby tile is same position along a bridge of the same length

Very useful shortcut for handling parallel aligned bridges of identical length.

Another core functionality comment for support of road/rail/monorail/maglev, which wasn't clear how it was handled by the current implementation. There are two obvious ways to handle this. 1) Require bridge 0x44 checks for the Action3 02 (bridge back) and 04 (ramp) sprites, to provide road/rail/monorail/maglev variants. 2) Add bridge deck overlays for road, rail, monorail and maglev to orig_extra.grf and overlay those in the sprite stack.

I'd prefer the latter. It brings base road/rail/monorail/maglev handling in line with tram and new road/rail type handling. A flag to prevent drawing of the default bridge deck overlay might be nice, to give GRF authors full control.

edit As pointed out by Brickblock1, the latter is necessary for support of multiple base graphics sets.

Came across this pull request while trying to fix an older bridge set, and I'd like to offer a helping hand (if needed) to finish the implementation, because this is going to do a huge difference with all the road and rail types in CZTR. (This is a question primarily to @kiwitreekor.)

To add my 5 cents on the design, I'm very much in favor of bridge deck overlays, and basically I completely agree with @zephyris on the other suggestions as well.

For what it's worth I intend to look through this again at some point. It'll probably involve splitting it up in to smaller more manageable pieces though.

I've also made the deck overlay sprites which would be needed to overlay the original graphics default road/(mono)rail/maglev infrastructure... Though now I'm posting this, I realise I've overlooked toyland. I've got OpenGFX ones kicking around somewhere too.