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

[RDY] Research fax options updating #1484

Merged
merged 4 commits into from Apr 20, 2019

Conversation

Projects
None yet
2 participants
@mugmuggy
Copy link
Contributor

commented Oct 12, 2018

When looking at the autospy discovery issue I raised, I also noted inconsistencies with seek treatment room fax as mentioned in #453. Some of these are documented, some are differences from the original (not necessarily desired though), and there are probably more better ways to perform the same functions.

Patients seeking a treatment room fax doesn't 'exist' for the 'need_to_employ' message when room exists but not the required staff - #469
Patients seeking a treatment - seek the room when unstaffed - related to above (this change compromises on the diagnostic rooms) and patients will not attempt to queue until a staff member exists in the hospital
Research button disabled when the treatment room is discovered when it should remain enabled (research and researcher exist)
Interrupted seeks - don't remove message
Fax updates dynamically depending on the state

Just created for some more discussion

@@ -166,6 +166,7 @@ function UIFax:choice(choice_number)
owner:goHome("over_priced", owner.disease.id)
end
elseif choice == "research" then
owner:unregisterCallbacks()
owner:setMood("idea", "activate")
owner:setNextAction(SeekRoomAction("research"))
end

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

This change I believe fixes the last issue in #133. Believe in this issue the callback may still be registered so when the edited room exists again the callback was executed requeuing the seek to the room. Yet to attempt to verify

if self.onClose then
self:onClose(false)
self.onClose = nil
end
self:close()

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

Calling the bottom_panel:removeMessage calls UIMessage:removeMessage twice and the onClose is nil.

end
self.message[3].text = output_text
else

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

Recalculating message content, only issue here is I need to validate if getRoomNameAndRequiredStaffName will return a ward message, if checkDiseaseRequirements returns just it, if op theatre exists, I just can't recall seeing a fax with 'ward' and nurse on it. The staffclass_to_string was just taken from some other code

-- update all messages for waiting patients
for _, patient in pairs(self.patients) do
patient:notifyOfStaff(nil)
end
end

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

The callbacks for hiring and firing.

@@ -48,7 +48,7 @@ function SeekRoomAction:setDiagnosisRoom(room)
return self
end

local function action_seek_room_find_room(action, humanoid)
local action_seek_room_find_room = permanent"action_seek_room_find_room"( function(action, humanoid)
local room_type = action.room_type

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

Believe I had to do this for the callbacks to persist correctly

else
output_text = strings.need_to_build_and_employ:format(room_name, staff_name)
output_text = strings.need_to_employ:format(staff_name)
end

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

Like Patient:updateMessage except we have a room type to get the staff string for this time.

action_seek_room_goto_room(room, humanoid, action.diagnosis_room)
return
end
end

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

Restructuring the code a bit here as one of the 'differences' between corsix and th. Seek diagnosis or research rooms, and remove the message if it exists (this cleans up interrupted seeks)

action_seek_room_goto_room(room, humanoid, action.diagnosis_room)
return
end
end

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

Similar logic for treatment as diagnosis, just checking if we alos have outstanding treatment requirements

end -- End of remove_callback function
action.remove_callback = remove_callback
humanoid:registerRoomRemoveCallback(remove_callback)

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

If the room exists and it crashes or edited the room or deleted the room - we also want to update the message so removed the research check.

end
end
end -- End of staff_callback function
action.staff_callback = staff_callback

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Oct 12, 2018

Author Contributor

Callback for when a staff member is hired/fired to update the message will also enable the Research button on the fax, not just the change in message

@TheCycoONE

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

@mugmuggy what is the state of this PR? Ready to check?

@mugmuggy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

I can recall testing each of the individual fax changes, by removing/adding rooms and hiring/firing staff. There are a few minor things, like a commented out assert that should probably just be removed, and the other concern I had at the time was around translated strings which I didn't switch languages on to check either.

And I included the unregister callback warnings, which makes for ugly game logs when executed which I think is the reason I haven't marked as RFC/RDY as yet. Getting a review on the current implementation and I'll rebase, fix app version, the commented assert and run it through some tests again.

@mugmuggy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

I had a play around with the original to see what it did. If you have an op theatre and no ward, and a patient is diagnosed with say Spare Ribs. The fax says, You cannot cure this disease at the moment.. Whereas, with this change it will Ward and Nurse (evn though you have a nurse), so its a little confused in the current state so I'll try and address that.

@mugmuggy mugmuggy force-pushed the mugmuggy:researchseeking branch 3 times, most recently from d3feed0 to e4f0479 Mar 21, 2019

@mugmuggy mugmuggy changed the title Research fax options updating [RFC] Research fax options updating Mar 21, 2019

@@ -300,6 +300,7 @@ function Humanoid:Humanoid(...)

self.build_callbacks = {--[[set]]}
self.remove_callbacks = {--[[set]]}
self.staff_callbacks = {--[[set]]}

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Apr 3, 2019

Member

I see, they are used on patients to notify about staff changes...

Can we get a more descriptive name?
Maybe separate callbacks for hiring and firing?

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Apr 4, 2019

Author Contributor

The only difference between a hire/fire is the room checking is technically not needed after updating the message if you have sacked anyone as the message will be updated to reflect the state. Could rename to hospital_staff_change_callbacks.

action.staff_callback = staff_callback
humanoid:registerStaffCallback(staff_callback)

build_callback = --[[persistable:action_seek_room_build_callback]] function(rm)

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Apr 3, 2019

Member

Is a similar callback needed for destroyed rooms?

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Apr 4, 2019

Author Contributor

remove_callbacks does this for updating a message. Once a room exists and the patient is on way to the room (via the build_callback if required), destroying the room will start a new SeekRoomAction and the reallocation of the callbacks will occur.

@@ -28,7 +28,7 @@ local runDebugger = corsixth.require("run_debugger")
-- Increment each time a savegame break would occur
-- and add compatibility code in afterLoad functions

local SAVEGAME_VERSION = 132
local SAVEGAME_VERSION = 133

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Apr 3, 2019

Member

This will need to be bumped again.

@@ -337,11 +338,15 @@ function Humanoid:afterLoad(old, new)
if old < 83 and self.humanoid_class == "Chewbacca Patient" then
self.die_anims.extra_east = 1682
end
if old < 133 then

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Apr 3, 2019

Member

Version bump needed

--!param callback (function) The callback to call when a staff member has been hired or fired
function Humanoid:registerStaffCallback(callback)
if self.staff_callbacks and not self.staff_callbacks[callback] then
self.staff_callbacks[callback] = true

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Apr 3, 2019

Member

This is peculiar and prevents ordering callbacks so execution order is random. A classic array is preferred.

This comment has been minimized.

Copy link
@mugmuggy

mugmuggy Apr 4, 2019

Author Contributor

I just copied the existing callback related functions that do the same. To me in theory, it looks like we only should ever have one call back at a time registered for each type, but I haven't checked that specifically but the individual action callbacks are unregistered, so its when each callback is satisfied they should unregister themselves, but then you only 'seek one room at a time' - ward and operating theatre being special for treatment. Difference here is that for diagnosis rooms you might have a list of possible diagnosis rooms to try, and you'd get another callback for another SeekRoomAction but I'd have to check that too.

Maybe it was envisioned as having a list of actions that were toggled for activity for the life of the patient rather than recreating them.

Though I can see in issue #1337 for example, where a callback for a GPs office exists, when GPs offices already exist. In that particular issue, many patients get redirected to the newly built room, as the build callbacks exist, whereas room:tryToFindNearbyPatients should probably be called instead or the callback not exist at all.

@TheCycoONE

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

Is there much more to do on this mugmuggy? It seems better than now already, I'd like to put it in the release.

@mugmuggy mugmuggy force-pushed the mugmuggy:researchseeking branch 2 times, most recently from 20a4147 to 7028167 Apr 20, 2019

@mugmuggy mugmuggy force-pushed the mugmuggy:researchseeking branch from 7028167 to bc2340a Apr 20, 2019

@mugmuggy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

I updated based on some feedback and found another edge case that trainee doctors would cause the fax not to update. I now have them updating the messages when any special skill is added, however its only effective on surgeon skills because you need two.

@mugmuggy mugmuggy changed the title [RFC] Research fax options updating [RDY] Research fax options updating Apr 20, 2019

@TheCycoONE TheCycoONE merged commit 635a71b into CorsixTH:master Apr 20, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.