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

Adds Positronic Brains to Latejoin Menu #11162

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

Programs-The-Station
Copy link
Contributor

@Programs-The-Station Programs-The-Station commented Jul 7, 2024

About The Pull Request

This PR Adds On Station Positronics to the Latejoin menu. A player at the lobby will activate a random positronic from the available ones on station.

It was going to be extremely difficult to separate round-start cyborg from positronics, so I left the ability for one late join cyborg to occur and added positronics as a separate category.

Posis created on station and moved off station will automatically be removed from the list of candidates.

Posis created off station and moved on station will automatically join the list.

Posis deleted / exploded will automatically remove themselves from the list if applicable.

Closes Request #9133 .

Why It's Good For The Game

You can now latejoin as a posibrain directly.

Testing Photographs and Procedure

Starting Test Case ![Before On Station Posis](https://github.com/BeeStation/BeeStation-Hornet/assets/100493881/71451b8e-20ec-4d89-8f6a-ea0256f22b27)

On Station Posi 1

3 Posis Created On Station: Total Jobs: 3

3 Posis On Station

Posi Created Off Station (Mining), no change in job slots.

4_Posi created off station no change

That Posi Being Brought On Station: Total Jobs: 4

5_Posi Created off Station, brought on station

Positronic Job Now Available in Latejoin Menu

6_Positronic Job Now Available

Joining From Latejoin

7_From Latejoin

Joining As Ghost

8_Ghost Posi Still Works

All Latejoin Tests

9_Posi Latejoin 1

10_Posi Latejoin 2

11_Posi Latejoin 3

12_Posi latejoin 4

No More Posis After Latejoining

13_No More posis

Deleting and Exploding Posis Removes Them From the List (Started with 2 in each case)

14_Deleting Posi Removes from List

15_Exploding Posi removes it from list

Changelog

🆑
add: You can now Latejoin directly as an on-station posibrain.
add: Due to Job-code, Positronic Brain is now a job.
/:cl:

@Programs-The-Station Programs-The-Station marked this pull request as draft July 7, 2024 03:51
@Programs-The-Station Programs-The-Station marked this pull request as ready for review July 7, 2024 04:02
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.

It should be a subtype of borg job, so that you don't have to touch many things manually. (since it shares the same things with borg job)

Copy link
Contributor

@Rukofamicom Rukofamicom left a comment

Choose a reason for hiding this comment

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

In order to actually fulfil #9133 it should replace cyborg as a job on latejoin entirely.

Cyborg shouldn't still be allowed the single late join, or any late joins if a cyborg goes to cryo.

@MarkusLarsson421
Copy link
Contributor

Having one single Cyborg slot is needed still I think. If there is no Roboticist or Scientist there will be zero Cyborgs no matter how many positronic brains you have.

@Programs-The-Station
Copy link
Contributor Author

In order to actually fulfil #9133 it should replace cyborg as a job on latejoin entirely.

Cyborg shouldn't still be allowed the single late join, or any late joins if a cyborg goes to cryo.

The problem with this logic is that implies that an AI that cryos should not open up a new slot, because you can just create more AIs.

Because of how Cyborgs are spawned vs the positronic activation, I'd have to remove the roundstart cyborg completely, or rework large amounts of the job code to pass in additional snowflake parameters.

The best I can do is set the Cyborg total positions to zero, and the spawn positions to 1, but I do not know if that will allow for a cyborg to spawn round start. I'd have to test it.

I'm not going to combine the two, but I can look at making Posi a subtype of Cyborg. The reason I didn't is because the "job" posibrain is just a workaround to get the jobs subsystem to recognize that they exist. It shouldn't exist in any consoles. Handling bans and such are already handled in the positronic code, so there's no reason to tie them to the job.

I think Bacon should decide if this fulfills #9133 or not.

@Rukofamicom
Copy link
Contributor

Rukofamicom commented Jul 7, 2024

I think Bacon should decide if this fulfills #9133 or not.

image

I would argue it's already very explicitly part of that link, but yes he may have changed his mind and should probably weigh in himself.

Having one single Cyborg slot is needed still I think. If there is no Roboticist or Scientist there will be zero Cyborgs no matter how many positronic brains you have.

There should be zero positronic brains without a roboticist to print them. That's the intent behind the change - cyborgs are introduced by roboticists and should only really be a thing at either roundstart or as a part of robotics?

@MarkusLarsson421
Copy link
Contributor

Wasn't there supposed to be a round-start positronic brain? Or are cyborgs just locked behind miners now?

@Programs-The-Station
Copy link
Contributor Author

Update: Setting Cyborg Total Positions to 0 allowed for 1 roundstart cyborg ( 1 spawned position). The cyborg cryoing did not open up a slot.

I'm not going to push this change until a decision is reached on whether or not we want the ability to have one late join cyborg.

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.

You need to make a proc /datum/job/cyborg/proc/spawn_silicon(...), and should call it from proc/after_spawn()
/datum/job/cyborg/after_spawn() codes should go to the new proc spawn_silicon()
positron brain should override the proc.

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.

Try to use onTransitZ() proc to make it more accurate.

@Programs-The-Station
Copy link
Contributor Author

You need to make a proc /datum/job/cyborg/proc/spawn_silicon(...), and should call it from proc/after_spawn() /datum/job/cyborg/after_spawn() codes should go to the new proc spawn_silicon() positron brain should override the proc.

So you want me to rewrite cyborg code as well? There's no proc like that, not even in the AI stuff. Both AI and Cyborg use the "equip" function to do the transformation from human to silicon.

@EvilDragonfiend
Copy link
Member

EvilDragonfiend commented Jul 8, 2024

You need to make a proc /datum/job/cyborg/proc/spawn_silicon(...), and should call it from proc/after_spawn() /datum/job/cyborg/after_spawn() codes should go to the new proc spawn_silicon() positron brain should override the proc.

So you want me to rewrite cyborg code as well? There's no proc like that, not even in the AI stuff. Both AI and Cyborg use the "equip" function to do the transformation from human to silicon.

No, it isn't asking you to rewrite the whole code. it's just simple. You need to do a simple touch.

/datum/job/cyborg/after_spawn(mob/living/silicon/robot/R, mob/M, latejoin = FALSE, client/preference_source, on_dummy = FALSE)
	if(!M.client || on_dummy)
		return
	after_spawn_silicon(R)

/datum/job/cyborg/proc/after_spawn_silicon(mob/living/silicon/robot/R)
	R.updatename(M.client)
	R.gender = NEUTER

/datum/job/cyborg/posibrain/after_spawn_silicon(mob/living/silicon/robot/R)
	// that you wrote in this PR

job code is miserable as fuck, but you don't have to refactor that much.

Copy link
Contributor

@Rukofamicom Rukofamicom left a comment

Choose a reason for hiding this comment

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

Got a response from bacon who is having Internet issues - yes the latejoin cyborg should be removed as per the initial posting.

They are either already part of the station or must be built by a roboticist (or someone standing in as one)

@Programs-The-Station
Copy link
Contributor Author

All requests updated.

The reason for my second comment on the after_spawn_silicon is that I just needed clarification if you wanted me to add that proc to Cyborg.dm, which your example was helpful.

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.

position shpuld take a length of the list, instead of doing ++ or --. like SSjob.GetJob(JOB_NAME_POSIBRAIN).total_positions = length(GLOB.posibrains)

also, Byond can't do
SSjob.GetJob(JOB_NAME_POSIBRAIN).extra_access
so you need to do
var/datum/job/job = getjob blabla and then do job.position = 1234

i am outside. sorry for poor review

Copy link
Contributor

@Rukofamicom Rukofamicom left a comment

Choose a reason for hiding this comment

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

Latejoin cyborgs resolved

code/modules/mob/living/brain/posibrain.dm Outdated Show resolved Hide resolved
code/modules/mob/living/brain/posibrain.dm Outdated Show resolved Hide resolved
code/modules/mob/living/brain/posibrain.dm Outdated Show resolved Hide resolved
code/game/machinery/fabricators/modular_fabricator.dm Outdated Show resolved Hide resolved
Copy link
Contributor

@tontyGH tontyGH left a comment

Choose a reason for hiding this comment

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

I think it just being positronic brains with 0 additional context is a very bad idea, since a posibrain being created doesn't necessarily mean that someone is available to insert it.

If we want to replace latejoin cyborgs, what should be done is making something similar to latejoin AI cores, which players create by installing vacant positronic brains into cyborg shells. Or something like that.

@MarkusLarsson421
Copy link
Contributor

MarkusLarsson421 commented Jul 11, 2024

It does feel weird. As if you are joining the game, you have a risk of not actually playing the game and possibly ghosting as you have to wait up to 30 minutes for the simple task of being inserted into a shell. I liked Tontys' idea of inserting inactive brains into shells in order to make them pop up on the menu instead. Then, you might not need the off-station check.

However, this can make antagging as a Roboticist slightly harder if you want to be around to emag a cyborg when it comes alive. Perhaps a cyborg should be emaggable ahead of time? That would also allow non-Roboticists to easily get a cyborg companion.

Copy link
Member

@PowerfulBacon PowerfulBacon left a comment

Choose a reason for hiding this comment

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

Since after_spawn doesn't expect the passed mob to be deleted, you should instead do what cyborg does and modify the player's mob inside of equip(). Instead of deleting the cyborg, override equip(), delete the passed mob and return the positronic barin instead.

Comment on lines 13 to 25
var/obj/item/mmi/posibrain/P = pick(GLOB.on_station_posis)

//Never show number of current posis
current_positions = 0

if(!P.activate(R)) //If we failed to activate a posi, kick them back to the lobby.
//Code taken from "send to lobby" admin panel
var/mob/dead/new_player/NP = new()
NP.ckey = M.ckey

to_chat(NP, "<span class='warning'>Failed to Late Join as a Posibrain. Look higher in chat for the reason.</span>")

qdel(R)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of picking a random one, loop through all of them until you find a valid one and then return when P.activate passes successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

90 percent of failures will be because the user is either:

  1. Banned from Silicon Roles
  2. Too new
  3. 1 Posi per round (which shouldn't happen if they are in lobby)
  4. Ghost spawns limited (very rare)
  5. Suicided (shouldn't happen if they are in lobby)

The only one that will be different is if at the exact instance they click the join button, an in round ghost clicks "Yes" on the posi prompt (which you don't get joining from lobby), and there is only one posi/the random chance matches.

I'd have to refactor activate to return error codes on failure to kick out early, instead of spamming the chat for every posibrain that it tries. Do you want me to do that?

Copy link
Member

Choose a reason for hiding this comment

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

You just have to return when it is successful, if it throws an error message in the chat then you would have hit a bugged state anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You just have to return when it is successful, if it throws an error message in the chat then you would have hit a bugged state anyway.

Activate returns a message in chat telling you why you did not spawn as a posibrain. The errors are anywhere from too new to banned to someone already grabbed it.

If I loop over all of the available posibrains, and someone is banned from posibrain, it will say:
"Banned from Posibrain"
"Banned from Posibrain"
"Banned from Posibrain"
"Banned from Posibrain"
"Banned from Posibrain"
"Banned from Posibrain"
"Banned from Posibrain"
"Banned from Posibrain"
"Banned from Posibrain"

"Failed to spawn, check above for reason"

This happens because that chat message exists in activate, where it needs to be so that ghost activations still work properly.

So, I'd have to add a return code that is more specific than "failure" so the loop can terminate early.

Programs-The-Station and others added 5 commits July 14, 2024 09:54
Co-authored-by: PowerfulBacon <26465327+PowerfulBacon@users.noreply.github.com>
Co-authored-by: PowerfulBacon <26465327+PowerfulBacon@users.noreply.github.com>
Co-authored-by: PowerfulBacon <26465327+PowerfulBacon@users.noreply.github.com>
@Programs-The-Station
Copy link
Contributor Author

Since after_spawn doesn't expect the passed mob to be deleted, you should instead do what cyborg does and modify the player's mob inside of equip(). Instead of deleting the cyborg, override equip(), delete the passed mob and return the positronic barin instead.

Resolved in latest commit (I can't "resolve" this because it's just a generic comment and not a review)


to_chat(NP, "<span class='warning'>Failed to Late Join as a Posibrain. Look higher in chat for the reason.</span>")
qdel(H)
return NP
Copy link
Member

Choose a reason for hiding this comment

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

Have you checked to see the consequences of returning a new player in the equip chain? This could cause some seriously unintended behaviours since the job equip code isn't made to handle not being able to equip afaik

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested it by disabling ghost roles, and it doesn't seem to impact things too badly (it does work properly). One side effect is that it leaves a "new player" atom (I think) on the spawn space, but I can't open the view variables tab to check what type it is. I can't interact with it at all.

@EvilDragonfiend
Copy link
Member

I am sorry to say, but it looks having this as cyborg subpath seems to be a bad idea. Please revert it to have its own path.

@ColonelOrion
Copy link
Contributor

@Rukofamicom this looks like a great addition as is. Why let perfect get in the way of good?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants