Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faxanadu: implement new game #3059

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Daivuk
Copy link
Collaborator

@Daivuk Daivuk commented Mar 30, 2024

What is this fixing or adding?

New game, Faxanadu.

Faxanadu is an NES game. It does not require the server to have the ROM to generate. The patching is done at runtime in the client. The client is a custom-made NES emulator for AP Faxanadu. (Called Daxanadu, because my name is Daivuk...)

The client can be found here: https://github.com/Daivuk/Daxanadu

How was this tested?

It has been played for months. apworld was available since a while.

If this makes graphical changes, please attach screenshots.

The palette on the NES is restricted, especially in this game. Only 3 colors could have been used for the AP icon. Here how it looks in-game:
image

Or it shows the proper graphic if they are your own items:
image

In the menus, the icon has a different palette. Here an example receiving progression item:
image
Followed by a dialog explaining what it is:
image

When an AP item is bought from a shop, it shows "SOLD OUT":
image

More client-only options that don't affect the randomizer, are available in the client:
image

There is also an option in the client to disable cigarette imagery. This is done by patching the sprites with a "diff" sprite. The diff sprites just include the pixels that need to be covered by something new. They don't look like much by themselves.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Mar 30, 2024
@ScipioWright ScipioWright added the is: new game Pull requests for implementing new games into Archipelago. label Mar 30, 2024
Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some brief comments, looks pretty good as always.
Obviously there's those test failures too that need to be solved

worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/Items.py Outdated Show resolved Hide resolved
worlds/faxanadu/Locations.py Outdated Show resolved Hide resolved
worlds/faxanadu/Locations.py Outdated Show resolved Hide resolved
worlds/faxanadu/Options.py Outdated Show resolved Hide resolved
worlds/faxanadu/Regions.py Outdated Show resolved Hide resolved
@Daivuk Daivuk force-pushed the faxanadu branch 2 times, most recently from 69ab42a to 1e6513f Compare March 30, 2024 19:08
Copy link
Collaborator

@Silvris Silvris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to add Faxanadu to README.md as well.

worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/docs/en_Faxanadu.md Outdated Show resolved Hide resolved
worlds/faxanadu/Rules.py Outdated Show resolved Hide resolved
@Daivuk
Copy link
Collaborator Author

Daivuk commented Apr 7, 2024

Done the requested changes, and tested locally.

worlds/faxanadu/Options.py Outdated Show resolved Hide resolved
worlds/faxanadu/Options.py Outdated Show resolved Hide resolved
worlds/faxanadu/Options.py Outdated Show resolved Hide resolved
Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed all of the code and the relevant pages in local webhost and added relevant inline comments. All docs comments are typos or suggestions for more natural English sentences.
benchmarks ran ok, but there were some outliers in location access rules that could be optimized but weren't larger than .21 seconds, and load_worlds was great
no issues in running tests locally in 3.8 or 3.11

did not generate any games nor use the client

worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/Rules.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/docs/en_Faxanadu.md Outdated Show resolved Hide resolved
worlds/faxanadu/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/faxanadu/Options.py Outdated Show resolved Hide resolved
worlds/faxanadu/docs/setup_en.md Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
@Daivuk
Copy link
Collaborator Author

Daivuk commented Apr 14, 2024

Done the requested changes, and tested locally.

Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

barring my new small comment on returning red_potion_in_shop_count #3059 (comment)
and my discussion on RequireDragonSlayer's docstring everything looks good from my review
I'd still like those addressed but I would be happy as-is so approving

@Daivuk
Copy link
Collaborator Author

Daivuk commented Apr 14, 2024

barring my new small comment on returning red_potion_in_shop_count #3059 (comment) and my discussion on RequireDragonSlayer's docstring everything looks good from my review I'd still like those addressed but I would be happy as-is so approving

Oversights, fixed.

@qwint
Copy link
Contributor

qwint commented Apr 14, 2024

one more thing i didn't notice before, you should add the return typing hints for prefill_shop_red_potions() and prefill_shop_wingboots()

@Daivuk
Copy link
Collaborator Author

Daivuk commented Apr 14, 2024

one more thing i didn't notice before, you should add the return typing hints for prefill_shop_red_potions() and prefill_shop_wingboots()

Ok done

