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

Kingdom Hearts: Implement New Game #3201

Open
wants to merge 339 commits into
base: main
Choose a base branch
from

Conversation

gaithern
Copy link

@gaithern gaithern commented Apr 23, 2024

What is this fixing or adding?

Adds Kingdom Hearts (EGS and Steam PC) as a playable game in AP

How was this tested?

Many test runs in unsupported games, both sync and async.

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.

Pretty simple and nice to review overall, seems to be working as well

I have many nitpicks and some genuine issues

As usual, any comments where the last line is "low priority" or some other note in italics, I'm okay if you just resolve them and don't change anything. (Especially "comment", where there's no suggested change)

On the other ones, I'd at least like a reply on why you don't think it's a good idea if you end up rejecting it

worlds/kh1/Rules.py Outdated Show resolved Hide resolved
worlds/kh1/Rules.py Outdated Show resolved Hide resolved
worlds/kh1/Rules.py Outdated Show resolved Hide resolved
worlds/kh1/Rules.py Outdated Show resolved Hide resolved
worlds/kh1/Rules.py Outdated Show resolved Hide resolved
worlds/kh1/__init__.py Outdated Show resolved Hide resolved
worlds/kh1/__init__.py Outdated Show resolved Hide resolved
worlds/kh1/__init__.py Outdated Show resolved Hide resolved
worlds/kh1/__init__.py Show resolved Hide resolved
worlds/kh1/test/test_goal.py 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.

Did a quick rereview - Couple of things still stuck out to me, but overall I think it's greatly improved now :)

worlds/kh1/Rules.py Show resolved Hide resolved
worlds/kh1/__init__.py Outdated Show resolved Hide resolved
worlds/kh1/Rules.py Outdated Show resolved Hide resolved
worlds/kh1/__init__.py Outdated Show resolved Hide resolved
@NewSoupVi
Copy link
Member

NewSoupVi commented Aug 18, 2024

Oh, you're also failing unit tests now, apparently there is a bug in your create_items? Might wanna do a test gen and see if it crashes

@gaithern
Copy link
Author

Oh, you're also failing unit tests now, apparently there is a bug in your create_items? Might wanna do a test gen and see if it crashes

Found where this was happening, had to do with a change above involving the level_up_item_pool, fixed the bug and gave it a clearer variable name.

@NewSoupVi
Copy link
Member

Thank you for your patience & cooperation so far, we're getting close I promise :) Gonna do a rereview now, with any luck it's the last

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