-
Notifications
You must be signed in to change notification settings - Fork 751
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
Blasphemous: Implement new game #1446
Conversation
init: fix plando not working docs: rename en_Hylics 2.md exits: fix airship having no entrances options: fix small typo rules: difficulty adjustments
Co-authored-by: PoryGone <[email protected]>
Co-authored-by: PoryGone <[email protected]>
Co-authored-by: PoryGone <[email protected]>
Co-authored-by: PoryGone <[email protected]>
Co-authored-by: PoryGone <[email protected]>
Will add it back once door randomizer is functional. Currently unused anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rules.py
here seems significantly longer and busier than it should be. Also made some comments on performance bottlenecks and nondeterminism. Otherwise looks fine.
return self.multiworld.random.choice(tears_set) | ||
|
||
|
||
def generate_basic(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire function looks really slow... for every single item in your table you're doing these comparisons every single time and you're going through every single dictionary. You should probably know what item you're on already when you start the initial loop so you don't have to do all of these comparisons every single time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote it to only check each option and dict/set once.
self.multiworld.itempool += pool | ||
|
||
|
||
def place_items_from_set(self, location_set: Set[str], name: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nondeterministic, which means it can't be reproduced which is very problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm misunderstanding something, but I can't see what makes this nondeterministic? I'm assuming it has something to do with the sets, but it still iterates through every entry in them and should have the same result either way.
if item["name"] == "Thorn Upgrade" and self.multiworld.thorn_shuffle[self.player].value == 1: | ||
self.multiworld.local_items[self.player].value.add("Thorn Upgrade") | ||
|
||
self.place_items_from_dict(Vanilla.unrandomized_dict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The remainder of this function should be in pre_fill
and it likely also causes failures with plando.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved into pre_fill
. Doesn't seem to cause any failures, just creates duplicates of the item and leaves something else unplaced.
My intention for all the options that place items in their vanilla locations was mainly for convenience on the web generator. I assume that anyone who wants to use plando for any items would realize the possibility of accidentally creating duplicates of items, so I'm fine with this.
state._blasphemous_silver_key(player)) | ||
|
||
# expert logic | ||
if world.expert_logic[player]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't expert logic be more difficult? Doesn't it make more sense to always set these rules, then add more lax rules with add_rule
when expert logic is off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I always set the expert rules and then added on top of them, wouldn't that effectively do nothing? If the expert rules are always there, wouldn't the randomizer always have a chance of using the more difficult solution, even with the option turned off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add_rule
defaults to and
, with or
being optional so it would be old_rule and new_rule
. alternatively set_rule
would just override it, but in lots of cases this is something where you want and because it might be "have this item and this other item" to reach it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, now I get what you're saying. But what I already had was already working anyways, so unless it would be faster if I rewrote them using add_rule
instead of set_rule
then I don't see why it needs to be changed.
Adds @BrandenEK's Blasphemous Randomizer as a new Archipelago game.
What is this fixing or adding?
Adds @BrandenEK's Blasphemous Randomizer as a new Archipelago game.
How was this tested?
Several private tests between the two of us, and our first test in the AP Discord server went very well.
If this makes graphical changes, please attach screenshots.