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

LADX: enable deathlink #3828

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

threeandthreee
Copy link
Contributor

What is this fixing or adding?

Adds deathlink support to LADX. There was already existing work on it but it wasn't quite finished. This also adds a command to the client for toggling deathlink.

How was this tested?

Tested sending and receiving deathlinks in a few multiworlds.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Aug 22, 2024
@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Aug 22, 2024
was failing to initialize from slot data, also took the opportunity to set up for nicer merge with magpie slot data PR
kill with damage rather than setting hp to zero in order to trigger medicine, also refunds trade items on deathlink if they were used but the player died before getting the check
Copy link

@Illeea Illeea left a comment

Choose a reason for hiding this comment

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

not a programmer. did a game earlier tonight and at first it failed. a quick adjustment and it worked just fine. based on the latter version, deathlink works. its as annoying as it should be. i killed the hardhit beetle and during its death animation i got killed by deathlink so i had to refight it. which feels like how it should be tbh. i approve of this change to be added.

Copy link

@RadzPrower RadzPrower left a comment

Choose a reason for hiding this comment

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

I have participated in two asyncs and one sync testing the new deathlink functionality. I can confirm that I properly received deaths from others in the sync and perhaps once in the asyncs, but even in the asyncs it did indicate within the text client that a death was being sent on the rare occasion I did die.

As for the code itself, I am hardly a Python expert and I know essentially nothing to speak of in terms of the LADX integration specifically. That said, I did look over the code and I do not see anything egregiously wrong with the code. It appears to be fairly self-explanatory considering I could still generally follow the thought processes even if I didn't necessarily always have the full context. I also see where they addressed a bug encountered in an earlier beta version where dying during the trade item swap would lose you an item (I think this may have actually happened to me during the sync, but it's been a while so my memory is foggy).

Overall, I see no reason not to merge this PR into main.

Copy link
Contributor

@palex00 palex00 left a comment

Choose a reason for hiding this comment

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

This PR was tested as part of a Beta apworld that was used in three dedicated Test Asyncs, as well as dozen of normal Syncs. I have also personally run 200 test generations which succeeded.

We have also done polling on what users want, etc. which can be found here

The new feature has worked as expected and other stuff does not seem impacted.
In the beginning there was a hiccup with Death Links occuring during trade quests but the fix works flawlessly. After that, no more issues happened.

@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: peer-review Issue/PR has not been reviewed by enough people yet. labels Jan 14, 2025
@threeandthreee threeandthreee marked this pull request as draft January 23, 2025 15:05
@threeandthreee
Copy link
Contributor Author

found a major bug, drafting until fixed

@Exempt-Medic Exempt-Medic added waiting-on: author Issue/PR is waiting for feedback or changes from its author. and removed waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. labels Jan 24, 2025
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants