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

Senior ID cards get a custom job name: Attempt 2 #1418

Closed
wants to merge 8 commits into from

Conversation

WarMechanic
Copy link
Contributor

@WarMechanic WarMechanic commented Jun 20, 2024

About the PR

So last time senior ID job titles tried to get implemented, it practically didn't work. I personally never saw senior IDs in effect, and they only applied to players at round-start without even influencing the crew manifest.
I'm not quite done yet, but right now my solution catches both roundstart and latejoining players. It also applies the correct ID icons, so security will finally see some fancy "M"s walking around. Senior ID job name and icon also display correctly in crew manifest now.

Technical details

This solution reverts #1218 which relied on an event that was raised only after ready players spawned in at roundstart. Instead of a customJobName variable, we use job prototypes for senior roles and add logic before a player joins to use a 'virtualJob' based on your ID card. Senior ID cards now link to senior role prototypes instead of normal roles.

Media

  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Not from what I've seen so far, although anything that relies on the redundant ID name change on spawn will have to be checked.

Changelog

🆑

  • fix: Fixed senior roles not displaying properly

@github-actions github-actions bot added Changes: YML Changes any yml files Changes: C# Changes any cs files Changes: Localization Changes any ftl files labels Jun 20, 2024
@WarMechanic
Copy link
Contributor Author

its in a working state now but the crew manifest ended up being bugged because senior job prototypes werent listed in their respective departments :blunt:

ill have to review the logic to cull anything thats not needed.

Copy link
Contributor

@NullWanderer NullWanderer left a comment

Choose a reason for hiding this comment

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

This is adding additional data that was the entire reason we avoided doing this in the first place. It should be investigated why the code was faulty and fixed, not removed and replaced by the one thing that was being avoided

@NullWanderer NullWanderer added the S: Do Not Merge Don't merge this yet label Jun 21, 2024
@WarMechanic
Copy link
Contributor Author

This is adding additional data that was the entire reason we avoided doing this in the first place. It should be investigated why the code was faulty and fixed, not removed and replaced by the one thing that was being avoided

is it including unnecessary data in prototypes, or the fact that there are prototypes in the first place that you want to avoid?

i added things like access to the senior job prototypes just as a formality although they will probably never be used so i can cull them if you want. i did initially try to parent the prototypes but that didn't work.

@WarMechanic
Copy link
Contributor Author

WarMechanic commented Jun 21, 2024

also i originally attempted to work from your fix, but i determined that id have to put bandaids in multiple different scripts and stuff. i didnt think the solution would be very readable and would be more annoying to maintain by extension

here most of the code takes place in one script, and it uses job prototypes for compatibility.

@WarMechanic
Copy link
Contributor Author

alright im going to assume the worst-case scenario in that you want me to remove the prototypes and replace them with components, ill start working on that now

@WarMechanic
Copy link
Contributor Author

actually ill upload it as a separate PR in the case that this solution would be preferred

LaryNevesPR pushed a commit to LaryNevesPR/Estacao-Andromeda that referenced this pull request Jan 11, 2025
# Description

Red Shirt and BBD didn't follow the pattern of disallowing their
opposite traits from being equipped (I.e. tenacity v glass jaw, will to
live v will to die, amplification v dampening traits).

---

:cl:
- tweak: You can no longer simultaneously equip Red Shirt and Brittle
Bones Disease and their positive counterparts, Will To Live and
Tenacity.

Co-authored-by: sleepyyapril <[email protected]>
LaryNevesPR pushed a commit to LaryNevesPR/Estacao-Andromeda that referenced this pull request Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: C# Changes any cs files Changes: Localization Changes any ftl files Changes: UI Changes: YML Changes any yml files S: Do Not Merge Don't merge this yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants