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.

Sign up for GitHub

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

Generate: remove tag "-" #3036

Merged
merged 7 commits into from
May 23, 2024
Merged

Generate: remove tag "-" #3036

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7678cb0
Core: remove now unused stuff in Generate.py
Berserker66 Mar 26, 2024
b8f78cb
Generate: introduce Remove, similar to Merge
Berserker66 Mar 26, 2024
0b13991
Generate: might be helpful to use the correct function
Berserker66 Mar 26, 2024
53532e4
make + dict behave as + for each value
Berserker66 May 23, 2024
58400c6
Merge branch 'main' into generate_remove
Berserker66 May 23, 2024
a5fd645
fix unittest
Berserker66 May 23, 2024
f1a7d84
Minor clarification to docs and add reference to `-` method.
ThePhar May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions Generate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import urllib.request
from collections import Counter
from typing import Any, Dict, Tuple, Union
from itertools import chain

import ModuleUpdate

Expand All @@ -21,7 +22,6 @@
from Main import main as ERmain
from settings import get_settings
from Utils import parse_yamls, version_tuple, __version__, tuplize_version
from worlds.alttp import Options as LttPOptions
from worlds.alttp.EntranceRandomizer import parse_arguments
from worlds.alttp.Text import TextTable
from worlds.AutoWorld import AutoWorldRegister
Expand Down Expand Up @@ -310,13 +310,6 @@ def handle_name(name: str, player: int, name_counter: Counter):
return new_name


def prefer_int(input_data: str) -> Union[str, int]:
try:
return int(input_data)
except:
return input_data


def roll_percentage(percentage: Union[int, float]) -> bool:
"""Roll a percentage chance.
percentage is expected to be in range [0, 100]"""
Expand All @@ -327,7 +320,7 @@ def update_weights(weights: dict, new_weights: dict, update_type: str, name: str
logging.debug(f'Applying {new_weights}')
cleaned_weights = {}
for option in new_weights:
option_name = option.lstrip("+")
option_name = option.lstrip("+-")
if option.startswith("+") and option_name in weights:
cleaned_value = weights[option_name]
new_value = new_weights[option]
Expand All @@ -339,6 +332,20 @@ def update_weights(weights: dict, new_weights: dict, update_type: str, name: str
raise Exception(f"Cannot apply merge to non-dict, set, or list type {option_name},"
f" received {type(new_value).__name__}.")
cleaned_weights[option_name] = cleaned_value
elif option.startswith("-") and option_name in weights:
Berserker66 marked this conversation as resolved.
Show resolved Hide resolved
cleaned_value = weights[option_name]
new_value = new_weights[option]
if isinstance(new_value, set):
cleaned_value.difference_update(new_value)
elif isinstance(new_value, list):
for element in new_value:
cleaned_value.remove(element)
Copy link
Contributor

Choose a reason for hiding this comment

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

.remove() only seems to remove the first occurrence; do we want to remove all?

>>> a = ['a', 'a', 'b']
>>> a.remove('a')
>>> a
['a', 'b']

I would think that for my use-cases, we should remove all occurrences (just because multiple triggers with overlapping effects are more likely to stack the same item in the list multiple times if it doesn't cause a difference). This means that lists that formerly didn't have a semantic meaning behind duplicates can now behave differently under triggers if they have duplicate entries. While I think it's possible for a yaml writer to make use of that behaviour somehow, I think that's well into dark yaml territory and I'd imagine removing all would be the more intuitive behaviour. Then again, I haven't played a game where duplicate list entries had any semantic meaning, they might be out there.

Whichever behaviour we go with (remove first or remove all), it should be documented somewhere because the question will either be asked or it will be an unknown effect that will torment yaml writers getting too fancy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since writing this I've dived more into options and learned the options API supplies both OptionSet and OptionList. Given .removed() should always remove all occurrences in a set (because there can only be one occurrence), I think the behaviour of only removing the first element in a list is more reasonable.

However, lists and sets are written the same way in the yaml, so yaml writers may have no indication which structure they are working with. Behaving differently for different structures introduces some sharp corners unless the world maintainers are very good about communicating what option is what.

The final behaviour should still be documented somewhere.

elif isinstance(new_value, dict):
cleaned_value = dict(Counter(cleaned_value) - Counter(new_value))
else:
raise Exception(f"Cannot apply remove to non-dict, set, or list type {option_name},"
f" received {type(new_value).__name__}.")
cleaned_weights[option_name] = cleaned_value
else:
cleaned_weights[option_name] = new_weights[option]
new_options = set(cleaned_weights) - set(weights)
Expand Down Expand Up @@ -468,9 +475,11 @@ def roll_settings(weights: dict, plando_options: PlandoOptions = PlandoOptions.b
world_type = AutoWorldRegister.world_types[ret.game]
game_weights = weights[ret.game]

if any(weight.startswith("+") for weight in game_weights) or \
any(weight.startswith("+") for weight in weights):
raise Exception(f"Merge tag cannot be used outside of trigger contexts.")
for weight in chain(game_weights, weights):
if weight.startswith("+"):
raise Exception(f"Merge tag cannot be used outside of trigger contexts. Found {weight}")
if weight.startswith("-"):
raise Exception(f"Remove tag cannot be used outside of trigger contexts. Found {weight}")

if "triggers" in game_weights:
weights = roll_triggers(weights, game_weights["triggers"])
Expand Down
Loading