Elevated roads do not prevent larger buildings from appearing beneath them.

[victor_18993]

The GitHub mirror issue has now been resolved.

After my SVN user was added to the author mapping, I made a minimal commit that only changed a comment to check the synchronization. The mirror processed all pending revisions correctly and the workflows started running again.

The r12122 nightly is now available, together with the new builds for Windows, Linux and macOS.

Thanks to ceeac for identifying the cause and to Prissi for fixing the mapping.

[victor_18993]

I have to reopen this one: r12121 is no longer in trunk, as I reverted it in r12124 after the automated tests failed. The reason turned out to be more general than the original patch.

The clearance check itself behaved correctly in builds with graphics. It rejected an elevated way or bridge when the building below required more clearance than the deck provided, using get_height_clearance() from r12106.

What I had missed is that get_height_clearance() is derived from the geometry of the building images, and the headless build currently discards that geometry:

  - in image_reader.cc, the COLOUR_DEPTH == 0 path replaces every image geometry with x=0, y=0, w=1, h=1;
  - in simgraph0.cc, base_tile_raster_width remains fixed at 16 because set_base_raster_width() is an empty stub.

As a result, the calculation runs on constant values and does not distinguish between tall and flat buildings. Measured with pak64 124-4, which is the pakset used by the CI:

    building                headless  graphical
    Theatre (2x2, tall)        2          2
    Tennis_Court (2x2, flat)    2          0
    BusStop                    2          0

Across all 291 pak64 buildings, the graphical build produces clearance values of 0/1/2/6/10, distributed as 100/115/21/54/1. The headless build returns 2 or more for every building.

With a way offset of 1, r12121 therefore rejected almost every elevated way or bridge over a building in the headless tests. That is what caused Automated Tests to fail. Graphical builds were not affected.

I think this is worth correcting at the source rather than adding a workaround around the clearance check, because get_height_clearance() is already used elsewhere. r12106 compares it with the available overhead clearance during city growth and renovation in simcity.cc:3489 and :3656, without a COLOUR_DEPTH guard.

From the code and the measured values, a headless build would reject buildings in cases where the graphical build accepts 215 of the 291 pak64 buildings. I could not reproduce enough organic city growth in a small test map to observe this directly, so this part is a deduction rather than a reproduced gameplay result.

Patch 1 keeps the image geometry in headless builds while still discarding the pixel data:

  - image_reader.cc keeps x/y/w/h and calls alloc(4) to release the pixel buffer;
  - simgraph0.cc stores the configured raster width;
  - ground_desc.cc replaces two implicit "w < 2" headless checks with explicit COLOUR_DEPTH == 0 guards.

The ground_desc.cc guards are necessary. create_alpha_tile() and create_texture_from_tile() previously returned early because headless images had w=1. Once the real width is preserved, those functions otherwise enter texture generation after the pixel data has been discarded and crash with SIGSEGV in init_ground_textures(). I searched for other uses of the same implicit check and found no others.

Memory usage:

  - pak64: no increase, 20.2 MB -> 19.9 MB peak;
  - pak128.german: 28.25 MB -> 30.9 MB, an increase of 2.6 MB.

These values were measured three times while alternating the binaries. My first attempt increased memory by 189 MB because setting len = 4 did not release the buffer already allocated by alloc(); calling alloc(4) does.

Patch 2 is r12121 unchanged, reapplied on top of patch 1. It is attached mainly as validation of the underlying fix:

                      patch 1 only  both patches
    headless suite        223/223        227/227
    graphical suite        223/223        227/227

Before patch 1, the combined version failed at test 172 in the headless build.

There is another possible design: makeobj could calculate the clearance and store it in the pak. The .dat format can already specify the value through the existing field in building_desc.cc:78, using 255 as the sentinel.

That would be cleaner in terms of data ownership, but it would require a format change and rebuilt paksets. Existing paks would continue to contain no calculated value and would therefore keep the incorrect headless behaviour.

For that reason I am proposing the geometry-preservation route, but I would like your preference before taking it further: should this remain derived from image geometry at runtime, or should the value be moved into the pak through makeobj?

Not verified: MSVC, which I cannot build locally.

[prissi]

I would suggest keeping the image dimensions. These informations may be needed later for other patches too. Your idea is exactly what I thought off too.

[victor_18993]

The corrected implementation is in trunk and ready to test.

• r12129 puts the height clearance check for bridges and elevated ways back, keeping the
  image geometry it needs in headless builds.
  
• r12130 fixes a memory waste not directly related to this thread. Kept separate so it can
  be reviewed or reverted on its own.
  

A correction to the figures I posted earlier: the baseline was measured before the unpatched build
had finished loading the images. Measured at the same point in both, in headless builds, the
ones servers use, the patch does not increase memory consumption, it reduces it from
248.9 MB to 30.2 MB with pak128.german and from 32.2 MB to 22.6 MB with pak64. I have
not worked out why the adler dedup barely fires there.

This saving does not affect normal PC or Android builds with graphics, since that branch is only
compiled when there is no graphics backend.

Validation:

• Full suite: 237/237, three runs per configuration.
  
• The graphical and headless builds return the same height clearance for all 291 buildings of

pak64.

• Negative controls: without the geometry the suite fails refusing a legal bridge, without the rule

it fails allowing an illegal one, in both builds.

• Values of height_clearance declared explicitly in a .dat file are covered too.
• Each revision was built and ran the full suite on its own, and the Windows nightlies are green.

The earlier simcity.cc warning is still open.

[prissi]

I think image_t would benefit from using freelist, as every object is allocated usually 256 bytes with many stdlibc.

Attached is a patch using freelist, putting image_id to the first item (can be discussed, maybe x,z,wh is better?) and does not use data at all for posix. Unfortunately, MSVC cannot built servers anymore, so I cannot debug it on this machine,

[victor_18993]

Thanks, Prissi.

I will try to review the freelist/image_t optimization as soon as possible and investigate the implications of the proposed changes.
At the moment I am finishing some SDL3 tests, so I would prefer to complete those first before switching context. I expect the review should not take me more than a couple of days, but I cannot commit to a specific date in case the testing uncovers something that needs more work.

Once I can focus on it properly, I will go through the patch and check the optimization and any possible side effects.