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

SMZ3: Fixed Ganon sign text on AllDungeonsDefeatMotherBrain goal #1617

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

ScorelessPine
Copy link
Contributor

Please format your title with what portion of the project this pull request is
targeting and what it's changing.

ex. "MyGame4: implement new game" or "Docs: add new guide for customizing MyGame3"

What is this fixing or adding?

SMZ3's Ganon Sign previously erroneously told the user how many crystals the "ganon_vulnerable" config option rolled, regardless of if it chose the AllDungeonsDefeatMotherBrain goal.

How was this tested?

A game was generated with the problematic settings and the sign was read to confirm the text change. A second game was generated with normal settings to confirm the text does not change on other goals.

If this makes graphical changes, please attach screenshots.

image
image

Last note: I would appreciate if anyone would like to suggest alternate wording for the all dungeons goal sign text. The current text is 4 lines long in game, and i'm not sure what total's smz3 uses for all dungeons goal text.

@ScorelessPine ScorelessPine changed the title Fixed Ganon sign text on AllDungeonsDefeatMotherBrain goal SMZ3: Fixed Ganon sign text on AllDungeonsDefeatMotherBrain goal Mar 28, 2023
@Berserker66
Copy link
Member

How does the sign behave on upstream SMZ3?

@ScorelessPine
Copy link
Contributor Author

I never really thought to check, it actually has this issue upstream too
image

@ScorelessPine ScorelessPine marked this pull request as draft March 28, 2023 20:50
@ScorelessPine ScorelessPine marked this pull request as ready for review April 2, 2023 22:56
@ScorelessPine
Copy link
Contributor Author

This change was just merged on main as seen in tewtal/SMZ3Randomizer#209
Below is an image from a game generated on beta.samus.link after the change was added to show the functionality should be the same, after 1e6829f.
image

@ScorelessPine
Copy link
Contributor Author

https://samus.link/changelog
This change (and a few more fixes) is now reflected on the main site. Ill update the branch with main as well so it can be ready for review.

@Berserker66
Copy link
Member

@lordlou looks good to you?

@lordlou
Copy link
Contributor

lordlou commented Apr 8, 2023

Yes! Altough, I would maybe wait for the new version of SMZ3 to come out before merging? I would include the fix with the next update of SMZ3 AP. That way, we keep in sync with upstream versioning.

@ThePhar ThePhar added the is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. label May 31, 2023
@eudaimonistic
Copy link
Contributor

This fix is live in upstream SMZ3 as of the 11.3.1 release (which is current release as of writing), so I think this is good to merge?

@ScorelessPine
Copy link
Contributor Author

This fix is live in upstream SMZ3 as of the 11.3.1 release (which is current release as of writing), so I think this is good to merge?

The fix has been in upstream since April, I was kind of just leaving this PR in limbo since @lordlou said they would include this fix with the next release of SMZ3 AP. I could close this PR if that is still the plan but I was leaving it open as a sort of reminder, I suppose.

Copy link
Contributor

@lordlou lordlou left a comment

Choose a reason for hiding this comment

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

This can be merged: it follows Upstream. I'll follow up shortly with another PR to wrap the update to 11.3.1. And maybe another one for some other reported bug fixes and core changes.

@ThePhar ThePhar merged commit 81b9564 into ArchipelagoMW:main Jul 6, 2023
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants