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

Core, all worlds: Hard-deprecate old options API (by August 10th 2024) #3284

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

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented May 10, 2024

world.options_definitions and accessing option values through world.multiworld.option_name[player] was deprecated in #993.

We are now looking to hard deprecate it.

Here is every world in source, and whether it is confirmed to have fully moved over:

  • Core
  • Adventure
  • ALTTP - LTTP: sort of use new options system #3764
  • ArchipIDLE
  • BK Sudoku
  • Blasphemous
  • Bumper Stickers
  • Celeste 64
  • ChecksFinder
  • Clique
  • CV64
  • DarkSouls3
  • DKC3
  • DLCQuest
  • Doom
  • Doom II
  • Factorio
  • FF1
  • FFMQ
  • Heretic
  • HK
  • Hylics 2
  • KDL3
  • KH2
  • LADX
  • Landstalker
  • Lingo
  • Lufia 2
  • Meritous
  • Messenger
  • Minecraft
  • MLSS
  • MMBN3
  • Muse Dash
  • Noita
  • OoT - Ocarina of Time: options and general cleanup #3767
  • Overcooked 2
  • Pokemon Emerald
  • Pokemon RB
  • Raft
  • Rogue Legacy
  • RoR2
  • SA2B
  • SC2
  • Shivers
  • A Short Hike
  • SM
  • SM64
  • SMW
  • SMZ3
  • SoE
  • Slay the Spire
  • Stardew Valley
  • Subnautica
  • Terraria
  • Timespinner
  • TLOZ
  • TUNIC
  • Undertale
  • V6
  • Wargroove
  • Witness
  • Yoshi's Island
  • Zillion
  • Zork - Grand Inquisitor

The reason some worlds are "unconfirmed" is that it's not a trivial thing, some worlds might be using the new options API mostly, but using the old one in one instance at runtime. Also, some worlds might call their multiworld reference "world" or "mw". I did my best, please forgive any shortcomings of this initial list.

Also, there are generic features such as starting_inventory which are part of the options api, while others, like local_early_items, are not. The ones that are part of options and have to be accessed through the new API can be found in the PerGameCommonOptions class:

@dataclass
class PerGameCommonOptions(CommonOptions):
    local_items: LocalItems
    non_local_items: NonLocalItems
    start_inventory: StartInventory
    start_hints: StartHints
    start_location_hints: StartLocationHints
    exclude_locations: ExcludeLocations
    priority_locations: PriorityLocations
    item_links: ItemLinks

If your game is not checked off on this list, you will be given three months (Until the 10th of August, 2024) to do one of the following:

  1. Confirm that your world is already fully moved over
  2. Open a PR to move it to the new options API (it does not need to be merged by the deadline, just opened)

If neither of these are met by the deadline, core maintainers are entitled to go in and fix it themselves, or move your world to worlds_disabled if they can't figure out how to do it. You also might lose some status as a "responsive world maintainer", so please do not rely on core maintainers going in and doing it for you.

The new API is detailed here: https://github.com/ArchipelagoMW/Archipelago/blob/main/docs/options%20api.md

If you have any questions, people will be happy to help you in the designated channels, such as #ap-world-dev in the Discord server!

World maintainers of worlds that are failing or unconfirmed:
@JusticePS @Berserker66 @TRPG0 @SunCatMC @ThePhar @LiquidCat64 @Marechal-L @axe-y @agilbert1412 @jtoyoda @Alchav @wildham0 @Daivuk @BadMagic100 @Silvris @JaredWeakStrike @zig-for @KonoTyran @espeon65536 @SunnyBat @PoryGone @RaspberrySpace @GodlFire @lordlou @Seldom-SE @Jarno458 @silent-destroyer @ScipioWright @NewSoupVi @jonloveslegos @FlySniper
If all of your worlds already are marked off, congratulations, your world passed the check independently since I created this PR thanks to the work of some lovely people~

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels May 10, 2024
@NewSoupVi NewSoupVi added the waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. label May 10, 2024
@NewSoupVi NewSoupVi changed the title Core, all worlds: Hard-depricate old options API Core, all worlds: Hard-deprecate old options API May 10, 2024
@NewSoupVi NewSoupVi changed the title Core, all worlds: Hard-deprecate old options API Core, all worlds: Hard-deprecate old options API (by August 10th 2024) May 10, 2024
@PoryGone
Copy link
Collaborator

SMW should be done as of this PR opened in January and merged in March:
#2762

SA2B is slated for resolution with our next content update, which does not yet have a release window.

@PoryGone
Copy link
Collaborator

Also, did you cross-reference with the output provided by this:

logging.warning(f"{name} Assigned options through option_definitions which is now deprecated. "

@ScipioWright
Copy link
Collaborator

TUNIC is good to go

@NewSoupVi
Copy link
Member Author

NewSoupVi commented May 10, 2024

SMW should be done as of this PR opened in January and merged in March: #2762

My bad, I was on something outdated :) Will mark SMW as resolved

@NewSoupVi
Copy link
Member Author

NewSoupVi commented May 10, 2024

Also, did you cross-reference with the output provided by this:

logging.warning(f"{name} Assigned options through option_definitions which is now deprecated. "

I did, yes, that's how I confirmed some worlds were bad immediately, before going in and checking whether any worlds offend in different ways at runtime

@NewSoupVi
Copy link
Member Author

TUNIC is good to go

Alrighty :)

@NewSoupVi
Copy link
Member Author

NewSoupVi commented May 10, 2024

SA2B is slated for resolution with our next content update, which does not yet have a release window.

As for SA2B, I will accept that for now, but if it takes so long that we're past the deadline and SA2B is the last world on old options, I will ask for something to be done about it ahead of the content update. Hopefully not tho

@BadMagic100
Copy link
Collaborator

Just mentioning because I was pinged here, the world side of HK is not my problem. This is a phar issue, I have no intent of trying to figure out how to programmatically generate a dataclass (this change is like actively bad for HK, fwiw)

@BadMagic100
Copy link
Collaborator

From a code perspective there at least one block of code somewhere with a "todo: remove this when the old way is deprecated" which I don't see removed here.

@NewSoupVi
Copy link
Member Author

NewSoupVi commented May 10, 2024

From a code perspective there at least one block of code somewhere with a "todo: remove this when the old way is deprecated" which I don't see removed here.

That's fair, this is a copy of #3284 which as I understand didn't actually aim to remove this functionality & wanted to preserve it working on the WebHost

I'm more here to lead the charge in getting all the worlds changed - @alwaysintreble can I involve you in this to think about the code side of what a hard removal with all worlds being forced to switch over would look like?
Maybe also some of the other core maintainers reading this, because there is the question of "should we completely break old unmaintained unsupported .apworlds with no way of getting them running anymore unless you go in and change the code" - Like, is that something we care about, to the point where we make some sort of arg like --allow-old-options or something

(Would prefer to have that discussion in Discord tho because I imagine this PR is gonna see a lot of activity already)

@Ziktofel
Copy link
Collaborator

SC2 already migrated with 0.4.5

@Jarno458
Copy link
Collaborator

Timespinner is worked on in #2485

@alwaysintreble
Copy link
Collaborator

alwaysintreble commented May 10, 2024

From a code perspective there at least one block of code somewhere with a "todo: remove this when the old way is deprecated" which I don't see removed here.

The old way is not removed, just hard deprecated rather than soft deprecated. This means it won't work on source but will still function for apworlds used with the 0.4.7 (if this gets merged before then) release.

@Exempt-Medic Exempt-Medic added the is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. label May 10, 2024
@GodlFire
Copy link
Collaborator

Shivers removal of last two instances of using old options API is fixed in this PR: #3287

@jtoyoda
Copy link
Collaborator

jtoyoda commented May 14, 2024

I will not be able to take a look at this and would recommend
A. Offering another developer to become the lead developer of FF1 as I have not been able to look at it
B. Deprecating FF1 (I am not sure how popular it is anyways)

Thanks for the continued improvements.

@JusticePS
Copy link
Collaborator

I should have time to get a PR put together for Adventure by sometime next weekend.

@FlySniper
Copy link
Collaborator

Wargroove has a PR for the new options system: #3306

@JusticePS
Copy link
Collaborator

Adventure PR #3326 to remove the deprecated options

@PoryGone
Copy link
Collaborator

#3357 Resolves SA2B. I'd forgotten that the Option Accessing was already handled with v2.3, and only migrating to the Dataclass was still required.

@LegendaryLinux
Copy link
Member

LegendaryLinux commented May 21, 2024

#3357 is merged.

ThePhar added a commit to ThePhar/Archipelago that referenced this pull request May 25, 2024
@ThePhar
Copy link
Member

ThePhar commented May 25, 2024

For Rogue Legacy: I believe I will wait for MultiClient.Net update to finalize and then do it in combination of that refactor to support #1933.

For Clique: I will update it this weekend.

@lordlou
Copy link
Contributor

lordlou commented May 26, 2024

#3372 resolves SM and SMZ3.

@Seldom-SE
Copy link
Collaborator

#3414 resolves Terraria

@BadMagic100
Copy link
Collaborator

#3428 will resolve HK once merged

@jonloveslegos
Copy link
Contributor

#3528 will resolve Undertale when merged.

@NewSoupVi
Copy link
Member Author

NewSoupVi commented Jun 20, 2024

As per https://discord.com/channels/731205301247803413/1214608557077700720/1253206955879694336, it is unclear whether Espeon will be able to go into OoT themselves by the deadline. Help on OoT is wanted as of today

@NewSoupVi NewSoupVi added the meta: help wanted Additional review/assistance is requested for these issues or pull requests. label Jun 20, 2024
@nicholassaylor
Copy link
Contributor

Subnautica had swim_rule fixed in #3685

@TRPG0
Copy link
Collaborator

TRPG0 commented Aug 4, 2024

Blasphemous is now ready for review #3355

@NewSoupVi
Copy link
Member Author

NewSoupVi commented Aug 9, 2024

August 10th is coming up.

As far as I'm aware, the following worlds are not fixed & do not have a PR open to fix them:

Everything is being worked on now. Let's go!!! (Edit)

@NewSoupVi
Copy link
Member Author

NewSoupVi commented Aug 9, 2024

Thank you everyone so much for the work you've done so far :) This was a humongous team effort, and I especially appreciate the efforts of those who made PRs for worlds that they don't maintain.

We are looking for help with Ocarina of Time. We have to start considering disabling it, but it is too important a game for AP for us to want to do that. Willing to trade for "priority PR reviews" for your own world, if you need an incentive.

OoT is being worked on!

@NewSoupVi NewSoupVi added waiting-on: other Issue/PR is waiting for something else, like another PR. and removed waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. meta: help wanted Additional review/assistance is requested for these issues or pull requests. labels Aug 20, 2024
@Jarno458
Copy link
Collaborator

Seems OOT is merged so only ALttP left?

@Exempt-Medic
Copy link
Member

Seems OOT is merged so only ALttP left?

And then anything and everything that got missed, yeah

@NewSoupVi
Copy link
Member Author

Seems OOT is merged so only ALttP left?

Yup

I wanna merge this (and Hint Priority) at the start of next version basically

@NewSoupVi
Copy link
Member Author

We will merge this (after merging #3764, and seeing if unit tests pass) at the start of the next version.

@NewSoupVi NewSoupVi removed the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Sep 22, 2024
@NewSoupVi
Copy link
Member Author

It's looking like the more complex rework of ALTTP will be required. Oh well, we'll get there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. waiting-on: other Issue/PR is waiting for something else, like another PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.