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

Yu-Gi-oh! 2006: implement new game #2795

Merged
merged 153 commits into from
May 17, 2024
Merged

Conversation

Rensen3
Copy link
Contributor

@Rensen3 Rensen3 commented Feb 4, 2024

What is this fixing or adding?

Adds Yu-Gi-Oh! Ultimate Masters: World Championship Tournament 2006 for the Game Boy Advance as a supported Archipelago game.
The multiworld for this runs on BizHawkClient.

How was this tested?

Very extensive playtesting by me, as well as from people on the discord, when I release an update.

If this makes graphical changes, please attach screenshots.

No graphical changes.

Rensen3 and others added 30 commits April 10, 2023 16:59
…into yugioh06

� Conflicts:
�	worlds/yugioh06/Items.py
�	worlds/yugioh06/Locations.py
�	worlds/yugioh06/Options.py
�	worlds/yugioh06/__init__.py
Bad commit organization a consequence of working with two different branches and not keeping the commits separated
… 5 campaign opponents. Added logic for TD16 Union.
@ScipioWright ScipioWright added the waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. label Apr 21, 2024
Co-authored-by: Scipio Wright <scipiowright@gmail.com>
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Code review

I had a lot to say, but that is not to say I think this implementation is bad, I think it's in a good state overall. But, these issues should be addressed nonetheless.

If I marked a review comment with "Low Priority" (or any other italicized line at the end), then you can resolve it if you don't wanna change it. Please still read and consider these comments.

@@ -0,0 +1,136 @@
import logging
import math
import sys
Copy link
Member

@NewSoupVi NewSoupVi May 9, 2024

Choose a reason for hiding this comment

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

This import is unused.

In general, this world has quite a few code style violations. I could list them all, but as always, I will recommend the "quick fix".

  1. python -m pip install ruff
  2. Put a file called ruff.toml in your world folder, with the following contents (already adjusted for Yu Gi Oh):
line-length = 120

[lint]
preview = true
select = ["E", "F", "W", "I", "N", "Q", "UP", "RUF", "ISC", "T20"]
ignore = ["RUF012", "RUF100"]

[per-file-ignores]
# The way options definitions work right now, world devs are forced to break line length requirements.
"options.py" = ["E501"]
# Yu Gi Oh specific: The structure of the Opponents.py file makes the line length violations acceptable.
"Opponents.py" = ["E501"]
  1. Run python -m ruff --fix worlds/yugioh06
  2. Optional but highly recommended: Run python -m ruff format worlds/yugioh06 (which will more aggressively reformat your code, but auto-fix more errors for you in the process. Your mileage may vary, I would at least run it once and see what it does, and then revert changes you don't like)
  3. Fix remaining errors that ruff outputs to the console manually

If you wanna do it yourself instead, the things I definitely want to see fixed are:

  • In AP, we use double quotes " for strings, never single quotes '.
  • Import blocks are messy and have unused imports
  • Many of the line length violations are warranted (I exempted those files in the ruff.toml I provided above), but a couple of them aren't
  • I would like to see a consistent-per-case and PEP8 compliant strategy for multi-line statements

Sorry that you got the core maintainer who is anal about code style :) I wouldn't be as anal about it if I didn't have a quick fix handy.

worlds/yugioh06/BoosterPacks.py Outdated Show resolved Hide resolved
worlds/yugioh06/BoosterPacks.py Outdated Show resolved Hide resolved
worlds/yugioh06/BoosterPacks.py Outdated Show resolved Hide resolved
worlds/yugioh06/Fusions.py Outdated Show resolved Hide resolved
worlds/yugioh06/__init__.py Outdated Show resolved Hide resolved
worlds/yugioh06/__init__.py Outdated Show resolved Hide resolved
worlds/yugioh06/logic.py Outdated Show resolved Hide resolved
worlds/yugioh06/logic.py Outdated Show resolved Hide resolved
worlds/yugioh06/utils.py Outdated Show resolved Hide resolved
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Integration test ❌

❌ This apworld does not load on Python 3.8, which we are still supporting. This needs to be rectified before merge.

Generations ✅

✅ 200 generations with a fully random yaml succeeded without errors.
✅ 100 generations with 5 fully random yamls succeeded without errors.

Generation time ⚠️

The generation time of Yu Gi Oh seeds (using the --skip_output flag to skip rom patching) is kind of unexpectedly high. On Python 3.11, generation of base settings (84 items) takes 0.6s. When going for random settings, I saw many generations go over 1s, all the way up to 1.5s or so.

Some comparison points, with all of these games having more than 84 locations:

  • SA2 Battle: 0.05s
  • Lingo base settings: 0.06s
  • Tunic with Ability and Ladder shuffle: 0.15s
  • Factorio base settings: 0.16s
  • Pokemon Emerald base settings: 0.4s
  • The Witness symbol shuffle + mixed doors 0.5s
  • Lingo with complex door shuffle: 0.5s
  • Pokemon Emerald with a lot of settings on: 0.6s
  • The Witness symbol shuffle + mixed doors + obelisk sides + obelisk keys: 1.0s
  • Ocarina of Time base settings: 1.5s (one of the slowest randos we have due to being set up as a wrapper of a different python project)

I'm not exactly sure why, I didn't see any bad patterns. There are a couple of explanations:

  • This game just has a lot of its items be considered progression, which might make sense if it's a game with "strength logic".
  • use of has_group, which can be slow, but was also the only option when this world was written. (Could use has_list now, but that would break apworld compatibility until the next version)
  • High use of event locations/items. Events are very slow compared to alternatives in a lot of cases. I can't say for sure whether they make sense here without deep diving into the game. The rule of thumb is: Unless they actually simplify logic, it is usually better to avoid them. (This is my personal most likely theory)

The access rule lambdas can probably be optimised as well, but this is also non-trivial. Here is an example of how something like that might work: Does every opponent entrance need to check all three of state.has(item, self.player, amount) and state.yugioh06_difficulty(self.player, opp.difficulty) and state.has_all(opp.additional_info, self.player), or are some of these always True depending on the opponent? If yes, could we have a function that smartly checks which things need to be applied and builds the lambda differently depending on that?

There's nothing simple I can recommend here and it's not so heinously slow that it'd be concerning, and again, the nature of the game might just be quite complicated, I wouldn't know, I've never played it. Rest assured that this world doesn't do anything obviously slow. But yeah, I just felt it was worth pointing out.

@Rensen3
Copy link
Contributor Author

Rensen3 commented May 11, 2024

I think I got everything now.

This game only has 9 non-progression items and uses a ton of events. All the important cards and beating every opponent/challenge are events.

From my testing having the goal set to challenges and having a lot of them slows down the generation time the most. Which makes sense since challenges check for a lot of cards, some for over 20. Fortunately this setting isn't very popular and only async viable.

I'm not sure how I could fix the high generation time.

@NewSoupVi
Copy link
Member

NewSoupVi commented May 11, 2024

I think I got everything now.

This game only has 9 non-progression items and uses a ton of events. All the important cards and beating every opponent/challenge are events.

From my testing having the goal set to challenges and having a lot of them slows down the generation time the most. Which makes sense since challenges check for a lot of cards, some for over 20. Fortunately this setting isn't very popular and only async viable.

I'm not sure how I could fix the high generation time.

I think the only thing I would investigate is if there is an alternative to the high event usage, but yeah, that sounds reasonable to me. My game generates slowly too if I min-max progression item to location count. I'll do a rereview soon

Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Did a very quick rereview of some of the more important parts

Seems like generation time has slightly improved actually! So that's nice :D

ruff changed the import order and that's now leading to circular imports in __init__.py - I should've warned about that

And I think my comment about open_file and pkgutil was misunderstood, although after looking at it, I question the need for that entire function in the first place

worlds/yugioh06/__init__.py Outdated Show resolved Hide resolved
worlds/yugioh06/utils.py Outdated Show resolved Hide resolved
Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Alright, did a proper rereview now.

Some small things that have come up since the first review.

I still think some of the formatting choices are a liiiiittle ugly but I'm not gonna hold this up over that identifinitely.

Still can't comment on any logic or playability. I would like to quickly hear if this has been played again since the last round of changes, since the changes were pretty major. :)

After that, I'd be happy to merge it

worlds/yugioh06/rules.py Outdated Show resolved Hide resolved
worlds/yugioh06/rules.py Outdated Show resolved Hide resolved
worlds/yugioh06/rules.py Outdated Show resolved Hide resolved
worlds/yugioh06/logic.py Outdated Show resolved Hide resolved
worlds/yugioh06/opponents.py Outdated Show resolved Hide resolved
worlds/yugioh06/opponents.py Outdated Show resolved Hide resolved
worlds/yugioh06/opponents.py Outdated Show resolved Hide resolved
worlds/yugioh06/rom.py Outdated Show resolved Hide resolved
@Rensen3
Copy link
Contributor Author

Rensen3 commented May 17, 2024

I played a few games with a lot of item-links and generated some games to check the spoiler-log to see if anything is off.

Should have implemented all of the suggestion.

@NewSoupVi NewSoupVi merged commit 539ee1c into ArchipelagoMW:main May 17, 2024
3 checks passed
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: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants