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

Feature: NewGRF Bridges without overriding #9161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kiwitreekor
Copy link
Contributor

@kiwitreekor kiwitreekor commented May 1, 2021

Motivation / Problem

Currently NewGRF Feature 0x06 (Bridge) is very limited. A bridge can only be added by replacing the existing bridge, and it is impossible to build more than 13 types of bridges. Also, it is impossible to change the graphic depending on the surrounding environment or random bits.

Description

This patch introduces action 1-2-3 chain for bridges, and provides possibility to add bridges instead of replacing original bridges.

Map array changes

  • m2 + m6 is now used for index into bridge pool.
  • bit 3 of type is freed, now only bit 2 is used.
  • lower 4 bits of m3 is used for random bits. random bits are now stored in bridge pool.

NewGRF specification changes

  • Action0
    • 0x00 (byte): Substitute bridge type. If this property exists, the patch consider this item is not replacing original bridge. This property must be at first (if exists). If it is set to 0xFF, it will disable original bridge type.
    • Property 0x0D and 0x0E bit 0 are ignored.
  • Action2
    • Basic Action2
      • Single set
    • VarAction2
      • 0x40: Construction date
      • 0x41: Terrain type
      • 0x42: Position along bridge (reversed position in upper 16 bits)
      • 0x43: Length of bridge
      • 0x44: Transport type
        • bit 0~7 - 0: rail, 1: road, 2: tram, 3: road+tram
        • bit 8~15 - railtype or roadtype
        • bit 16~23 - tramtype
      • 0x60: Bridge ID of nearby tile
      • ... (can be added later)
  • Action3
    • 00: GUI icons (1 sprite)
    • 01: Front sprites (2 sprites)
    • 02: Back sprites (2 sprites)
    • 03: Pillar sprites (2 sprites)
    • 04: Ramp sprites (8 sprites)

Limitations

Callbacks are not yet implemented.
Total bridges are limited to 16711680.

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

@kiwitreekor kiwitreekor force-pushed the newbridge branch 2 times, most recently from 519b05a to 96fb3c8 Compare May 1, 2021 12:31
@michicc
Copy link
Member

michicc commented May 1, 2021

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 ❤️

@James103
Copy link
Contributor

James103 commented May 1, 2021

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

@kiwitreekor
Copy link
Contributor Author

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.

@kiwitreekor
Copy link
Contributor Author

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 👍

StringID material; ///< the string that contains the bridge description
StringID transport_name[2]; ///< description of the bridge, when built for road or rail
PalSpriteID** sprite_table; ///< table of sprites for drawing the bridge
byte flags; ///< bit 0 set: disable drawing of far pillars.
Copy link
Contributor

@nielsmh nielsmh May 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be an enum for the flags?

src/table/newgrf_debug_data.h Outdated Show resolved Hide resolved
@nielsmh
Copy link
Contributor

nielsmh commented May 1, 2021

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.

@kiwitreekor kiwitreekor force-pushed the newbridge branch 2 times, most recently from 839e872 to 20bee78 Compare May 1, 2021 13:46
@glx22
Copy link
Contributor

glx22 commented May 1, 2021

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

@kiwitreekor kiwitreekor force-pushed the newbridge branch 3 times, most recently from 5a13aa2 to df81019 Compare May 1, 2021 15:08
@kiwitreekor
Copy link
Contributor Author

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'.

@kiwitreekor kiwitreekor force-pushed the newbridge branch 3 times, most recently from ac2eb81 to 02676ca Compare May 1, 2021 16:06
src/tunnelbridge_cmd.cpp Outdated Show resolved Hide resolved
@kiwitreekor kiwitreekor force-pushed the newbridge branch 2 times, most recently from e8cc5b3 to 64d58e2 Compare May 2, 2021 17:00
@nielsmh
Copy link
Contributor

nielsmh commented May 2, 2021

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.

@JGRennison
Copy link
Contributor

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.

@frosch123
Copy link
Member

frosch123 commented May 2, 2021

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.

@kiwitreekor kiwitreekor force-pushed the newbridge branch 3 times, most recently from 771b8a6 to 1e59558 Compare May 5, 2021 05:11
@kiwitreekor
Copy link
Contributor Author

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.

src/tunnelbridge_cmd.cpp Outdated Show resolved Hide resolved
@kiwitreekor kiwitreekor force-pushed the newbridge branch 5 times, most recently from 7c25f38 to 208271c Compare May 6, 2021 07:04
@kiwitreekor
Copy link
Contributor Author

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

@kiwitreekor
Copy link
Contributor Author

Added Variable 60, querying bridge type of nearby tiles.

@kiwitreekor kiwitreekor force-pushed the newbridge branch 2 times, most recently from a88243a to 9813335 Compare May 9, 2021 12:15
@kiwitreekor kiwitreekor force-pushed the newbridge branch 3 times, most recently from 020c0b9 to b6964d0 Compare February 19, 2022 05:44
@kiwitreekor
Copy link
Contributor Author

newbridge.zip
sample newgrf for this patch

image

@kiwitreekor kiwitreekor force-pushed the newbridge branch 3 times, most recently from 1ebeeb0 to 53c11c7 Compare February 21, 2022 17:03
@2TallTyler 2TallTyler added the needs review: NewGRF Review requested from a NewGRF expert label Nov 9, 2022
src/bridge_gui.cpp Outdated Show resolved Hide resolved
@kiwitreekor kiwitreekor force-pushed the newbridge branch 2 times, most recently from c5b5d6d to 42eefa1 Compare May 9, 2023 17:23
@kiwitreekor
Copy link
Contributor Author

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

psid = base_offset + GetBridgeSpriteTable(b->type, piece);
}
else {
const PalSpriteID *psid;

Check notice

Code scanning / CodeQL

Declaration hides variable Note

Variable psid hides another variable of the same name (on
line 1657
).
static const BridgeID NEW_BRIDGE_OFFSET = 13; ///< Offset for new bridges.
static const BridgeID NUM_BRIDGES = 64000; ///< Number of supported bridges
static const BridgeID NUM_BRIDGES_PER_GRF = NUM_BRIDGES; ///< Number of supported bridges per NewGRF
static const BridgeID INVALID_BRIDGE_TYPE = 0xFFFF; ///< An invalid bridge

Check notice

Code scanning / CodeQL

Unused static variable Note

Static variable INVALID_BRIDGE_TYPE is never read.

static const BridgeID NEW_BRIDGE_OFFSET = 13; ///< Offset for new bridges.
static const BridgeID NUM_BRIDGES = 64000; ///< Number of supported bridges
static const BridgeID NUM_BRIDGES_PER_GRF = NUM_BRIDGES; ///< Number of supported bridges per NewGRF

Check notice

Code scanning / CodeQL

Unused static variable Note

Static variable NUM_BRIDGES_PER_GRF is never read.
typedef uint BridgeType; ///< Bridge spec number.


static const BridgeID NEW_BRIDGE_OFFSET = 13; ///< Offset for new bridges.

Check notice

Code scanning / CodeQL

Unused static variable Note

Static variable NEW_BRIDGE_OFFSET is never read.
@zephyris
Copy link
Contributor

zephyris commented Sep 3, 2023

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.

@zephyris
Copy link
Contributor

zephyris commented Sep 3, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review: NewGRF Review requested from a NewGRF expert
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants