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

Docs: specify that deathlink cause should contain the player name #2557

Merged
merged 4 commits into from
Jan 13, 2024

Conversation

alwaysintreble
Copy link
Collaborator

yes

@ScipioWright ScipioWright added the is: enhancement Issues requesting new features or pull requests implementing new features. label Dec 3, 2023
@alwaysintreble alwaysintreble added is: documentation Improvements or additions to documentation. affects: core Issues/PRs that touch core and may need additional validation. and removed is: enhancement Issues requesting new features or pull requests implementing new features. labels Dec 3, 2023
@alwaysintreble alwaysintreble changed the title Docs: specify that the cause should contain the player name Docs: specify that deathlink cause should contain the player name Dec 3, 2023
@Jarno458
Copy link
Collaborator

Jarno458 commented Dec 3, 2023

This surprises me, this wasn't a requirement before, and i know several games that don't include it

Infact it used to be the opposite, not to include your player name in the case so you could do

					!string.IsNullOrEmpty(lastDeathLink.Cause)
						? $"DeathLink received from {lastDeathLink.Source}, Reason: {lastDeathLink.Cause}"
						: $"DeathLink received from {lastDeathLink.Source}");

@ScipioWright
Copy link
Collaborator

And I know several games that do include it, and so doing it the way you're doing it would show up strangely there.
Either the source needs to be included in the death link message or it needs to not be included. Including it makes it more flexible, such as being able to do "A grue thought Player looked tasty".

I guess there's the idea of, like in your example, specifically stating that you received a DeathLink. I don't really agree with the idea, though, since you need to explicitly enable death link in the first place, and that reduces creativity in death link messages.

@alwaysintreble
Copy link
Collaborator Author

when i implemented deathlink i asked how cause messages were supposed to be displayed, and what this change is doing is what i was told at the time and how clients are expected to handle it.

@Jarno458
Copy link
Collaborator

Jarno458 commented Dec 4, 2023

hmm well if different games do it differently, then i guess it would be good to explicitly specify it and i guess Timespinner for now willl stop supporting the standard

Copy link
Collaborator

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

if you go all out with the spaces in the table, please also reformat the columns to properly align.

| Name | Type | Notes |
| ---- | ---- |--------------------------------------------------------------------------------------------------------------------------------------------------------|
| time | float | Unix Time Stamp of time of death. |
| cause | str | Optional. Text to explain the cause of death. When provided, or checked, this should contain the player name, ex. "Berserker was run over by a train." |
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest removing the 'or checked' here as to me i have no idea what this is meant to mean

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when your client checks the death link packet to see what it should display.

| cause | str | Optional. Text to explain the cause of death, ex. "Berserker was run over by a train." |
| source | str | Name of the player who first died. Can be a slot name, but can also be a name from within a multiplayer game. |
| Name | Type | Notes |
|--------|-------|--------------------------------------------------------------------------------------------------------------------------------------------------------|
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|--------|-------|--------------------------------------------------------------------------------------------------------------------------------------------------------|
| ------ | ----- | ------------------------------------------------------------------------------------------------------------------------------------------------------ |

Copy link
Member

Choose a reason for hiding this comment

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

Nvmd this suggestion. the other tables follow your style. I think we should keep consistency for now.

@black-sliver black-sliver merged commit 2725c02 into ArchipelagoMW:main Jan 13, 2024
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
…chipelagoMW#2557)

* Docs: specify that the cause should contain the player name

* accidental whitespace moment

* fix table formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: documentation Improvements or additions to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants