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

alttp: Add second power star to differentiate otherworld progression items #2139

Closed
wants to merge 4 commits into from

Conversation

krelbel
Copy link

@krelbel krelbel commented Aug 31, 2023

This change updates the alttp basepatch to add item ID 0x69 for advancement items from other worlds, and adds the corresponding item and sprite handling to allow displaying gold power stars for advancement/progression items from other worlds and silver power stars for other items.

The basepatch was generated from krelbel/z3randomizer@91c3dd6

This was tested with a 2-world alttp/hollow knight multiworld, verifying that both advancement (gold) and non-advancement (silver) power stars are displayed correctly in shops and chests.

Example image: https://i.imgur.com/iMPxzgw.png

This change updates the alttp basepatch to add item ID 0x69 for advancement items from other worlds, and adds the corresponding item and sprite handling to allow displaying gold power stars for advancement/progression items from other worlds and silver power stars for other items.

The basepatch was generated from krelbel/z3randomizer@91c3dd6

Example image: https://i.imgur.com/B2LUdFS.png
Comment on lines 103 to 105
'Blue Power Star': ItemData(IC.progression, None, 0x69, 'a small victory', 'and the power star', 'star-struck kid', 'star for sale', 'see stars with shroom', 'mario powers up again', 'a Blue Power Star'),
'Triforce': ItemData(IC.progression, None, 0x6A, '\n YOU WIN!', 'and the triforce', 'victorious kid', 'victory for sale', 'fungus for the win', 'greedy boy wins game again', 'the Triforce'),
'Power Star': ItemData(IC.progression, None, 0x6B, 'a small victory', 'and the power star', 'star-struck kid', 'star for sale', 'see stars with shroom', 'mario powers up again', 'a Power Star'),
'Power Star': ItemData(IC.filler, None, 0x6B, 'a small victory', 'and the power star', 'star-struck kid', 'star for sale', 'see stars with shroom', 'mario powers up again', 'a Power Star'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't the progression type be reversed per the description?

Copy link
Author

Choose a reason for hiding this comment

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

Yes; I fixed this, and also fixed a bug where silver power stars weren't animating correctly after pickup. Please let me know if I messed anything up in the process of pushing my fixup/merge changes, or if everything looks okay now.

Comment on lines 768 to 769
# Set all non-native sprites to a yellow Power Star for non-advancement
# items and a blue Power Star for advancement items as per 13 to 2 vote at
Copy link
Collaborator

Choose a reason for hiding this comment

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

again, think you got this backwards per the description of the PR?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch; fixed.

This change updates the gold/silver power star item names to be consistent with the final planned behavior of progression items being gold and all other items being silver, and imports a new basepatch based on ArchipelagoMW/z3randomizer@b787b83 which fixes a bug in the previous commit where the silver power star item in newitems.asm:.properties still used the gold power star palette, causing the "item get" animation to incorrectly show a gold power star upon picking up a silver power star.

Tested with a 2-player lttp/hollowknight multiworld verifying standing items look correct in and out of shops and picking up items animates correctly for both gold and silver power stars.

The previous commit listed an outdated screenshot (with the colors reversed); the updated screenshot showing this feature is: https://i.imgur.com/iMPxzgw.png
Fix outdated comment
Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

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

Tested personally, and saw Bel test it too. Works, and looks great.

if 84173 >= code >= 84007: # LttP item in SMZ3
return code - 84000
return 0x6B # set all non-native sprites to Power Star as per 13 to 2 vote at
# Set all non-native sprites to a gold Power Star for advancement
# items and a silver Power Star for non-advancement items as per 13 to 2 vote at
# https://discord.com/channels/731205301247803413/827141303330406408/852102450822905886
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment regarding the vote seems at least somewhat misleading now, since that was not about gold vs. silver at all.

'Triforce': ItemData(IC.progression, None, 0x6A, '\n YOU WIN!', 'and the triforce', 'victorious kid', 'victory for sale', 'fungus for the win', 'greedy boy wins game again', 'the Triforce'),
'Power Star': ItemData(IC.progression, None, 0x6B, 'a small victory', 'and the power star', 'star-struck kid', 'star for sale', 'see stars with shroom', 'mario powers up again', 'a Power Star'),
'Silver Power Star': ItemData(IC.filler, None, 0x6B, 'a small victory', 'and the power star', 'star-struck kid', 'star for sale', 'see stars with shroom', 'mario powers up again', 'a Power Star'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the golden one called "a Blue Power Star", while the silver one is "a Power Star"?
(Although I am not sure whether that hint text is ever actually used for otherworld items.)

@krelbel
Copy link
Author

krelbel commented Sep 8, 2023

Closing based on harassment from Berserker and others, will finish this work on the Doors-on-AP project instead.

@krelbel krelbel closed this Sep 8, 2023
@eudaimonistic
Copy link
Contributor

Allegations of harassment in this repository are actually my domain and responsibility, so it falls upon me to provide some context here. I should also add that I'm amongst those accused, only, not in this particular comment from krelbel but instead in the community discord. Moreover, the subject of krelbel's last comment isn't even about this PR, but rather a separate conversation about ALTTPR and it's future in Archipelago. So, understand that his remarks here don't reflect the reception of this particular PR, which was generally pretty positive. I don't take back the positive impression I gave of it either.

The reality here is that krelbel initiated a lengthy (5+ hours!) argument in the community discord #link-to-the-past channel bemoaning the divergence of base patches in the various ALTTPR forks of the last 3 years. There is way too much history to even begin to fully explain here, but the long and short of it is that this project exists in part due to our upstream BMW project leaving the larger ALTTPR scene. The conflict that happened back then directly impacts base patch efforts today, because Berserker has placed particularly stringent requirements on this individual game. I'm not here to air out all of the old laundry, but the consequences for Berserker back then were severe, due to false accusations of stealing the work of others. Even after pointing out the MIT licensing and seeking every author's personal consent to use their work, he was accused of theft and bullied out of that community. The only people who dispute Berserker's fair use of MIT-licensed work are people who didn't contribute to it. As a result, Berserker has enacted a policy that meets both the ethical and legal demands of licensing, while still avoiding the historically misplaced drama.

Work towards unifying the base patches of the disparate forks genuinely requires that the original authors port their work, or the perpetrators of harassment of yesteryear need to make amends. Without one of those two steps, trying to make this effort happen is an assured recipe for community conflict, because it has already happened before. Berserker has made this policy explicit by pinning this expectation in the #link-to-the-past channel of the community discord server. In krelbel's eyes (per his words in that conversation, not my interpretation), this requirement makes any hope of a unified base patch (and therefore, unified projects in Archipelago such as Doors randomizer) dead on arrival. One of the relevant devs agreed that these requirements make it pretty difficult to simply merge the changes, or cherry-pick them with ease. Not impossible, but significantly harder to do. There are other devs of these projects who would almost assuredly not go along with that plan, further complicating any efforts to incorporate base patch changes from other projects into Archipelago.

The accusation of harassment however is difficult to substantiate. On one hand, krelbel did in fact find hostility in the community server. However, this followed attempts to splinter the community, accuse it's founding member of hostage-taking the ALTTP world, ignoring reasonable counterpoints, and generally re-arguing the point to an extreme degree. The community was exasperated at the unwillingness of krelbel to accept that Berserker has a right to protect himself and his project and his community from external harm. krelbel wanted the incorporation of other projects so badly, that he was willing to fork the current world implementation of ALTTP in Archipelago, despite the overflowing context provided in that discussion, knowing that it would come at great personal cost to Berserker and the community he has built. Despite all of this, absolutely nothing would make krelbel budge even an inch. Rather than simply leaving the conversation at that, he dug his heels deeper and deeper, getting more and more aggressive as time passed. He has taken this course of action knowing full well that it will result in fallout, of which he doesn't even take the brunt of the punishment from. He will likely claim this is a false fear, but we have proof that this is how things unfolded before.

I arrived to the discussion 4 hours in. After krelbel had made it perfectly clear that they were going to ignore the consequences for the community and move to a forked project instead, I dropped presumptions and called out the bad faith circular argumentation that had plagued the conversation up to this point. He has every right to fork the project in any capacity if he wants. He cannot pretend that nobody informed him of the consequences of that choice. To characterize a day's worth of the community defending itself against krelbel's well-poisoning as "harassment" is at best a weak bait to the very conduct he projects. It took hours of attacking Berserker's character, ignoring reasonable counterarguments, and feigning investment in alternatives before he eventually showed his true colors and announced he would be forking the project.

I'll admit that the community could have managed a few less sharp words in the public channel this conversation took place in. Nothing was deleted, you can peruse it for yourself by viewing the entire day's chat log in #link-to-the-past on September 7th, 2023. Is it possible that users directly harassed krelbel in private that I can't see? I can't verify it, and krelbel offers zero evidence of it. It didn't happen on this PR, let alone within the repository. The discord community at large did not harass krelbel, he actively instigated conflict and has expressed no qualms about the threat of conflict against the community. People rightfully got frustrated at his conduct. A whopping dozen or so people who actually weighed in, compared to the presently over 8000 community members in the server. In the worst cases, people expressed frustration and didn't go out of their way to be extra diplomatic about it. Disagreeing isn't harassment. Defending yourself isn't harassment. When you swing a bat at a bee hive, don't be surprised that the bees will want to sting you in response.

Calling out a false narrative and sticking to points that you ignore is not harassment. It's holding a person to account when they play the crybully. We will not stand by idly and watch you put a torch to the community just because you aren't getting your way. We would be foolish to ignore that this has happened before.

@krelbel
Copy link
Author

krelbel commented Sep 9, 2023

Just correcting a couple mistakes there:

In krelbel's eyes (per his words in that conversation, not my interpretation), this requirement makes any hope of a unified base patch (and therefore, unified projects in Archipelago such as Doors randomizer) dead on arrival.

Those were Aerinon's words, not mine (any time I used the phrase "dead on arrival" I was just quoting (and agreeing) with him):

"Without reading everything in its entirety, it seems one path boils down to permission from original code authors. If that's required, then I think porting the baserom is dead on arrival. I'm pretty sure kat & kan won't be giving their permission based on prior discussions. I've stated before that I don't care if people work off my work, I mean that's why I put a license in the first place so people can go do whatever with it. Obviously that didn't help avoid drama apparently. Shopsanity was pepperpow's work originally and I adapted it for DR, not sure if I really gave anyone the heads up on that; it's been too long. Cassidy and codemann might be open to it and cassidy's done most of the recent work on the baserom but it definitely was a collaboration with kan. Furthermore, it's been a huge pain to re-integrate all the features I've added, and I'm not done yet. So yeah, it looks from a policy standpoint; this way lies madness.

I'm not sure on the hair being split here in making a new AP world, but if that's an actual option, I'd consider it. If it uses my or codemann's fork of the baserom though then I don't know if it'll be disallowed just on principle because the only practical way I see to do it will contain other people's work under the hood. Any other solution and we're talking maintaining two versions of the baserom, and I'm not sure I can realistically cherry-pick work out of the baserom without just starting over. Being one person too, there's no realistic way for me to maintain two forks of the baserom in any practical sense either. But I've no conception of the work it'd take to make a separate AP world so I'm not really committed yet to the work."

krelbel wanted the incorporation of other projects so badly, that he was willing to fork the current world implementation of ALTTP in Archipelago, despite the overflowing context provided in that discussion, knowing that it would come at great personal cost to Berserker and the community he has built.

The plan involves updating the existing https://github.com/aerinon/ALttPDoorRandomizer project, making it work with the AP apworld API like any other new randomizer, not forking any AP repo.

@Berserker66
Copy link
Member

In the past, and planned in continuation here, is the transfer of features from Archipelago to ALTTPR, a transfer that we have so far at least barely resisted against, despite transfers in the other direction being strongly opposed with accusations of theft and creation of community drama.
The outlined path by krelbel, I therefore find malicious in intent.

@ThePhar ThePhar added the is: enhancement Issues requesting new features or pull requests implementing new features. label Oct 16, 2023
@krelbel krelbel reopened this Feb 25, 2024
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Feb 25, 2024
@krelbel
Copy link
Author

krelbel commented Feb 25, 2024

Reopening this PR to make clear my desire to eventually see this feature added to AP. The more recent PR for this feature is #2858 with an ongoing discussion there. Please let me know if there's anything else I can do to help (and please feel free to close this PR again if you'd rather continue this work in a different PR).

@Exempt-Medic Exempt-Medic added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Apr 5, 2024
@Exempt-Medic
Copy link
Collaborator

Should we just close this since #2866 exists?

@krelbel
Copy link
Author

krelbel commented Aug 7, 2024

Yes, I'm okay with closing this, #2866 looks good to me.

@krelbel krelbel closed this Aug 7, 2024
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. waiting-on: author Issue/PR is waiting for feedback or changes from its author. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants