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

Super Mario 64: ItemData class and tables #4321

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

Conversation

josephwhite
Copy link
Contributor

@josephwhite josephwhite commented Dec 2, 2024

What is this fixing or adding?

Adds an ItemData class to include ItemClassification in the item tables, similar to other worlds.

  • Simplifies create_item.
  • Utilize base id when assigning IDs.

How was this tested?

  • Several seed generations.
    • Item ID range
    • Move rando
  • Ran Webhost.py and generated a YAML (MoveRandomizerActions in Options.py).
  • Repo wide unit tests

@@ -1,47 +1,59 @@
from BaseClasses import Item
from typing import Dict, NamedTuple, Optional
Copy link
Member

Choose a reason for hiding this comment

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

Should be able to just use dict since 3.8/3.9 was dropped

@josephwhite josephwhite changed the title SM64: ItemData Super Mario 64: ItemData class and tables Dec 2, 2024
@josephwhite josephwhite marked this pull request as ready for review December 2, 2024 22:38
"Vanish Cap": 3626183,
"1Up Mushroom": 3626184
class SM64ItemData(NamedTuple):
code: Optional[int] = None
Copy link
Member

Choose a reason for hiding this comment

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

Can do this now too (and remove the Optional import)

Suggested change
code: Optional[int] = None
code: int | None = None

item_table = {name: data.code for name, data in item_data_table.items() if data.code is not None}
Copy link
Member

Choose a reason for hiding this comment

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

Might as well add the EoF newline now

Suggested change
item_table = {name: data.code for name, data in item_data_table.items() if data.code is not None}
item_table = {name: data.code for name, data in item_data_table.items() if data.code is not None}

@Exempt-Medic Exempt-Medic added is: enhancement Issues requesting new features or pull requests implementing new features. is: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Dec 3, 2024
@Exempt-Medic
Copy link
Member

It's probably fine to add them, but just letting you know that item_descriptions don't currently do anything

@josephwhite
Copy link
Contributor Author

It's probably fine to add them, but just letting you know that item_descriptions don't currently do anything

Did I mess up the implementation or did item descriptions brake between #2409/#2508 and now?
I tried to follow the World API and Dark Souls III implementation and didn't see any GitHub issues regarding it.

@Exempt-Medic
Copy link
Member

Did I mess up the implementation or did item descriptions brake between #2409/#2508 and now? I tried to follow the World API and Dark Souls III implementation and didn't see any GitHub issues regarding it.

When the options system was overhauled, their functionality was removed. DS3 still has them, they just don't do anything. I believe you added them correctly

Copy link
Collaborator

@N00byKing N00byKing left a comment

Choose a reason for hiding this comment

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

Sorry, but I don't think I'm in favor of this change. It seems to introduce a lot of redundancy for very little benefit, if at all.
Adding the item classification into the table just means that there's now another field that has to be filled in.
The existing solution with the if-else looks ugly, but to me preferable than giving the classification explicitly for every item.

@josephwhite
Copy link
Contributor Author

The existing solution with the if-else looks ugly, but to me preferable than giving the classification explicitly for every item.

Explicitly stating classification for each item doesn't have to be necessary if the same if-else logic is ported to the item tables where the default classification of items is progressive. Code has been updated to show this change if verbosity was the main downside of this change.

When the options system was overhauled, [item descriptions] functionality was removed. DS3 still has them, they just don't do anything. I believe you added them correctly.

💀. Removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features. is: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants