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] Remove handyman task upon sweep action start #1286

Merged
merged 1 commit into from Nov 29, 2017

Conversation

Projects
None yet
2 participants
@mugmuggy
Contributor

mugmuggy commented Nov 25, 2017

Litter not 'tickable' as in to interrupt Handyman tasks when Litter:remove on build over.

I didn't update Litter:remove() to enforce the ticks = false, just left that to new litter only.

Removing the task in the action_sweep_floor_start is similar to Plant:restoreToFullHealth called from the end of action_use_object_start.
Hopefully addresses issues in #1155.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 25, 2017

Member

I looked into the history of

if task.object.ticks ~= true then
and found this curiosity:

index 94be3f83..c0bf692e 100644
--- a/CorsixTH/Lua/hospital.lua
+++ b/CorsixTH/Lua/hospital.lua
@@ -1513,7 +1513,7 @@ function Hospital:removeHandymanTask(taskIndex, taskType)
     local task = subTable[taskIndex]
     table.remove(subTable, taskIndex)
     if task.assignedHandyman then
-      if taskType ~= "cleaning" and task.object.ticks ~= true then
+      if task.object.ticks ~= true then
         task.assignedHandyman:intreruptHandymanTask()
       end
     end

Which effectively is the same thing that you are doing by disabling ticks on litter. There was no indication in the commit as to why they removed that test.

I don't like that the purpose of ticks was overridden like this - it's suppose to only indicate when we can call enity.onTick() - but the use of ticks for handyman tasks predates your PR by years so I will let it go in favour of a future refactor.

Otherwise, I was concerned about the action getting disrupted, and litter being abandoned, but it seems that the must_happen takes care of that.

I'm still concerned about the state deviation between old saves and new litter tasks. I means a code cleanup in the future could easily lead to broken state with little clue as to why. Should be able to fix that with an afterLoad on staff.lua that checks humanoid_class, and user_of and removes the task for any litter found?

Member

TheCycoONE commented Nov 25, 2017

I looked into the history of

if task.object.ticks ~= true then
and found this curiosity:

index 94be3f83..c0bf692e 100644
--- a/CorsixTH/Lua/hospital.lua
+++ b/CorsixTH/Lua/hospital.lua
@@ -1513,7 +1513,7 @@ function Hospital:removeHandymanTask(taskIndex, taskType)
     local task = subTable[taskIndex]
     table.remove(subTable, taskIndex)
     if task.assignedHandyman then
-      if taskType ~= "cleaning" and task.object.ticks ~= true then
+      if task.object.ticks ~= true then
         task.assignedHandyman:intreruptHandymanTask()
       end
     end

Which effectively is the same thing that you are doing by disabling ticks on litter. There was no indication in the commit as to why they removed that test.

I don't like that the purpose of ticks was overridden like this - it's suppose to only indicate when we can call enity.onTick() - but the use of ticks for handyman tasks predates your PR by years so I will let it go in favour of a future refactor.

Otherwise, I was concerned about the action getting disrupted, and litter being abandoned, but it seems that the must_happen takes care of that.

I'm still concerned about the state deviation between old saves and new litter tasks. I means a code cleanup in the future could easily lead to broken state with little clue as to why. Should be able to fix that with an afterLoad on staff.lua that checks humanoid_class, and user_of and removes the task for any litter found?

@mugmuggy

This comment has been minimized.

Show comment
Hide comment
@mugmuggy

mugmuggy Nov 25, 2017

Contributor

For the ticks thing, I should have set it correctly as
self.ticks = object_type.ticks - which is false - like all other Objects do

function Object:Object(world, object_type, x, y, direction, etc)
local th = TH.animation()
self:Entity(th)
if etc == "map object" then
if direction % 2 == 0 then
direction = "north"
else
direction = "west"
end
end
self.ticks = object_type.ticks
self.object_type = object_type

A quick dive into the history and the context I believe is the 'soot' in a crash room was invoking an addHandymanTask call in the litter constructor. The afterLoad version < 54 check was added to remove the existing tasks for soot in crashed rooms, calling removeHandymanTask and in doing so, to interrupt handyman assigned to clean soot in rooms, the ~= "cleaning" was removed, with the upside it should fix the tasks, but alas I don't think it was due to the Entity constructor setting it ticks to true.

Yep currently must_happen prevents disrupting the task.

For cleaning up tasks already assigned, the minimum required is, but not necessarily providing coverage to potentially all edge cases litter afterLoad.
if old < 121 then
self.ticks = object.ticks
end
As Litter:remove() still needs to call removeHandymanTask for when placing objects over litter, there is cleanup being done still there and that will remove the handyman task also. I'll look into the staff change to remove the task earlier as there could be edge cases I'm just not hitting.

Contributor

mugmuggy commented Nov 25, 2017

For the ticks thing, I should have set it correctly as
self.ticks = object_type.ticks - which is false - like all other Objects do

function Object:Object(world, object_type, x, y, direction, etc)
local th = TH.animation()
self:Entity(th)
if etc == "map object" then
if direction % 2 == 0 then
direction = "north"
else
direction = "west"
end
end
self.ticks = object_type.ticks
self.object_type = object_type

A quick dive into the history and the context I believe is the 'soot' in a crash room was invoking an addHandymanTask call in the litter constructor. The afterLoad version < 54 check was added to remove the existing tasks for soot in crashed rooms, calling removeHandymanTask and in doing so, to interrupt handyman assigned to clean soot in rooms, the ~= "cleaning" was removed, with the upside it should fix the tasks, but alas I don't think it was due to the Entity constructor setting it ticks to true.

Yep currently must_happen prevents disrupting the task.

For cleaning up tasks already assigned, the minimum required is, but not necessarily providing coverage to potentially all edge cases litter afterLoad.
if old < 121 then
self.ticks = object.ticks
end
As Litter:remove() still needs to call removeHandymanTask for when placing objects over litter, there is cleanup being done still there and that will remove the handyman task also. I'll look into the staff change to remove the task earlier as there could be edge cases I'm just not hitting.

Remove handyman task upon sweep action start
Litter not 'tickable' as in to interrupt Handyman tasks when Litter:remove on build over

Remove handyman tasks when already user_of litter
Corrected the assignment of the Litter.ticks to passed in object.
@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 26, 2017

Member

lgtm

Member

TheCycoONE commented Nov 26, 2017

lgtm

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE
Member

TheCycoONE commented Nov 29, 2017

RDY @mugmuggy?

@mugmuggy mugmuggy changed the title from [RFC] Remove handyman task upon sweep action start to [RDY] Remove handyman task upon sweep action start Nov 29, 2017

@mugmuggy

This comment has been minimized.

Show comment
Hide comment
@mugmuggy

mugmuggy Nov 29, 2017

Contributor

Yup

Contributor

mugmuggy commented Nov 29, 2017

Yup

@TheCycoONE TheCycoONE merged commit 7c8fdab into CorsixTH:master Nov 29, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment