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

Various Games: Improve custom death link option description #4171

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

SunCatMC
Copy link
Contributor

@SunCatMC SunCatMC commented Nov 11, 2024

What is this fixing or adding?

#3951 fixed the description in core, but of course that didn't change all the custom docstrings in various games. This PR seeks to remedy that.

How was this tested?

genned template yamls using Launcher.py

If this makes graphical changes, please attach screenshots.

blasphemous
image
bomb rush cyberfunc
image
cv64
image
hylics2
image
kh1
image
image
noita
image

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Nov 11, 2024
@SunCatMC
Copy link
Contributor Author

Seeking review from @TRPG0, @LiquidCat64, @gaithern, @ScipioWright @heinermann, @Berserker66

@Berserker66
Copy link
Member

Subnautica change should probably just be deleting its custom text? But otherwise, the change looks fine to me.

@Exempt-Medic Exempt-Medic added is: documentation Improvements or additions to documentation. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Nov 11, 2024
Co-authored-by: Exempt-Medic <[email protected]>
@SunCatMC
Copy link
Contributor Author

SunCatMC commented Nov 11, 2024

Subnautica change should probably just be deleting its custom text? But otherwise, the change looks fine to me.

It contains the following addition:

Note: can be toggled via in-game console command "deathlink".

so i can't remove the message without removing the addition

@Berserker66 if you are fine with that part being removed, then i can remove it
Edit: ah, i see now what you did

Copy link
Collaborator

@heinermann heinermann left a comment

Choose a reason for hiding this comment

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

Noita desc is fine.

@Berserker66
Copy link
Member

Subnautica change should probably just be deleting its custom text? But otherwise, the change looks fine to me.

It contains the following addition:

Note: can be toggled via in-game console command "deathlink".

so i can't remove the message without removing the addition

fixed it in #4172

@Berserker66
Copy link
Member

It looks like what I did in my PR should work for most if not all of the other games as well, if you want core changes to auto-propagate, instead of changing your text when core changes.

@SunCatMC
Copy link
Contributor Author

made all possible changes auto-updating, and removed changes to subnautica in favor of #4172

Copy link
Collaborator

@TRPG0 TRPG0 left a comment

Choose a reason for hiding this comment

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

Looks good for Blasphemous, Bomb Rush, Hylics 2 👍

@gaithern
Copy link
Contributor

Looks good for KH1!

worlds/cv64/options.py Outdated Show resolved Hide resolved
Co-authored-by: LiquidCat64 <[email protected]>
@LiquidCat64
Copy link
Contributor

LiquidCat64 commented Nov 11, 2024

Importing the common DeathLink class into CV64's options.py causes PyCharm to throw a warning with CV64's own DeathLink class, saying "Redeclared 'DeathLink' defined above without usage", so I think it would be best to rename the CV64 DeathLink class's name to CV64DeathLink like the other changed worlds are doing. All usages of it that need updating are in options.py and rom.py (which is importing it from options.py).

@SunCatMC
Copy link
Contributor Author

SunCatMC commented Nov 12, 2024

edited the class, the game did gen
can't test rom creation 'cos i don't have the rom

Copy link
Contributor

@LiquidCat64 LiquidCat64 left a comment

Choose a reason for hiding this comment

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

Tested the ROM sending/receiving deaths and everything is working as it should. CV64 should now be good to go in this! 👍

@Exempt-Medic
Copy link
Collaborator

Importing the common DeathLink class into CV64's options.py causes PyCharm to throw a warning with CV64's own DeathLink class, saying "Redeclared 'DeathLink' defined above without usage", so I think it would be best to rename the CV64 DeathLink class's name to CV64DeathLink like the other changed worlds are doing. All usages of it that need updating are in options.py and rom.py (which is importing it from options.py).

Can't you just subclass DeathLink instead?

Co-authored-by: Scipio Wright <[email protected]>
@SunCatMC
Copy link
Contributor Author

Importing the common DeathLink class into CV64's options.py causes PyCharm to throw a warning with CV64's own DeathLink class, saying "Redeclared 'DeathLink' defined above without usage", so I think it would be best to rename the CV64 DeathLink class's name to CV64DeathLink like the other changed worlds are doing. All usages of it that need updating are in options.py and rom.py (which is importing it from options.py).

Can't you just subclass DeathLink instead?

how would you turn a DeathLink Toggle into a Choice via subclassing?

@Exempt-Medic
Copy link
Collaborator

how would you turn a DeathLink Toggle into a Choice via subclassing?

Oh 🤦

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.

Noita's looks good now

@Exempt-Medic Exempt-Medic added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: documentation Improvements or additions to documentation. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants