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

[Port/Refactor] Job Refactor #11129

Closed

Conversation

XeonMations
Copy link
Contributor

@XeonMations XeonMations commented Jun 28, 2024

About The Pull Request

Requires:

Dehardcodes alot of stuff, bringing our jobs to updated TG standards.

Ports fully or bits of code from:

Why It's Good For The Game

QoL for custom jobs and stuff like that, also helps to bring our code up to date.

Testing Photographs and Procedure

Screenshots&Videos

Things for when there is noone in a command position:
image
image
(Departures lounge is the location of the communications console, in this case, on RuntimeStation: Departures Lounge)
image
image

Changelog

🆑 EvilDragonfiend, Rohesie, Timberpoes, Fikou, JohnFulpWillard, Jacquerel, MrMelbert, Ghommie, XeonMations
refactor: Dehardcoded a lot of job code, added job flags, as well as making the code hopefully better
add: Communications Consoles now have the ability to request codes to the spare ID safe when there are no crew members in the chain of command on the shift. This can be done without logging into the comms console.
/:cl:

@EvilDragonfiend
Copy link
Member

EvilDragonfiend commented Jun 28, 2024

TG's job code isn't really great so that I won't recommend porting it.
Note that this would have a high chance to get a close key from me.

@EvilDragonfiend EvilDragonfiend self-assigned this Jun 28, 2024
Copy link
Member

@EvilDragonfiend EvilDragonfiend left a comment

Choose a reason for hiding this comment

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

I skimmed, but the majour problem comes from something that you really shouldn't port.
The port is trying to make ROLE into JOB codes, to merge ROLE and JOB codes into one. This isn't a good refactor, only increases the complexity of the code to maintain.

@EvilDragonfiend
Copy link
Member

EvilDragonfiend commented Jun 29, 2024

🔑With the reason said above, I put my key.

Why Role and Job shouldn't be merged?

Simple. This means roles like Heretic, Traitor, something else as a job, while crews should have a job with such special role.
Many roles shouldn't be a job, but the code makes them as a job. (I know heretic, traitor isn't included here, but we really shouldn't have others as job.)

What should I do port these correctly?

I really recommend posting a port PR one by one rather than trying to bring everything here.

@EvilDragonfiend EvilDragonfiend added the 🔑 Close Key (1/3) Indicates that someone has requested this PR to be keyed label Jun 29, 2024
@XeonMations
Copy link
Contributor Author

XeonMations commented Jun 29, 2024

Currently all given changes were purely from tgstation/tgstation#59841 so far, as requested/reverting reverted all changes that merged roles and jobs.

@PowerfulBacon
Copy link
Member

PowerfulBacon commented Jun 29, 2024

Getting rid of special role is a good idea, does this mean that assigned_role is a list now so people can have multiple job role at one?

Job refactor 1 is a good PR, using strings in GetJob is kinda silly and typepaths are way better. Job refactor 2 looks good too.

@EvilDragonfiend
Copy link
Member

EvilDragonfiend commented Jun 29, 2024

Getting rid of special role is a good idea, does this mean that assigned_role is a list now so people can have multiple job role at one?

This PR doesn't do that.

Job refactor 1 is a good PR, using strings in GetJob is kinda silly and typepaths are way better. Job refactor 2 looks good too.

typepath check is not any better. when you can just do assigned_role.title == JOB_NAME_CAPTAIN instead of is_job_captain(job), I don't think having N size of is_job_this() macro should be a thing.
I don't mind changing assigned_role into var/datum/job/assigned_role, but what I was addressing isn't related with it.

@EvilDragonfiend
Copy link
Member

Lifting the key since the majour concern has been resolved.

@EvilDragonfiend EvilDragonfiend removed the 🔑 Close Key (1/3) Indicates that someone has requested this PR to be keyed label Jun 29, 2024
@github-actions github-actions bot added the TGUI-Changes Contains changes to TGUI. Make sure its up to date with TGUI 4.0 label Jun 29, 2024
@XeonMations
Copy link
Contributor Author

49 merge conflicts later....

Copy link

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

@XeonMations
Copy link
Contributor Author

ARE YOU KIDDING ME

@XeonMations XeonMations marked this pull request as ready for review September 1, 2024 09:13
@XeonMations XeonMations requested a review from itsmeow as a code owner September 1, 2024 09:13
Copy link

github-actions bot commented Sep 4, 2024

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

@XeonMations XeonMations marked this pull request as draft September 21, 2024 00:40
@XeonMations
Copy link
Contributor Author

okay this entire PR is a shitshow, im pretty sure starting from scratch will be better than... sorting this mess now that #11209 is in

@XeonMations XeonMations deleted the crew-manifest-refactor branch September 21, 2024 04:08
@XeonMations
Copy link
Contributor Author

Will be continuing this in a clean PR.

@XeonMations
Copy link
Contributor Author

nevermind, time for actual TGUI i guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Administration Code Improvement Feature Mapping DMM Change Refactor Sound Sprites TGUI-Changes Contains changes to TGUI. Make sure its up to date with TGUI 4.0 Tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants