DOOM 1993: implement new game

[Daivuk]

New implementation.

Been well testing by the community.

The APDOOM source port is found here:
https://github.com/Daivuk/apdoom

Graphical changes to the original game.

They are packaged into a new APDOOM.WAD file.

[Jarno458]

I see a lot of things are based on Set's now, the problem with python sets is thier order isnt deterninistic

so i ran a test generations .\Generate.py --seed 1
then rename the output file
and repeat .\Generate.py --seed 1

it seem your progression logic is correctly kept the same across seeds, the order the locations are logged are not. i am not sure what to make of it, it seems to work but i think its best if the locations had a set order

[Jarno458]

I also tested generating a 2player game, but it seems to hang indenfently at the Creating Items.. step
if i abort it, it seems to break at

Creating World.
Creating Items.
Traceback (most recent call last):
  File "T:\Git\Archipelago\Generate.py", line 643, in <module>
    main()
  File "T:\Git\Archipelago\Generate.py", line 226, in main
    callback(erargs, seed)
  File "T:\Git\Archipelago\Main.py", line 128, in main
    AutoWorld.call_all(world, "create_items")
  File "T:\Git\Archipelago\worlds\AutoWorld.py", line 92, in call_all
    call_single(multiworld, method_name, player, *args)
  File "T:\Git\Archipelago\worlds\AutoWorld.py", line 84, in call_single
    return method(*args)
  File "T:\Git\Archipelago\worlds\doom_1993\__init__.py", line 250, in create_items
    self.multiworld.itempool.append(item)

yaml used:

description: Not provided
name: 'Jarno{NUMBER}'
game:
  DOOM 1993: 1
DOOM 1993: {}
---
description: Not provided
name: 'Jarno{NUMBER}'
game:
  DOOM 1993: 1
DOOM 1993: {}

command used: py -3.10 .\Generate.py --seed 100

[Daivuk]

I also tested generating a 2player game, but it seems to hang indenfently at the Creating Items.. step if i abort it, it seems to break at

Creating World.
Creating Items.
Traceback (most recent call last):
  File "T:\Git\Archipelago\Generate.py", line 643, in <module>
    main()
  File "T:\Git\Archipelago\Generate.py", line 226, in main
    callback(erargs, seed)
  File "T:\Git\Archipelago\Main.py", line 128, in main
    AutoWorld.call_all(world, "create_items")
  File "T:\Git\Archipelago\worlds\AutoWorld.py", line 92, in call_all
    call_single(multiworld, method_name, player, *args)
  File "T:\Git\Archipelago\worlds\AutoWorld.py", line 84, in call_single
    return method(*args)
  File "T:\Git\Archipelago\worlds\doom_1993\__init__.py", line 250, in create_items
    self.multiworld.itempool.append(item)

yaml used:

description: Not provided
name: 'Jarno{NUMBER}'
game:
  DOOM 1993: 1
DOOM 1993: {}
---
description: Not provided
name: 'Jarno{NUMBER}'
game:
  DOOM 1993: 1
DOOM 1993: {}

command used: py -3.10 .\Generate.py --seed 100

This is fixed.

[Daivuk]

I see a lot of things are based on Set's now, the problem with python sets is thier order isnt deterninistic

so i ran a test generations .\Generate.py --seed 1 then rename the output file and repeat .\Generate.py --seed 1

it seem your progression logic is correctly kept the same across seeds, the order the locations are logged are not. i am not sure what to make of it, it seems to work but i think its best if the locations had a set order

I am using Sets for location_name_groups and item_name_groups. This is their base definition.
The only other Set I use if for this loc_name in self.death_logic_locations which should be deterministic, but I have change it to a list. :)

[Jarno458]

I see a lot of things are based on Set's now, the problem with python sets is thier order isnt deterninistic
so i ran a test generations .\Generate.py --seed 1 then rename the output file and repeat .\Generate.py --seed 1
it seem your progression logic is correctly kept the same across seeds, the order the locations are logged are not. i am not sure what to make of it, it seems to work but i think its best if the locations had a set order

I am using Sets for location_name_groups and item_name_groups. This is their base definition. The only other Set I use if for this loc_name in self.death_logic_locations which should be deterministic, but I have change it to a list. :)

you also use a set for Events.events after my previus comment. dont want to be an issue

[Daivuk]

I see a lot of things are based on Set's now, the problem with python sets is thier order isnt deterninistic
so i ran a test generations .\Generate.py --seed 1 then rename the output file and repeat .\Generate.py --seed 1
it seem your progression logic is correctly kept the same across seeds, the order the locations are logged are not. i am not sure what to make of it, it seems to work but i think its best if the locations had a set order

I am using Sets for location_name_groups and item_name_groups. This is their base definition. The only other Set I use if for this loc_name in self.death_logic_locations which should be deterministic, but I have change it to a list. :)

you also use a set for Events.events after my previus comment. dont want to be an issue

Thanks! Missed that one. Fixed

[Daivuk]

Note to reviewer: This is now final, I did all the features I wanted to do for a 1.0.0 release of APDOOM.

[Jarno458]

Can confirm the hanging when generating with more than two players is fixed
still the output order of locations seems to be random, but each locations gets the same item assigned so the seed reproduceability is correctly working

[Daivuk]

Can confirm the hanging when generating with more than two players is fixed still the output order of locations seems to be random, but each locations gets the same item assigned so the seed reproduceability is correctly working

Could it be the way I am iterating locations to add them to regions? Locations are in a dictionnary

        # Add locations to regions
        for loc_id in Locations.location_table:

So I am thinking their order is not predictable. I could just loop IDs in order instead. Other games seem to store their location_table the same way. But maybe not looping through them the same way.

[Jarno458]

Can confirm the hanging when generating with more than two players is fixed still the output order of locations seems to be random, but each locations gets the same item assigned so the seed reproduceability is correctly working

Could it be the way I am iterating locations to add them to regions? Locations are in a dictionnary

        # Add locations to regions
        for loc_id in Locations.location_table:

So I am thinking their order is not predictable. I could just loop IDs in order instead. Other games seem to store their location_table the same way. But maybe not looping through them the same way.

Dicts should preserve order in python so not really sure what the cause here
I tryed locally to change your location_name_groups and item_name_groups to Dict[str, Tuple[str, ...]] rather then Dict[str, Set[str]] but that didnt help either

[Jarno458]

I google a bit more, while Dict do preserver order dict.items() returns a set like object of tuples, so its the .items() that breaks order it seems

[Daivuk]

I google a bit more, while Dict do preserver order dict.items() returns a set like object of tuples, so its the .items() that breaks order it seems

I was googling also, in Subnautica I was doing the .items(), but in Doom I am iterating them differently.

The items() method returns a view object. The view object contains the key-value pairs of the dictionary, as tuples in a list.

It seems like .items() is what I want here. It should keep them ordered

[Jarno458]

Oke iv been trying by adding sorted() around a few methods but that seemed to have no effect

[Daivuk]

Oke iv been trying by adding sorted() around a few methods but that seemed to have no effect

Just tried another game, the Locations: in the spoiler is also different. So I don't think DOOM broke that.
To be ultra certain, I removed this commit and tried it again on The Witness, same issue. Some items in locations are in different order.

[Daivuk]

Found the issue:

Typo :)

[Jarno458]

Can confirm, generates perfectly now

[ScipioWright]

It looks good to me, no real issues. I put in some review comments on things that could use a little cleaning up, but none are particularly necessary.

[ScipioWright]

Did many many test generations last night, both alone and with an assortment of other games, and with every setting combination for Doom that I could think to do (it's fairly limited, thankfully). All successfully generated.

Also played it a bit last night solo and it seemed to work perfectly fine. Also really liked what was done with the ap item appearance, I think it looks excellent.

[Daivuk]

Did many many test generations last night, both alone and with an assortment of other games, and with every setting combination for Doom that I could think to do (it's fairly limited, thankfully). All successfully generated.

Also played it a bit last night solo and it seemed to work perfectly fine. Also really liked what was done with the ap item appearance, I think it looks excellent.

Thanks! Glad you like it. And glad it doesnt fail tests :D

[DeamonHunter]

Mostly just some nitpicking. Logic wise it seems fine to me.

[ThePhar]

Logic seems fine, there are quite a few PEP8 issues in __init__.py and a lot of code that is redundant and can be removed or replaced by built-in AP / python functions. Before I review those though, I'm gonna reach out to you via Discord for your opinion on suggestions for __init__.py refactors or if you'd rather just have the optimizations/PEP8 violations reviewed.

Pending my review for anyone else reviewing.

[ThePhar]

Logic looks solid and even though there's technically some PEP8 (and 120 char per line) issues in the generated files, I'm able to look past them.

The __init__.py file though has a lot that can be refactored to utilize AP functions or python functions, and to remove redundant code. A couple conditionals can also be moved to help optimize the flow. I wrote a refactored version of it (you can take the whole thing or use the info to restructure it yourself) in Daivuk#1

Quick link to file: https://github.com/Daivuk/Archipelago/blob/0d021fd8f5750ba91fec06a35c892cd112015988/worlds/doom_1993/__init__.py

At the very least if the PEP8 and 120 char issues can be resolved should be good to merge, but would recommend going through the refactor for anything you can use. Feel free to request review from me again once you've had a chance to go through it.