Zillion: Finalize item locations in either generate_output or fill_slot_data

[Mysteryem]

What is this fixing or adding?

generate_output and fill_slot_data are run in separate threads so there is no guarantee that one runs before the other.

Before this patch, the threads were synchronized by making fill_slot_data wait for generate_output. The problem with doing this is that the test.general.test_implemented.test_slot_data test does not run generate_output, so Zillion had to be disabled from the test.

This patch changes the synchronization of the threads such that it does not matter which function is run first. Whichever function is run first will run finalize_item_locations() and the other function will wait until finalize_item_locations() is complete and has been cached in a new instance attribute.

Zillion has now been re-enabled for the test.general.test_implemented.test_slot_data test.

How was this tested?

I dumped slot data to a .json file and used thread synchronization to force fill_slot_data to run before generate_output. Then did the same but forced generate_output to run before fill_slot_data. The slot data json files written in both cases were identical.

I also ran the test_slot_data test with fill_slot_data modified to raise an exception to confirm that it was now being tested.

[Mysteryem]

@beauxq

[beauxq]

This might be ok, I'll have to look at it more closely.

But this is wrong:

finalize_item_locations() does not mutate self or any attribute of self

self.zz_system has references to the zz_locs that could be mutated by the lines
z_loc.zz_loc.item = z_item.zz_item
and
z_loc.zz_loc.item = multi_item

I'll look at it more when I get the time.

[Mysteryem]

But this is wrong:

finalize_item_locations() does not mutate self or any attribute of self

self.zz_system has references to the zz_locs that could be mutated by the lines z_loc.zz_loc.item = z_item.zz_item and z_loc.zz_loc.item = multi_item

I'll look at it more when I get the time.

Ah yes, you are correct. I'll have another look tomorrow, but initially it looks fine.

[beauxq]

This change doesn't look safe to me.
It might work right most of the time, but the get_slot_data function is looking at the objects being mutated by finalize_item_locations, so a race condition could throw a wrench in things with arbitrary timing.

There have been discussions in discord about a generation stage before multithreading but after all items are in their final locations.
If we had that, then finalize_item_locations could be there.
But right now we don't have any stage before multithreading and after all items are in their final locations. We have to wait until generate_output to rely on all items being in their final locations.

[Mysteryem]

finalize_item_locations() mutates the zz_loc of each Location belonging to the Zillion player. This zz_loc is a zilliandomizer.logic_components.locations.Location, so not to be confused with a Baseclasses.Location.

self.zz_system.get_game() constructs a Game instance, which calls Randomizer.get_region_data() which iterates through zilliandomizer.logic_components.regions.Region and converts them to json serializable RegionData objects. The process of converting to RegionData also converts the zilliandomizer.logic_components.locations.Location in that zilliandomizer.logic_components.regions.Region to json serializable LocationData, but these Location are the same as the zz_loc in finalize_item_location.

The problem with my test was that I only called self.zz_system.get_game() once before finalize_item_locations(), but it is the self.zz_system.get_game() call itself that must be run after finalize_item_locations(), whereas I incorrectly thought it was zillion.id_maps.get_slot_info() that had to be run after finalize_item_locations(). zillion.id_maps.get_slot_info() only relies on the result of get_game().

I could change generate_output to instead wait for fill_slot_data to call finalize_item_locations(), but because all worlds' generate_output/stage_generate_output are submitted to the ThreadPoolExecutor first, I'm concerned about deadlock in the case of all output threads being stuck in a generate_output/stage_generate_output waiting for a fill_slot_data that never comes.

Edit: I guess I can synchronize both threads and have whichever of generate_output or fill_slot_data that is run first call finalize_item_locations() while the other waits.

[Mysteryem]

Edit: I guess I can synchronize both threads and have whichever of generate_output or fill_slot_data that is run first call finalize_item_locations() while the other waits.

I have updated the PR to do this instead.

[beauxq]

This looks like it will probably be good. I'll play through a test game some time soon.

[beauxq]

tested a 2-player game

[beauxq]

also tested after latest change

[Exempt-Medic]

This has conflicts btw