Wario Land 4: Implement new game

[lilDavid]

What is this fixing or adding?

Adds Wario Land 4 as an available game for AP.

How was this tested?

Many playtests and unsupported games over several APworld releases.

If this makes graphical changes, please attach screenshots.

[Exempt-Medic]

Some comments and things I noticed briefly. I did not look at the client or rom or any of the tests. Some things in setup_locations I haven't quite figured out yet and I will later go back through Rules.py again since it's doing some pretty strange things I need to wrap my head around.

[Exempt-Medic]

These are options now (same change should be applied elsewhere in this file)

Suggested change

	The [player settings page for this game](../player-settings) contains all the
	The [player options page for this game](../player-options) contains all the

[Exempt-Medic]

Usually "in" is used here

Suggested change

	be displayed at the bottom left corner of the screen. By default, you will have to carry the item to
	be displayed in the bottom left corner of the screen. By default, you will have to carry the item to

[Exempt-Medic]

I think this reads a bit better:

Suggested change

	the end of the level before the other player will receive it, at which point you will be told who
	the end of the level for the other player to receive it, at which point you will be told who

[Exempt-Medic]

This isn't a thing anymore

Suggested change

(select `Wario Land 4 Client` during installation).

[Exempt-Medic]

Settings -> Options

[Exempt-Medic]

These are few places with redundant parens, but this one struck me as especially unneeded

Suggested change

	if (treasure_hunt):
	if treasure_hunt:

[Exempt-Medic]

Multiworld.random should be avoided

Suggested change

	item_name = self.multiworld.random.choice(junk_item_pool)
	item_name = self.random.choice(junk_item_pool)

[Exempt-Medic]

I guess this was working, but it seems like .value makes more sense

Suggested change

	self.options.pool_jewels = PoolJewels(self.options.required_jewels)
	self.options.pool_jewels = PoolJewels(self.options.required_jewels.value)

[Exempt-Medic]

It looks like this error is entirely from user options, so two things.

1. This could be changed to an OptionError
2. This could be done much earlier, like in generate_early so that generation doesn't halt as late as create_items over this

[Exempt-Medic]

goal changes types here, so using a new variable is best

Suggested change

	goal = self.multiworld.get_location(goal, self.player)
	goal.place_locked_item(self.create_item('Escape the Pyramid'))
	goal.show_in_spoiler = False
	goal_loc = self.multiworld.get_location(goal, self.player)
	goal_loc.place_locked_item(self.create_item('Escape the Pyramid'))
	goal_loc.show_in_spoiler = False

[Exempt-Medic]

This test fails for me. This passes though:

Suggested change

	assert all(map(lambda p: p[1].type, pieces))
	assert all(map(lambda p: p[1].type == ItemType.JEWEL, pieces))

[Exempt-Medic]

Some more comments. One general question I have is why this code is using Sequence and Mapping so much instead of List and Dict?

[Exempt-Medic]

An extra space

Suggested change

	'Traps': { 'Wario Form Trap', 'Lightning Trap'},
	'Traps': {'Wario Form Trap', 'Lightning Trap'},

[Exempt-Medic]

So here checks changes from a filter with tuples to a set of strings. Could do this instead:

Suggested change

	checks = filter(lambda p: self.options.difficulty in p[1].difficulties, location_table.items())
	if not self.options.goal.needs_treasure_hunt():
	checks = filter(lambda p: p[1].source != LocationType.CHEST, checks)
	checks = {name for name, _ in checks}
	if self.options.goal.needs_diva():
	checks.remove('Sound Room - Emergency Exit')
	else:
	checks.remove('Golden Diva')
	return checks
	checks = filter(lambda p: self.options.difficulty in p[1].difficulties, location_table.items())
	if not self.options.goal.needs_treasure_hunt():
	checks = filter(lambda p: p[1].source != LocationType.CHEST, checks)
	check_names = {name for name, _ in checks}
	if self.options.goal.needs_diva():
	check_names.remove('Sound Room - Emergency Exit')
	else:
	check_names.remove('Golden Diva')
	return check_names

[Exempt-Medic]

You could use a comprehension instead:

Suggested change

	for _ in range(junk_count):
	item_name = self.multiworld.random.choice(junk_item_pool)
	itempool.append(self.create_item(item_name))
	itempool.extend([self.create_item(self.random.choice(junk_item_pool)) for _ in range(junk_count)])

[Exempt-Medic]

It seems a little odd to me that you're filtering these for each world instead of creating some lists once on the class that they can all share and only have to be created once.

[Exempt-Medic]

Can use the helper function

Suggested change

	location = world.multiworld.get_location(name, world.player)
	location = world.get_location(name)

[Exempt-Medic]

Is there a way for this error to be hit? Seems more debug-y to me, so you could use an assert instead

Suggested change

	except KeyError as k:
	# Raise for invalid location names, not ones that aren't in this player's world
	if name not in locations.location_table:
	raise ValueError(f'Location {name} does not exist') from k
	except KeyError:
	# Raise for invalid location names, not ones that aren't in this player's world
	assert name in locations.location_table, f"Location {name} does not exist"

[Exempt-Medic]

The way you're doing this, these are all ints
You can also remove the Options import with this change

Suggested change

	def option(option_name: str, choice: Option):
	return Requirement(lambda w, _: getattr(w.options, option_name) == choice)
	def difficulty(difficulty: options.Difficulty):
	return option('difficulty', difficulty)
	def not_difficulty(_difficulty: options.Difficulty):
	return Requirement(lambda w, s: not difficulty(_difficulty).inner(w, s))
	def logic(logic: options.Logic):
	return option('logic', logic)
	def option(option_name: str, choice: int):
	return Requirement(lambda w, _: getattr(w.options, option_name) == choice)
	def difficulty(difficulty: int):
	return option('difficulty', difficulty)
	def not_difficulty(_difficulty: int):
	return Requirement(lambda w, s: not difficulty(_difficulty).inner(w, s))
	def logic(logic: int):
	return option('logic', logic)

[Exempt-Medic]

make_boss_access_rule passes in a tuple of strings and ints so this function is used with both types of RequiredItem

Suggested change

	def has_all(items: Sequence[str]) -> Requirement:
	def has_all(items: Sequence[RequiredItem]) -> Requirement:

[Exempt-Medic]

Because has_all can thus call this with a tuple of strings and ints, this also handles all RequiredItem types

Suggested change

	def has(item_name: str) -> Requirement:
	def has(item_name: RequiredItem) -> Requirement:

[Exempt-Medic]

So because this accepts tuples of ints, you're calling helpers.get with a tuple of ints which isn't exactly supported here (it works, but it's kind of taking advantage of this not causing an error). You could do this instead:

def resolve_helper(item_name: RequiredItem):
    if not isinstance(item_name, str):
        return item_name
    item_name = helpers.get(item_name, item_name)
    if not isinstance(item_name, str):
        return item_name
    return item_name, 1

Another option that I think is quite a bit cleaner is changing helpers to a Mapping[str, Tuple[str, int]] so you'd change 'Progressive Ground Pound' to ('Progressive Ground Pound', 1) and then just having this instead:

def resolve_helper(item_name: RequiredItem):
    if not isinstance(item_name, str):
        return item_name
    return helpers.get(item_name, (item_name, 1))

[Exempt-Medic]

Something else I forgot to mention, you may want to implement a get_filler_item_name function, so that when the multiworld has to ask for items from your world (like with plando or itemlinks) it will only pull from filler items you define instead of all items, including progression ones. Another thing you could consider is adding start_inventory_from_pool support, it's a pretty simple options change and you wouldn't have to rearrange anything to support it (it would also benefit from get_filler_item_name).

[lilDavid]

Some more comments. One general question I have is why this code is using Sequence and Mapping so much instead of List and Dict?

I like to use Sequence and Mapping because they give more of a hint that the value won't be modified. I use tuples as immutable sequences a lot, and Sequence covers both tuples and lists.