[Solved] Which event should be used for LuaEntity.destroy()?

[Klonan]

Statistics on my side:
When the on_tick listener was added, FPS/UPS started to drop slowly when the number of turrets reached 8k.
When the on_tick listener was removed, FPS/UPS started to drop when the number reached 18k. FPS/UPS was around 54 when there were 19k turrets.
Then, the on_tick listener was added back, FPS/UPS dropped to 34.

I couldn't say whether the number of turrets is good or not for the users, as I mainly write mods recently rather than actually playing the game. I would say it is better than I thought.
Few things to note:
- the numbers are showing the situation when ONLY ONE mod is running. I expect the actual number of modded entities before FPS/UPS starts dropping will be divided by the number of mods running the same algorithm.
- when I tried to blueprint 1k turrets and placed them at once, FPS/UPS dropped significantly and could not recover. (I doubt it is something related to the code in game.)
(Creative Mode was used for clearing out obstacles, creating the nice flat land and placing turrets. But it has been removed before testing.)

Now judge it yourself.

You function is just very plain, you can optimize it very easily, you are checking all entities on every tick, you could spread the checks across multiple ticks, or only do a check every 60 ticks, or any number of things,
The way you have done it is somewhat of the least efficient way, for example this small addition would increase performance 60 times:

Code: Select all

local function on_tick(event)
  for surface_name, surface_position_x in pairs(global.supportive_turrets) do
    for x, surface_position_y in pairs(surface_position_x) do
      if (x + game.tick) % 60 == 0 then
        for y, data in pairs(surface_position_y) do
          if (x + y + game.tick) % check_period == 0 then
            if not data.turret.valid thn
              if data.unit.valid then
                data.unit.destroy()
              end
              global.supportive_turrets[surface_name][x][y] = nil
            end
          end
        end
      end
    end
  end
end

Besides,
The point i am getting at, is that you shouldn't worry about this,
Have no on_tick,
Don't worry about them being destroyed, because it is highly unlikely

In all my fancy mods with creating and destroying entities, i have never had an on_tick checker,
And i have never had any complaints about the entities being destroyed by some other mod

[aubergine18]

Can we get LuaEntity.can_destroy() added to the API? It will return `true` if specified entity can currently be destroyed, otherwise `nil`.

[mknejp]

Besides,
The point i am getting at, is that you shouldn't worry about this,
Have no on_tick,
Don't worry about them being destroyed, because it is highly unlikely

In all my fancy mods with creating and destroying entities, i have never had an on_tick checker,
And i have never had any complaints about the entities being destroyed by some other mod

Well "lucky you"? I guess?

It's great this never happened to you, but dismissing the problem entirely just because it never happened to you specifically isn't going to solve the problem others do have.

We certainly can live without the pre_destroy event but that means we have to waste precious CPU cycles on validity checks that *should not be necessary*. Why do workload at all when you can setup the system so that said work doesn't have to be done periodically? It is really perplexing to me why this is considered the better option seeing how focused the devs apparently are on performance.

Not having standardized events for other actions mods do (that are not auto-triggered by the game), like creating entities or placing items into equipment, can break a mod's functionality without manual interaction by the player, which completely defeats the point. See the example between Vehicle Wagons and Electric Vehicles. There is currently no *reliable* way for Vehicle Wagon to unload an electric car that is actually functional without having to resolve to raising hardcoded game-triggered events (which IMO is a bad thing to do for reasons explained earlier).

[Klonan]

There is currently no *reliable* way for Vehicle Wagon to unload an electric car that is actually functional without having to resolve to raising hardcoded game-triggered events (which IMO is a bad thing to do for reasons explained earlier).

There is no way to do it except the exact way you should do it?
I don't see what you are arguing against here, and your opinion of what is bad isn't valid. Raising events is the best solution in many cases.

Well "lucky you"? I guess?

It's great this never happened to you, but dismissing the problem entirely just because it never happened to you specifically isn't going to solve the problem others do have.

Yes i am dismissing the problem, because i have seen countless mods use entity.destroy() over the course of the last 3 major versions without worrying about raising any event,
And i have not yet seen any issue with it.

We certainly can live without the pre_destroy event but that means we have to waste precious CPU cycles on validity checks that *should not be necessary*.

Validity checks are best practice for any mod. If you are doing something to an entity, check if it is valid. The 'wasted' cpu is negligible.

[aubergine18]

I've made a module that provides `create_entity()` and `destroy_entity()` global functions - they act like `LuaSurface.create_entity()` and `LuaEntity.destroy()`, only they trigger events.

https://github.com/aubergine10/lifecycle-events

It's not fully tested (wrote it out of frustration so there might be some bugs in there) and I'm not sure if the robot version of `create_entity()` will work (ISTR game strips events if they pass a non-valid entity).

I'll need help with the `can_destroy()` function - I've never done anything with rail/locomotives so don't know best way to work out of train is on a rail that's being destroyed, etc.

Anyway, the events triggered are the ones most mods listen for and act upon, so in theory this should work great when interacting with other mods. Let me know what you think.

[Rseding91]

I've made a module that provides `create_entity()` and `destroy_entity()` global functions - they act like `LuaSurface.create_entity()` and `LuaEntity.destroy()`, only they trigger events.

https://github.com/aubergine10/lifecycle-events

It's not fully tested (wrote it out of frustration so there might be some bugs in there) and I'm not sure if the robot version of `create_entity()` will work (ISTR game strips events if they pass a non-valid entity).

I'll need help with the `can_destroy()` function - I've never done anything with rail/locomotives so don't know best way to work out of train is on a rail that's being destroyed, etc.

Anyway, the events triggered are the ones most mods listen for and act upon, so in theory this should work great when interacting with other mods. Let me know what you think.

And now the vast majority of mods listening will break because you exclude things like player_index or other properties expected to be in the events

If you have specific inner-mod compatibility issues you make a remote interface and have other mods listen to it when you do weird things.

[aubergine18]

Fixed typo, it now sends player_index.

If there are other errors in that script, I'll fix them too. No changes should be required to other mods.

[Mooncat]

I thought we are talking about changes for 0.15? Why worry about breaking mods when they are already broken by the update?
(e.g. the change of gui.left's direction)

When the game develops, we should expect mods to be more advanced, doing more stuffs that have not been expected before. So, there will be more conflicts between different mods.

But I see you devs seeing the default events to be something like a bridge between the game to each individual mod (i.e. transfer data from the game to mods), rather than used for communicating between mods, so it has nothing to do with mods calling create_entity() or destroy().

I won't argue that you are doing it wrong or not, as I also agree that we should spend more time on improving the game itself first. But just don't decline there is a problem in the current event system - it is not scalable.

If you have specific inner-mod compatibility issues you make a remote interface and have other mods listen to it when you do weird things.

It is only applicable after the issue has occurred, so you know what should your remote function provide. And to solve this particular issue, at least 2 modders are involved. One providing the remote interface and one using it.
As Fatmice said on IRC, it is just an ad-hoc.

Validity checks are best practice for any mod. If you are doing something to an entity, check if it is valid. The 'wasted' cpu is negligible.

Yes, if you are talking about the valid check itself... maybe I should clarify myself: I would do the valid check "when using something" anyway even if there is an event for destroy().
Like, when I use on_player_selected_area, I would do the valid check for each selected entity before accessing them. So it makes no difference.
The problem is that without any event for destroy(), we will have to use on_tick to do the valid check. While I have said that 8k was better than I expected, 8k vs 18k is still very obvious.


TL;DR The problem still exists, but there are alternative (dirty) solutions and we can still survive. I sincerely hope that you the devs can improve the event system. But it is totally up to you to do it or not.

I am out now, since I think 8k should be enough for the players, and as bob said, the iteration can be further divided into multiple ticks (3 ticks should be fine).

[Klonan]

I thought we are talking about changes for 0.15? Why worry about breaking mods when they are already broken by the update?
(e.g. the change of gui.left's direction)

When the game develops, we should expect mods to be more advanced, doing more stuffs that have not been expected before. So, there will be more conflicts between different mods.

But I see you devs seeing the default events to be something like a bridge between the game to each individual mod (i.e. transfer data from the game to mods), rather than used for communicating between mods, so it has nothing to do with mods calling create_entity() or destroy().

I won't argue that you are doing it wrong or not, as I also agree that we should spend more time on improving the game itself first. But just don't decline there is a problem in the current event system - it is not scalable.

If you have specific inner-mod compatibility issues you make a remote interface and have other mods listen to it when you do weird things.

It is only applicable after the issue has occurred, so you know what should your remote function provide. And to solve this particular issue, at least 2 modders are involved. One providing the remote interface and one using it.
As Fatmice said on IRC, it is just an ad-hoc.

Validity checks are best practice for any mod. If you are doing something to an entity, check if it is valid. The 'wasted' cpu is negligible.

Yes, if you are talking about the valid check itself... maybe I should clarify myself: I would do the valid check "when using something" anyway even if there is an event for destroy().
Like, when I use on_player_selected_area, I would do the valid check for each selected entity before accessing them. So it makes no difference.
The problem is that without any event for destroy(), we will have to use on_tick to do the valid check. While I have said that 8k was better than I expected, 8k vs 18k is still very obvious.


TL;DR The problem still exists, but there are alternative (dirty) solutions and we can still survive. I sincerely hope that you the devs can improve the event system. But it is totally up to you to do it or not.

I am out now, since I think 8k should be enough for the players, and as bob said, the iteration can be further divided into multiple ticks (3 ticks should be fine).

Once again let me say,
Just don't worry about it

Doing an on_tick for every user of your mod in the off chance that someone uses a mod which might at some point cause one of your entities to be destroyed isn't worth it,
And once again, I have had mods exactly like yours, i create an entity with on built, destroy it on mined, and i never did an on_tick, and i never had any reported issues

Checking for this, is not worth your time, it is not worth the CPU for all the on_tick checks, and is not worth worrying about, as it will probably never be an issue for 99.99% of the people who use your mod

[Mooncat]

Maybe you are right. I don't need to worry about it NOW.

But I doubt it if this discussion did not exist.

Well, the statement about "check valid before using something" has distracted us for awhile, but after a second thought about what you have said...

Same with this, if another mods scripting is breaking your mod, it is their issue to fix

I know what you meant now.
If someone breaks my mod by calling LuaEntity::destroy(), it is his responsibility to make sure my mod knows my entity is destroyed.
In that case, I won't need to use the on_tick listener.

Problem is, there is no standard way to do that. He may do it wrong.
(And no, it is not an edge case. Electric Vehicle and Vehicle Wagon has just hit that "0.01%". The chance of having this issue will increase because it is so easy to call destroy() without thinking about the consequences.)

What we were asking since page 1 is the standard solution, provided by you, the devs, so all modders know how to do it right.
It is unfortunate that such solution is impossible due to the limitation of event system.

Luckily, we had this discussion. Now we can say to other modders: raise on_entity_died before calling LuaEntity::destroy(), or prepare for bug reports!
Similarly, raise on_built_entity after LuaSurface::create_entity if your function can create entities not from your mod, e.g. something like Instant Blueprint.

So, yes, I think my mod can live happily without on_tick, given that I can copy the link to this thread and paste to any mod author who breaks it.

I will update the OP about this as the conclusion, if no objection is raised. Thanks for your precious time.

[aubergine18]

Post a link to lifecycle-events in OP pls, as it shows a rough template for creating the events, at least a starting point for other modders to use when they are doing the stuff that they definitely will never do.

https://github.com/aubergine10/lifecycle-events

Still need better way to know if an entity can be destroyed (not just for this events stuff, but in general).

[aubergine18]

Also, note in OP that the event only really needs to be triggered when .destroy() or .create_entity() are called from Lua script (ie. by a mod), and furthermore there could be a parameter on those methods to choose whether to send it (default = false).

In most cases, mods creating/destroying stuff has no impact on other mods, thus not vital for the events to be sent. It's only when mods are likely to create/destroy entities external to themselves that the events are going to be needed.

We don't really need these events for particles, smoke, remnants, etc. Although, with that optional parameter, if we did we could easily achieve it on a case by case basis.

[Mooncat]

Sure, I just wanted to ask for your permission before posting it.
And it is done now.