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

Xeno strains refactor #5542

Merged
merged 20 commits into from
Feb 15, 2024
Merged

Xeno strains refactor #5542

merged 20 commits into from
Feb 15, 2024

Conversation

SabreML
Copy link
Member

@SabreML SabreML commented Jan 24, 2024

About the pull request

Refactors xeno strains by taking out the old /datum/xeno_mutator//datum/mutator_set system and replacing it with a new datum/xeno_strain, since as far as I'm aware mutators haven't been touched in almost 5 years.

As a side note, the DMI changes are from me deleting the icon states of the unused 'Tremor' Burrower strain, since I figured it fit within the bounds of the PR. (The strain's sprites are identical to that of the base caste.)

Explain why it's good for the game

The new datum should be a lot more streamlined than the old system, and require much less boilerplate when making a new strain. Ideally this could even replace /datum/behavior_delagate with signal handlers in the future, although a few more refactors would be required before that's possible.

Testing Photographs and Procedure

Screenshots & Videos

Put screenshots and videos here with an empty line between the screenshots and the <details> tags.

Changelog

🆑
refactor: Refactored xeno strains.
add: Added a keybind setting to open the 'Purchase Strain' window. (Also renamed the macro)
/:cl:

oh boy here we go
I removed the `tacklestrength_*` part since it wasn't being used at all
Trapper first pass

Eggsac first pass

Charger first pass

Steel Crest first pass

Drone strains first pass

Resin Whisperer first pass

Vampire first pass

Praetorian strains first pass

Ravager strains first pass

Acider first pass

Watcher strain (almost forgot this existed lol)
The 'Tremor' burrower sprites look the same as the regular burrower, so I've just removed the icon update part.
It makes more sense there really
There's a lot of removed `mutation_type` checks in here, but as far as I can tell there's no way for other strains to access the ability in those cases.
I don't like using `istype()`, but I think one's needed here.
(woo final error fixed!!)
Mainly just so that `update_icons()` can be called after it.
@github-actions github-actions bot added Sprites Remove the soul from the game. UI deletes nanoui/html Refactor Make the code harder to read labels Jan 24, 2024
@SabreML SabreML marked this pull request as ready for review January 25, 2024 16:48
@SabreML SabreML requested a review from fira as a code owner January 25, 2024 16:48
@SabreML
Copy link
Member Author

SabreML commented Jan 25, 2024

The UI label can be removed now. It was just from me putting this in a jsx file: bd43b18.

@mullenpaul mullenpaul removed the UI deletes nanoui/html label Jan 27, 2024
@Birdtalon
Copy link
Contributor

I will try to get through this soon.

@cm13-github cm13-github added the Merge Conflict PR can't be merged because it touched too much code label Feb 1, 2024
@cm13-github
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@cm13-github cm13-github removed the Merge Conflict PR can't be merged because it touched too much code label Feb 1, 2024
@cm13-github
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@nauticall
Copy link
Contributor

please add ingame screenshots of your sprites to the PR description, thanks 🙏

@Birdtalon
Copy link
Contributor

please add ingame screenshots of your sprites to the PR description, thanks 🙏

@nauticall just FYI this PR only deletes unused Tremor burrower sprites.

@cm13-github cm13-github added the Merge Conflict PR can't be merged because it touched too much code label Feb 5, 2024
@cm13-github
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Renamed the variable to be a bit more descriptive, and replaced the defines with their strings.
I'm SURE this was working before, but obviously it broke at some point
cm13-github added a commit that referenced this pull request Feb 12, 2024
@ekcja1 ekcja1 mentioned this pull request Feb 12, 2024
3 tasks
cm13-github added a commit that referenced this pull request Feb 13, 2024
cm13-github added a commit that referenced this pull request Feb 13, 2024
@Crimsonerva Crimsonerva mentioned this pull request Feb 13, 2024
3 tasks
@Megaddd
Copy link
Contributor

Megaddd commented Feb 13, 2024

@SabreML this PR breaks/removes the purchase strains hotkey currently

@SabreML
Copy link
Member Author

SabreML commented Feb 13, 2024

@SabreML this PR breaks/removes the purchase strains hotkey currently

Shoot, thanks for letting me know!

@SabreML
Copy link
Member Author

SabreML commented Feb 13, 2024

@Megaddd It looks like the issue was from me changing the verb's name from 'Purchase Strains' to 'Purchase Strain' (since it's a bit more accurate), which means that the command to activate it also changed to Purchase-Strain.

I've just added an "official" keybind setting for it, so I'd suggest either swapping over to that or editing your macro to use the non-plural command.

@FighterOfKeyboard FighterOfKeyboard mentioned this pull request Feb 13, 2024
3 tasks
cm13-github added a commit that referenced this pull request Feb 14, 2024
cm13-github added a commit that referenced this pull request Feb 14, 2024
cm13-github added a commit that referenced this pull request Feb 14, 2024
cm13-github added a commit that referenced this pull request Feb 14, 2024
cm13-github added a commit that referenced this pull request Feb 14, 2024
cm13-github added a commit that referenced this pull request Feb 15, 2024
cm13-github added a commit that referenced this pull request Feb 15, 2024
Copy link
Contributor

@Birdtalon Birdtalon left a comment

Choose a reason for hiding this comment

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

Looking good, testing issues have been resolved.

@Birdtalon Birdtalon added this pull request to the merge queue Feb 15, 2024
Merged via the queue into cmss13-devs:master with commit d998245 Feb 15, 2024
26 checks passed
cm13-github added a commit that referenced this pull request Feb 15, 2024
@SabreML SabreML mentioned this pull request Feb 15, 2024
github-merge-queue bot pushed a commit that referenced this pull request Feb 19, 2024
# About the pull request

Adds a unit test for xeno strains.
I could have included this in #5542, but I thought it would be better in
its own PR in order to keep the checks and everything separate.

The removed sprites for the Trapper, Charger, Gardener, and Watcher
strains are (as far as I'm able to tell) completely identical to that of
the base caste, and are only there in order to make the old system work
properly.

Currently, this includes the following tests:
- Checking that the `caste_type` variable on each
`/mob/living/carbon/xenomorph` subtype has a corresponding entry in
`GLOB.xeno_datum_list`. (Not really strain related but I figured no harm
in including it)
- Checking that each `/datum/xeno_strain` subtype has its own `name` and
`description`.
- Making sure that each strain's `actions_to_add` and
`actions_to_remove` lists are added and removed from the xeno properly.
(Taking into account any which are re-added.)
- Making sure that the strain's `behavior_delegate_type` is successfully
added to the xeno.
- If the strain doesn't have an `icon_state_prefix`, going through the
xeno's `icon` file and double checking that there aren't strain sprites
in there (That is, sprites beginning with the strain's name). If there
are any, then `icon_state_prefix` should probably be set to use them.
- If the strain *does* have an `icon_state_prefix`, making sure that the
xeno's sprite actually got changed to the strain version.

# Explain why it's good for the game

More unit tests! (woo)

# Testing Photographs and Procedure
<details>
<summary>Screenshots & Videos</summary>

Put screenshots and videos here with an empty line between the
screenshots and the `<details>` tags.

</details>


# Changelog
:cl:
code: Added a unit test for xeno strains.
/:cl:
github-merge-queue bot pushed a commit that referenced this pull request Feb 22, 2024
# About the pull request

Fixes #5765.
This was caused by me in #5542 deleting a couple of lines from
`/mob/living/carbon/xenomorph/proc/recalculate_tackle()`, when I should
have only removed the `mutators`/`hive.mutators` part.


![image](https://github.com/cmss13-devs/cmss13/assets/57483089/b54a3e03-03cd-49a3-a093-ab0765059a7b)

# Explain why it's good for the game

Whoops

# Testing Photographs and Procedure
<details>
<summary>Screenshots & Videos</summary>

`tacklestrength_min` and `tacklestrength_max` are being applied
correctly:

![image](https://github.com/cmss13-devs/cmss13/assets/57483089/8404ddf3-17e0-42a5-a3cf-91a85beb1e50)

</details>


# Changelog
:cl:
fix: Fixed Xenomorphs not having their caste's tackle duration values
applied to them.
/:cl:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Make the code harder to read Testmerge Candidate we'll test this while you're asleep and the server has 10 players
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants