Codechange: Use vector for airport tile layouts. #12607
Conversation
Simplify AirportSpec data by storing layout information together in a vector, instead of separate arrays. This removes manual memory management and separate count members. The default layouts will be copied instead of always referring to the originals.
src/newgrf.cpp
Outdated
| tile.ti.x = (int8_t)GB(tile.ti.x, 0, 8); | ||
| tile.ti.y = (int8_t)GB(tile.ti.y, 0, 8); |
There was a problem hiding this comment.
| tile.ti.x = (int8_t)GB(tile.ti.x, 0, 8); | |
| tile.ti.y = (int8_t)GB(tile.ti.y, 0, 8); | |
| tile.ti.x = static_cast<int8_t>(GB(tile.ti.x, 0, 8)); | |
| tile.ti.y = static_cast<int8_t>(GB(tile.ti.y, 0, 8)); |
There was a problem hiding this comment.
I'm confused by what this line is actually doing. Seems like just doing tile.ti.x & 0xFF ?
There was a problem hiding this comment.
It's converting an unsigned 8 bit value to a signed 8 bit value.. This allows the airport layout to "check" for clear tiles outside of the north edges.
| /* Convert terminator to our own. */ | ||
| tile.ti.x = -0x80; | ||
| tile.ti.y = 0; | ||
| tile.gfx = 0; |
There was a problem hiding this comment.
Not for this PR, but if the tile table is now a vector or span then the termination sentinel does not need to be saved in OpenTTD's state.
There was a problem hiding this comment.
Agreed, I figured this already touches enough :)
src/newgrf.cpp
Outdated
| tile.ti.x = (int8_t)GB(tile.ti.x, 0, 8); | ||
| tile.ti.y = (int8_t)GB(tile.ti.y, 0, 8); |
There was a problem hiding this comment.
I'm confused by what this line is actually doing. Seems like just doing tile.ti.x & 0xFF ?
| @@ -348,7 +348,7 @@ class BuildAirportWindow : public PickerWindowBase { | |||
| for (int i = 0; i < NUM_AIRPORTS; i++) { | |||
| const AirportSpec *as = AirportSpec::Get(i); | |||
| if (!as->enabled) continue; | |||
| for (uint8_t layout = 0; layout < as->num_table; layout++) { | |||
| for (uint8_t layout = 0; layout < static_cast<uint8_t>(as->layouts.size()); layout++) { | |||
There was a problem hiding this comment.
would it be nicer to change layout to be a size_t, rather than have the cast? Might need some signature changes as well I suppose
There was a problem hiding this comment.
That does lead to a bit of a rabbit hole, when there's no need for for a size_t field further down.
Motivation / Problem
AirportSpec contains some pointers, and pointers to pointers, along with counts of the number of items that they point to.
These pointers point to either the original airport layouts or custom airport layouts, which may or may not need to be freed afterwards.
The layout of the data is awkward to understand as well, being stored in separate arrays instead of an array of structs.
Description
Simplify AirportSpec data by storing layout information together in a vector, instead of separate arrays.
Adds the struct
AirportTileLayoutwhich holds both theAirportTileTabledata and the rotation, keeping this data together.The depot data is actually not changeable as it depends on the FSM, so this pointer is removed and replaced with a span as it never needs to be copied or changed.
This removes manual memory management and separate count members.
The default layouts will be copied instead of always referring to the originals.
Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.