fix: handle destroyed buildings properly

[winex]

READY FOR TESTING DEPENDS: #307, #308 don't squash, pls

because on_entity_destroyed event doesn't provide entity.position (it's already gone!) we need to remember .position field in global.tycoon_city_buildings[] and free this cell by new City::freeCellAtPosition() function. City::clearCell() does different thing, sadly - so i'm reworking it...

notes

i've fixed adding all entities from Constants.CITY_SPECIAL_BUILDINGS to global.tycoon_city_buildings[], since code was already doing it, but not fully. this one probably should have been named as global.tycoon_buildings while this could be split into city-built and player-built lists, it would require checking two arrays in event handlers.

ok, this was harder than i thought before. sorry for such big changes, but i couldn't find another way. diffs are horrible, looking at resulting files is easier my tests was successful, i've tried to avoid assert()s so any save should still be playable, but couldn't decide in a few places (iirc). will fix asap if you find anything!

tl;dr

• new function City::freeCellAtPosition()
• new functions Util:: {add,remove,get}GlobalBuilding()
• global.tycoon_city_buildings[] must now have every related tycoon-* entity, or on_removed() handler will skip it
• we always must script.register_on_entity_destroyed(entity) after creation
• migration script should fix possible errors in old saves
• logging might be a bit excessive, but helps testing. we can reduce it later
• NOTE: if any market (hw-store, etc...) is missing for a city after migration resets special buildings cache - it needs to be rebuilt or replaced closer
• _using CITY_RADIUS differs a little when placing vs find_entities_*() probably because town-hall vs city.center_

todo migration

• [x] finish position processing correctly
• [x] process each city grid looking for buildings/etc, as it could be wrong
• [x] add missing entities to global.tycoon_city_buildings[] by Util.addGlobalBuilding()

nothing else would be needed at runtime, as new buildings will already work

[bahrmichael]

I like this! I left the code to restore houses in a bit of a broken state unfortunately. Thank you for picking this up!

[winex]

i have a question so i won't revert your refactoring w/o reason:

• constructors and destructors (in C++ terms) must be in the same file close together, so we never miss things
• construction-event-handler currently does some citizen recalc and lights removal from global

is it fine to move this part back into City:: and sync it? and it probably should only callback related methods, without actually checking any internals, smth like this pseudo-code:

local function on_some_event(event)
    if this_is_related_to_city then
        City.destroyHouse(city, event.bulding.posGrid)
    end
    if this_is_related_to_something_else then
        something_else.callback(event.entity_name, ...)
    end
end

[bahrmichael]

is it fine to move this part back into City:: and sync it?

I'm fine with that! For the most of it, don't hold strong opinions on lua coding because I lack the experience with it.

[winex]

after reworking destruction/clearing code paths... oh, lol! totally missed the point that city.grid is being enlarged over time from {1, 1}, so my .posGrid becomes definitely wrong. if it was centered and supported negative coordinates - it would work perfectly...

reworking city.grid would require very accurate migration script - i'm not ready for this, yet. i'm writing like 4th migration already, which is not good...

then, we must remember every building's .position instead :( sometimes Factorio makes simple things sooo hard and ugly

[winex]

-- event.name:
-- on_player_entity_mined = 67
-- on_entity_destroyed = 160

ok, this is why we must remember .position for every building:

-- valid: true
75070.809 Script @__tycoon__/construction-event-handler.lua:103: on_removed(): event: 67 unit: 394 valid: true position: {x = 137, y = 277} city: Ironhaven name: tycoon-house-simple-6
-- valid: false
75131.876 Script @__tycoon__/construction-event-handler.lua:103: on_removed(): event: 160 unit: 6499 valid: false position: {x = 179, y = 265} city: Ironhaven name: tycoon-house-simple-8

mined first one, but the last one i machine-gunned...

garden mining works (and city rebuilt it), but destruction differs - do we put it into global.tycoon_city_buildings as well? edit: yes

[winex]

this possible mismatch hell (separate commit, just to emphasize) took last pieces of my energy today! just can't handle every possible thing in the universe... probably nobody will understand this code fully :(

good thing that it works nicely now, except that i couldn't catch event for supply buildings and train-stations even if i register them - idk what's wrong there

i'll try to split it a bit, so the change is not that big: new functions + change. then finish city grid parsing when i get what to do there. i have few saves that could be good test case

but now i need to sleep, lol...

[winex]

this town was rebuilt succesfully few times (>10 buildings) around town-hall:

[winex]

the last thing depends on mentioned PR, i'll rebase this after that one is merged

fixed and resolved few more things, it looks way better now

[winex]

one last step (from todo) and this is going to be ready for testing...

[winex]

when machine-gunning tycoon-treasury, city can't rebuild it, but this is another issue...

[winex]

ok, marking this as ready for testing! i've been working on top of this branch already and everything worked fine.

i'll rebase one last time after deps are fully merged in...

[winex]

fixed my own errors and it looks like i've tested almost every code path now. this work was hard and i'm glad it's nearing to an end. migration works nicely and fixes many saves with different states as it can be seen in the logs

@bahrmichael waiting for feedback of your tests...

[winex]

rebased on updated #308 (avoid goto), reused utility functions

global.tycoon_city_buildings now only exists in util.lua wrappers and migrations (as latter probably should depend on minimal amount of utility calls)

[winex]

github was a bit wrong in conflicts again, now it's cleaner than ever! ;)

[bahrmichael]

I'll test and merge this later today 👍 Thank you very much for your deep dive into the structure handling!

[bahrmichael]

Just tested by nuking a town, and eventually it rebuilt. It took much longer than expected (building houses in many other places first), but eventually got back to the destroyed houses.

[winex]

sorry, missed window for review changes and can't add anything more after this was merged and closed, so i created another little PR

Just tested by nuking a town, and eventually it rebuilt. It took much longer than expected (building houses in many other places first), but eventually got back to the destroyed houses.

yeah, when buildingLocationQueue gets bigger and bigger, like 30-ish or more, it takes time to get to "new entries". i used addBuildingLocations() because it does many more checks, so nothing goes wrong for now.

it probably should be changed somehow to iterate from city center in circles as i commented here (i think L1529 can still happen if force-pushed into queue): https://github.com/bahrmichael/factorio-tycoon/blob/8956c35c5e4119daaad2f95dfdabf248929afefa/city.lua#L1527-L1531 didn't try to change it (back then) to keep this important PR minimal. thanks for accepting my contributions! :+1: