Delay entity garbage collection

[tobylane]

Fixes #1467

Describe what the proposed change does

• Mark entities for deletion where they were previously deleted, and delete them in the next tick
• Don't run tick functions for entities that have been marked for deletion. This may be optional, I had an error I can't recreate that made me think of this.

[lewri]

Queries:

• Is staff not an entity that requires addressing?
• It may be better to go one level higher, prevent the entity:tickDay() in World being called at all. To make it nice you'll need a helper in Entity e.g.

function Entity:isMarkedForDeletion()
  return self.delete
end

Then, you'd want to insert that check somewhere neatly in World:onEndDay(). I tried myself to think of a way to avoid unnecessary nesting but wasn't able to. You may have to move the contents inside the entity for loop to a local function or something. (This would also cover resolve the first query by allowing any entity to have its deletion delayed if needed).

I would generally be on the side that as much of this as possible should remain in control of the entity class and not leak to its children.

[tobylane]

https://github.com/CorsixTH/CorsixTH/blob/master/CorsixTH/Lua/entities/humanoids/staff.lua#L43

Staff (and doctor) have their parent class run tickday first, and staff:tick runs entity:tick first, so they don't need extra handling.

Good point, I'll experiment with world:onendday.

[Alberth289346]

I am not sure what you're trying to achieve here, but I doubt this change will do what you may think it does.

An (entity) object becomes garbage only when ALL its references have ceased to exist. Thus all variables in actions, rooms, queues etc must not point to the (entity) object before it's considered garbage. self.patients is thus just one spot rather than THE spot.
If you want to fix that, likely it should be much more difficult to grab an entity. (That is, one spot where you can query it, and strict rules that you cannot store entity references in persistent data.) Probably the only way to achieve that is by moving entity data into CPP, so you can't access anything without going through the Lua-CPP bridge.

---

However, your PR got me thinking if Lua handles deletion during iterating. That is

a = {}
for i = 1, 5 do a[i] = i end

for k, v in ipairs(a) do
  print(v)
  if v == 2 then table.remove(a, k) end
end

This code walks over the table, and removes the 2nd entry. If lua handles it, you'd expect output 1, 2, 3, 4, 5. In reality (Lua 5.3.6) it prints 1, 2, 4, 5.
Java would probably throw an error here to warn about it, but Lua apparently doesn't :(

Maybe you're solving your problem at a wrong spot?

---

As for the approach: We have self.gohome and self.dead, and you're adding self.delete. All these are mutually exclusive states of the entity. Why not have a self._state with different values instead of adding more boolean variables?

[Alberth289346]

For a completely different approach wrt self.patients:
I have been reading in libgdx, a Java game engine. One of the things they do there is avoiding garbage collection as much as possible. The reason for that is apparently that mobiles are very slow in handling garbage collection so they are pushing towards not creating garbage as much as possible.
Obviously that idea can be applied here as well. "Just" keep deleted patients in the list, and re-use them for the next spawned patient. I am not sure that will fly currently, as we don't even know what variables may exist in a patient and creating a new clean instance may thus prove to be less than trivial.

[tobylane]

With the limitations of Lua and the need to run entity tick functions in numerical order, I think this is the right way to do it.

[lewri]

Have you verified entities returns to iterative after this process? (so ipairs can be used again)

[Alberth289346]

local entries = {{delete=true}, {delete=true}, {delet=true}, {delete=true}}
for i, entry in pairs(entries) do
  if entry.delete then
    table.remove(entries, i)
  end
end

local count = 0
for x, y in pairs(entries) do count = count + 1 end
print(count)

prints "2" here, which is incorrect.

You simply cannot safely delete entries from a table while iterating over it.

[lewri]

It should return 1 since there's a typo {delet=true} =]
But yes, looks like even pairs isn't safe from this.

[tobylane]

I see. Option a is to move through entries backwards.

local entries = {{delete=true}, {delete=true}, {delete=true}, {delete=true}}
for n = #entries,1,-1 do
  if entries[n].delete then
    table.remove(entries, n)
  end
end

local count = 0
for x, y in pairs(entries) do
    count = count + 1
end
print(count)

Option b is the function we looked at in discord from https://stackoverflow.com/a/53038524

[lewri]

@Alberth289346 what about this?

local entries = {{delete=true}, {delete=true}, {delet=true}, {delete=true}}
for i=#entries, 1, -1 do
  if entries[i].delete then
    table.remove(entries, i)
    if #entries == 0 then break end
  end
end

local count = 0
for x, y in pairs(entries) do count = count + 1 end

left the typo one in so it should return 1 (it did for me), and I think the table remained iterative.
(Edit: I actually forgot to put the typo back in, it's there now!)
Edit2: Tried with about 75 values, each with 1/4 erroneous records, looks like it worked properly

[Alberth289346]

Ha, well spotted lewri :)

I agree that backward iteration will work. A nice solution that I didn't realize as a solution yet.

[Alberth289346]

Note that a problem only occurs if you have 2 consecutive entries that must be deleted. The second one is never checked for deletion, but if that entry is to be kept then you won't find the problem.

[lewri]

I'd still query if world is the right place for this deletion process or in rather in entity itself.

[TheCycoONE]

It has been so long that I don't remember what I was originally complaining about when I opened the issue... but it definitely had more to do with game logic than garbage collection.