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

Corrupted Queen can set up personal alliances with humans #6155

Merged
merged 10 commits into from
May 1, 2024

Conversation

ihatethisengine
Copy link
Contributor

@ihatethisengine ihatethisengine commented Apr 16, 2024

About the pull request

Corrupted queen can mark people of her choice (within her view) as allies. Corrupted xenos won't be able to harm them. Just as regular alliances, personal alliances break on queen's death. Synths are human subtypes so you can ally them too.

Credits to @SubjectD9341 for the sprites.

Explain why it's good for the game

More roleplay :)

ally.mp4

Testing Photographs and Procedure

Changelog

🆑 ihatethisengine
add: Corrupted Queen can set up personal alliances with humans
/:cl:

@github-actions github-actions bot added Sprites Remove the soul from the game. Feature Feature coder badge labels Apr 16, 2024
fira
fira previously requested changes Apr 16, 2024
Copy link
Member

@fira fira left a comment

Choose a reason for hiding this comment

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

To sum up some things mentioned in other comments:

  • Typing consistency across code - this could also be generalized to /mob or /mob/living
  • Missing post alert/input checks in adding/removing/clearing allies, things can change during the dialogs
  • You can't just sanitize deleted mobs once in a while, they need to be cleared immediately by signal or just using a weakref

As extra points I'd like to bring up, hardcoding this to Corrupted hive is bit of a shame. This could be made into a feature toggleable for all hives from the get-go. You could also two-birds-one-stone it by using a component hooked to is_ally instead of bothering with CORRUPTED_ALLY at all. But both of these are secondary points...

@Drulikar
Copy link
Contributor

Drulikar commented Apr 16, 2024

As extra points I'd like to bring up, hardcoding this to Corrupted hive is bit of a shame. This could be made into a feature toggleable for all hives from the get-go.

That was a requirement set by me. But if you'd rather it be a variable that only corrupted has by default set so an admin can toggle it on for other hives, sure. Also of note, other research hives this is fine for; but I don't think it appropriate for normal hive.

@ihatethisengine
Copy link
Contributor Author

ihatethisengine commented Apr 16, 2024

HUDs is another reason I went for hardcode because making personal HUDs for every hive just for a feature that meant to be corrupted only sounds like a big overkill.

Also of note, other research hives this is fine for

As far as I now, only corrupted (and renegades, but they don't have queen so it doesn't matter) can act friendly towards humans. I.e. Alpha hive should always be hostile even if research makes them.

Anyway, tell me if you need this feature for all hives and I will look what can I do. If not, I prefer to keep it simple.

@ihatethisengine
Copy link
Contributor Author

  • You can't just sanitize deleted mobs once in a while, they need to be cleared immediately by signal or just using a weakref

Won't I need to sanitize weekrefs once in a while anyway?

@ihatethisengine ihatethisengine marked this pull request as draft April 16, 2024 18:57
@fira
Copy link
Member

fira commented Apr 17, 2024

That was a requirement set by me. But if you'd rather it be a variable that only corrupted has by default set so an admin can toggle it on for other hives, sure. Also of note, other research hives this is fine for; but I don't think it appropriate for normal hive.

Yeah I meant this purely codewise, it can be incorporated into global hive code and disabled when not appropriate rather than hardcoded and require a refactor if used elsewhere. But it might just be wishful thinking, if we really have no use case for it, it's not worth bothering

Won't I need to sanitize weekrefs once in a while anyway?

You can pop invalid ones out of the list if you want, but i'd say it's not a requirement for something low traffic like this

@ihatethisengine ihatethisengine marked this pull request as ready for review April 17, 2024 06:03
@Drulikar Drulikar added the Testmerge Candidate we'll test this while you're asleep and the server has 10 players label Apr 20, 2024
cm13-github added a commit that referenced this pull request Apr 20, 2024
cm13-github added a commit that referenced this pull request Apr 20, 2024
Copy link
Contributor

@nauticall nauticall left a comment

Choose a reason for hiding this comment

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

sprite 👍

the health hud bit being modified is a bit weird but it looks the same so, alright

@nauticall nauticall added the Sprites Approved confirmed no stray pixels label Apr 20, 2024
cm13-github added a commit that referenced this pull request Apr 20, 2024
cm13-github added a commit that referenced this pull request Apr 21, 2024
cm13-github added a commit that referenced this pull request Apr 21, 2024
cm13-github added a commit that referenced this pull request Apr 22, 2024
cm13-github added a commit that referenced this pull request Apr 23, 2024
cm13-github added a commit that referenced this pull request Apr 23, 2024
cm13-github added a commit that referenced this pull request Apr 24, 2024
cm13-github added a commit that referenced this pull request Apr 24, 2024
cm13-github added a commit that referenced this pull request Apr 24, 2024
cm13-github added a commit that referenced this pull request Apr 25, 2024
cm13-github added a commit that referenced this pull request Apr 25, 2024
cm13-github added a commit that referenced this pull request Apr 25, 2024
cm13-github added a commit that referenced this pull request Apr 26, 2024
cm13-github added a commit that referenced this pull request Apr 26, 2024
cm13-github added a commit that referenced this pull request Apr 26, 2024
cm13-github added a commit that referenced this pull request Apr 26, 2024
cm13-github added a commit that referenced this pull request Apr 27, 2024
cm13-github added a commit that referenced this pull request Apr 27, 2024
@Shibouze Shibouze mentioned this pull request Apr 27, 2024
3 tasks
Copy link
Contributor

This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself

@github-actions github-actions bot added the Stale beg a maintainer to review your PR label Apr 28, 2024
@Biolock1 Biolock1 mentioned this pull request Apr 28, 2024
3 tasks
cm13-github added a commit that referenced this pull request Apr 28, 2024
cm13-github added a commit that referenced this pull request Apr 29, 2024
cm13-github added a commit that referenced this pull request Apr 29, 2024
cm13-github added a commit that referenced this pull request Apr 30, 2024
Copy link
Contributor

@Drulikar Drulikar left a comment

Choose a reason for hiding this comment

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

If nothing has been discovered at this point, we can fix it ever something comes up. But as far as I can tell its all working locally.

@Drulikar Drulikar added this pull request to the merge queue May 1, 2024
Merged via the queue into cmss13-devs:master with commit 029cdbd May 1, 2024
26 checks passed
cm13-github added a commit that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feature coder badge Sprites Approved confirmed no stray pixels Sprites Remove the soul from the game. Stale beg a maintainer to review your PR 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.

4 participants