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

New Game Implementation: Civilization VI #3736

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

Conversation

hesto2
Copy link

@hesto2 hesto2 commented Aug 6, 2024

What is this fixing or adding?

Support for Civilization VI as a new world.

How was this tested?

The world has several test suites as well as running it through alpha & beta with the community since May

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Aug 6, 2024
@BadMagic100 BadMagic100 added the is: new game Pull requests for implementing new games into Archipelago. label Aug 6, 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.

These are mainly suggestions, feel free to agree, disagree, or discuss any of them with me as desired.

worlds/civ_6/Items.py Outdated Show resolved Hide resolved
worlds/civ_6/__init__.py Outdated Show resolved Hide resolved
worlds/civ_6/__init__.py Outdated Show resolved Hide resolved
worlds/civ_6/__init__.py Outdated Show resolved Hide resolved
worlds/civ_6/__init__.py Outdated Show resolved Hide resolved
worlds/civ_6/Rules.py Outdated Show resolved Hide resolved
worlds/civ_6/Options.py Outdated Show resolved Hide resolved
worlds/civ_6/Options.py Outdated Show resolved Hide resolved
worlds/civ_6/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/civ_6/docs/setup_en.md Outdated Show resolved Hide resolved
@hesto2
Copy link
Author

hesto2 commented Aug 6, 2024

@ScipioWright thank you for the review 🙏

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 a few other things while I have a moment

worlds/civ_6/__init__.py Outdated Show resolved Hide resolved
worlds/civ_6/__init__.py Outdated Show resolved Hide resolved
worlds/civ_6/Rules.py Outdated Show resolved Hide resolved
worlds/civ_6/Civ6Client.py Outdated Show resolved Hide resolved
worlds/civ_6/Items.py Show resolved Hide resolved
worlds/civ_6/Locations.py Outdated Show resolved Hide resolved
worlds/civ_6/Options.py Outdated Show resolved Hide resolved
worlds/civ_6/__init__.py Outdated Show resolved Hide resolved
worlds/civ_6/__init__.py Outdated Show resolved Hide resolved
@NewSoupVi
Copy link
Member

NewSoupVi commented Nov 30, 2024

World load times have become a serious issue for AP. Every generation has to load the worlds, and right now, it has to load all worlds (we don't have lazy / selective world loading yet)

As a result, we're no longer as willing to merge games that store their data in this many json files and use file-io to load them. These files cannot be optimised / compiled down for the .exe build and have to be read from inside zip files (.apworld) and parsed as json every time.

Your json data doesn't look very layered, so I think these could just be rewritten as Python files fully.
If you want to keep the json files, there are several solutions, but they all come down to the same thing: Generate .py files from the json files. Usually people write a script for this that they include in the world, or they use something like pickle.

@hesto2
Copy link
Author

hesto2 commented Nov 30, 2024 via email

@hesto2
Copy link
Author

hesto2 commented Dec 1, 2024

@NewSoupVi I've updated all the data files to be .py files. Let me know if the way I went about it solves the issue 👍

@NewSoupVi
Copy link
Member

It'll probably be until the weekend until I can look at New Game implementations again, but yeah it seems correct! :) Thank you

pre_hint_items: PreHintItems
hide_item_names: HideItemNames
advisor_show_progression_items: InGameFlagProgressionItems
death_link: DeathLink
Copy link
Member

Choose a reason for hiding this comment

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

You may want to create a DeathLink subclass to overwrite the docstring, as I suspect many will ask how deathlink works in this game otherwise. An example can be found in Subnautica's options.

for location in locations.values():
self.location_table[location.name] = location

def get_filler_item_name(self) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

This method will be very slow if a lot of filler needs to be created, since the items are filtered for each call with no caching.

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.

7 participants