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] Crashed room despawn #1451

Merged
merged 2 commits into from Sep 24, 2018

Conversation

Projects
None yet
2 participants
@mugmuggy
Copy link
Contributor

mugmuggy commented Sep 1, 2018

Addresses #1429.

One additional change here was to remove the humanoidDeath call from Staff:die() as staff deaths didn't count to death count in TH or affect reputation (room being destroyed did).

There is one outstanding issue which might contribute (or not) to some bugs, is that Room:crashRoom calls World:destroyEntity, which removes multiple entities. From my understanding that can result in skipped tick calls as calling table.remove from within an iteration from ipairs is bad.

Ensure despawn in room crashes
Moved Staff:despawn call to crashRoom (so we can call it for all humanoids, for the benefit of patients)
Removed humanoidDeath call for staff deaths (they don't contribute to reputation/death counts in Theme Hospital)
Flag to indicate dead staff - prevents Staff:tick from causing crashes
self.world:destroyEntity(humanoid)
end

-- Remove all humanoids in the room
for humanoid, _ in pairs(self.humanoids) do
remove_humanoid(humanoid)
end
self.humanoids = nil

This comment has been minimized.

@TheCycoONE

TheCycoONE Sep 6, 2018

Member

nit pick, but I think I'd prefer you set this back to the empty set rather than nil, similar to what happens to humanoids_enroute in deactivate

@mugmuggy mugmuggy changed the title [RFC] Crashed room despawn [RDY] Crashed room despawn Sep 11, 2018

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Sep 14, 2018

destroyEntity loop is safe because it breaks out of the loop immediately on delete.

Humanoid and findAllObjects loops in crashRoom are safe. Not over entities.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Sep 14, 2018

Earthquake case is safe all the way up (looping over room objects not entities)

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Sep 14, 2018

Action case is not safe, action calls machineUsed, called inside ipairs loop over entities in world:onTick

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Sep 14, 2018

I will create a new issue. crashRoom shouldn't happen in machineUsed, it should happen in a new loop in world near the end of tick()

Edit: for a less impactful fix, destroyEntity could just mark entities as destroyed and the actual cleanup done later in World.

@mugmuggy

This comment has been minimized.

Copy link
Contributor

mugmuggy commented Sep 14, 2018

Yep, I didn't update the issue, but I realise I made a mistake in that - the staff:despawn was being called - but it was the Staff:tick and the action being called that was stuffing it all up.

I haven't been able to find an obvious issue with destroyEntity, ie litter removal is also called from an action and happens all the time but we are probably only talking about 1 or 2 animation ticks when they are skipped because of that removal.

@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Sep 14, 2018

If the destroyed entity is earlier in the entity list than the current entity, or is the current entity, then the next entity in the list will have it's tick() skipped. This cascades for every removal, so if 5 entities are removed (<= current), five will be skipped that loop through. Removed entities ahead of the current one cause no effect.

ipairs is basically equivalent to for (i = 1; table[i] != nil; i++) { res = table[i]; ... }, and table.remove(t, x) does something like for (i = x; table[i] != nil; i++) { table[i] = table[i + 1] } (excuse the C style notation)

An additional benefit to doing all the removals at once is that we can use a single pass (worse case order N, instead of order N^2)

@mugmuggy

This comment has been minimized.

Copy link
Contributor

mugmuggy commented Sep 14, 2018

I've done 3 solutions to this problem, as I noticed it before but got side tracked. I implemented a fix to change destroyEntity to set e.destroyed = true and processed in a loop after the enitity:tick() avoiding the 'tick' also within that loop.

And I believe with the additional check in entity:tick anyway, it would be just as good to process self.entities in reverse.

  for i = #self.entities, 1, -1 do
    if self.entities[i].destroyed then
      table.remove(self.entities, i)
    elseif self.entities[i].ticks then
      self.current_tick_entity = self.entities[i]
      self.entities[i]:tick()
    end
  end
@TheCycoONE

This comment has been minimized.

Copy link
Member

TheCycoONE commented Sep 14, 2018

local destroyed_entities = 0
local entity_count = #self.entities
for i = 1, entity_count, 1 do
  if self.entities[i].destroyed then
    destroyed_entities += 1
    self.entities[i] = nil
  else
    self.entities[i - destroyed_entities] = self.entities[i]
  end
end

@TheCycoONE TheCycoONE merged commit 309460a into CorsixTH:master Sep 24, 2018

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