@qwint
Copy link
Contributor

qwint commented Apr 14, 2024

you put the hint on put_wingboot_in_shop() not prefill_shop_wingboots() which is the one who returns wingboots count

@Daivuk
Copy link
Collaborator Author

Daivuk commented Apr 14, 2024

you put the hint on put_wingboot_in_shop() not prefill_shop_wingboots() which is the one who returns wingboots count

I went too fast 😄 . Fixed

Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reapproving after the last couple comments were addressed as well
also, fyi the force-pushes make it harder to review just the new changes from review, but at least this world is small enough that it's not that bad

@Daivuk
Copy link
Collaborator Author

Daivuk commented Apr 14, 2024

reapproving after the last couple comments were addressed as well also, fyi the force-pushes make it harder to review just the new changes from review, but at least this world is small enough that it's not that bad

Ok I do this to keep the PR clean, as it's the first commit for this game. It's what we do at work to, force of habit. Squash/amend

@Daivuk
Copy link
Collaborator Author

Daivuk commented Apr 14, 2024

Before anyone merge, I have to do a commit there was a logic issue I had noted that I forgot to fix.

@Daivuk
Copy link
Collaborator Author

Daivuk commented Apr 14, 2024

Before anyone merge, I have to do a commit there was a logic issue I had noted that I forgot to fix.

Ok the logic fix is in

Copy link
Collaborator

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly a lot of minor suggestions/things. Looked through all the files and did 1000 fully random generations (100 single-player, nine 100-player) and there were no errors of any kind, though it did take quite a while at the end of progression fill for the 100-player seeds (everywhere from 1.5m to 19.9m). That said, numerous test generation with only 50 players all took less than 10 seconds (edit: I just had one take 3.5 minutes, so maybe there's just some restrictive cases these can run into), so something might be up with the scaling here. Looked through a local WebHost and all the links and documents seem good.

The big things that I would want clarification on are:

  • Whether defeating Evil One is meant to require all of all three progressive items, because logically it only expects one of each
  • Your events are done in a bit of a strange way, and could likely be separated out from the regular items to save a lot of time iterating over the lists to try and find them again and again. They should also be removed from the name_to_id lookups
  • Similarly, shop locations could have their own table so you don't have to iterate and find them over and over
  • I'm unsure why the shop/wingboot counter is so complex

worlds/faxanadu/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/faxanadu/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/faxanadu/docs/en_Faxanadu.md Outdated Show resolved Hide resolved
worlds/faxanadu/docs/en_Faxanadu.md Outdated Show resolved Hide resolved
worlds/faxanadu/Items.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
@Exempt-Medic
Copy link
Collaborator

@Exempt-Medic Exempt-Medic added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Jul 30, 2024
@Daivuk Daivuk force-pushed the faxanadu branch 5 times, most recently from e21916b to ee81c24 Compare September 1, 2024 12:09
@Daivuk
Copy link
Collaborator Author

Daivuk commented Sep 1, 2024

All requested changes are in

Copy link
Collaborator

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some final/additional comments

worlds/faxanadu/Rules.py Outdated Show resolved Hide resolved
worlds/faxanadu/Rules.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
worlds/faxanadu/__init__.py Outdated Show resolved Hide resolved
@Daivuk Daivuk force-pushed the faxanadu branch 2 times, most recently from 7909e04 to 40b441e Compare September 1, 2024 15:04
@Exempt-Medic Exempt-Medic removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Sep 1, 2024
Copy link
Collaborator

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comments were all addressed. I think there's some strange things going on with the shops and events here, but it's mostly fine, just a little over-engineered to me. I looked over all the code and did 1000 random solo generations, 100 10-player generations, 50 25-player generations. and 50 50-player generations. The time for a seed to finish generating was very consistent with single-player and 10-player generations but with the 25-player gens it was either relatively short (0.5 seconds) or quite long in comparison (about 20 seconds). For 50 players there was a similar but much more extreme case of generation either taking about four seconds or, well, still not being done one hour later (five of those are running now)

Edit: oops, missed that two of them finished. One took 107 seconds, the other 159. One of the remaining three has now finished and took 74 minutes. Another has finished after 86 minutes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: new game Pull requests for implementing new games into Archipelago. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants