Breaking change/fix: script.raise_event rework - opinions wanted

Place to post guides, observations, things related to modding that are not mods themselves.
Bilka
Factorio Staff
Factorio Staff
Posts: 2331
Joined: Sat Aug 13, 2016 9:20 am
Contact:

Re: Breaking change/fix: script.raise_event rework - opinions wanted

Post by Bilka » Thu Nov 28, 2019 11:59 am

mrvn wrote:
Thu Nov 28, 2019 11:42 am
That was a deep look at the events. But what remains makes me wonder: Why doesn't the game raise the events already?

Any action taking by scripts that would normally raise an event when done by the player or C++ code should just raise the event when done by mods. Drop all the "script_raised_*" stuff. If knowing something was done by script is desired (and yes, it is) then add a field to the events "bool script_raised" or more flexible "string mod_name", which would be nil when not mod raised. If doing things without raising the events is desired/necessary then add an option to the action (e.g. create_entity) to be quiet.

So if some script like bluebuilt revives a ghost the C++ code automatically emits the right event. No need for the mod to do anything and therefore no need to have the raise_event with complex parameter checking. The cost for making that work right would be the addition of script_raised/mod_name to each event on the c++ side. Would that be easier?
Let me imagine a world where .destroy() and .create_entity() automatically raise events. That means that the simple code

for y=0,2 do
for x=0,2 do
game.player.surface.create_entity({name="crude-oil", amount=100000, position={game.player.position.x+x*7-7, game.player.position.y+y*7-7}})
end
end

can now crash your mod! The mod reacting to the event can delete the player or the surface and poop goes your mod. Or the mod just deletes the oil again, or it moves the player or teleports the player to a different surface or whatever. The result is that you don't get what you want. So, you will have to rewrite the command to account for that, perhaps by storing the surface and player position and checking .valid on the surface. That's only for a simple command like that. Imagine doing that in an entire scripting-heavy mod, like Factorissimo. Hell, I even already had enough difficulty handling raising events deleting surface/player in my damn simple Portals mod, and I know Mylon had to deal with mods deleting revived entities in Bluebuild. Or, just to give a concrete example, see this more than 1000 line commit just to deal with mods deleting the entity I just created.

TL;DR: Making raising events the default makes you have to handle all the bullshit other mods can do with the events.

As a side note, some things already automatically raise events, such as surface deletion and chunk deletion.
I'm an admin over at https://wiki.factorio.com. Feel free to contact me if there's anything wrong (or right) with it.

mrvn
Smart Inserter
Smart Inserter
Posts: 3897
Joined: Mon Sep 05, 2016 9:10 am
Contact:

Re: Breaking change/fix: script.raise_event rework - opinions wanted

Post by mrvn » Thu Nov 28, 2019 12:24 pm

Bilka wrote:
Thu Nov 28, 2019 11:59 am
mrvn wrote:
Thu Nov 28, 2019 11:42 am
That was a deep look at the events. But what remains makes me wonder: Why doesn't the game raise the events already?

Any action taking by scripts that would normally raise an event when done by the player or C++ code should just raise the event when done by mods. Drop all the "script_raised_*" stuff. If knowing something was done by script is desired (and yes, it is) then add a field to the events "bool script_raised" or more flexible "string mod_name", which would be nil when not mod raised. If doing things without raising the events is desired/necessary then add an option to the action (e.g. create_entity) to be quiet.

So if some script like bluebuilt revives a ghost the C++ code automatically emits the right event. No need for the mod to do anything and therefore no need to have the raise_event with complex parameter checking. The cost for making that work right would be the addition of script_raised/mod_name to each event on the c++ side. Would that be easier?
Let me imagine a world where .destroy() and .create_entity() automatically raise events. That means that the simple code

for y=0,2 do
for x=0,2 do
game.player.surface.create_entity({name="crude-oil", amount=100000, position={game.player.position.x+x*7-7, game.player.position.y+y*7-7}})
end
end

can now crash your mod! The mod reacting to the event can delete the player or the surface and poop goes your mod. Or the mod just deletes the oil again, or it moves the player or teleports the player to a different surface or whatever. The result is that you don't get what you want. So, you will have to rewrite the command to account for that, perhaps by storing the surface and player position and checking .valid on the surface. That's only for a simple command like that. Imagine doing that in an entire scripting-heavy mod, like Factorissimo. Hell, I even already had enough difficulty handling raising events deleting surface/player in my damn simple Portals mod, and I know Mylon had to deal with mods deleting revived entities in Bluebuild. Or, just to give a concrete example, see this more than 1000 line commit just to deal with mods deleting the entity I just created.

TL;DR: Making raising events the default makes you have to handle all the bullshit other mods can do with the events.

As a side note, some things already automatically raise events, such as surface deletion and chunk deletion.
First: Why don't you store the surface and player position? Looking those up every time in the loop must cost more time than caching them.

Secondly: So what if another mod breaks your mod? If your mod places oil and the other mod deletes all oil then they are obviously incompatible. Set a conflict and be done with it. There are a million ways for one mod to destroy another. Events are nothing special there.

Third: I mentioned events should (must) have a script_raised flag or mod_name. Fix the other mod to not mess with script created entities if that is the right thing to do.

Fourth: As you noticed entity creation can fail or entities can be deleted even without the game emitting events automatically. So I see to fail how having the events would make things worse. You already need those extra lines of code.

Fifth: Why do you create entities one by one? Or rather: Why is there no surface.create_entities()?

Six: Are there more cases of mods raising broken events causing other mods to fail or more mods doing bad things to other mods on correct events? It's a balance.

Seven: Ever tried to place waterfill in a loop like your example under the player? The game can delete the player too.

PyroFire
Fast Inserter
Fast Inserter
Posts: 222
Joined: Tue Mar 08, 2016 8:18 am
Contact:

Re: Breaking change/fix: script.raise_event rework - opinions wanted

Post by PyroFire » Thu Nov 28, 2019 1:37 pm

mrvn wrote:
Thu Nov 28, 2019 11:42 am
Why doesn't the game raise the events already?
In most cases, it does! see e.g. game.delete_surface.
It's how we get from 144 down to about 30 to start with.
mrvn wrote:
Thu Nov 28, 2019 11:42 am
Any action taking by scripts that would normally raise an event when done by the player or C++ code should just raise the event when done by mods. Drop all the "script_raised_*" stuff.
Please note the script_raised stuff is called with surface.create_entity{raise_event=true} for example.
I'm suggesting keep this, give us tile functions, and then give a parameter to credit the action to something and/or add flags.
This could look something like this: surface.create_entity{raise_event={source=game.player}} entity.destroy{raise_event={source=game.player,was_mined=true}} surface.set_tiles{raise_event={source=game.player}} entity.revive{raise_event={source=gameplayer}}
Or simply true to raise it without parameters (backwards compatibility)
mrvn wrote:
Thu Nov 28, 2019 11:42 am
So if some script like bluebuilt revives a ghost the C++ code automatically emits the right event. No need for the mod to do anything and therefore no need to have the raise_event with complex parameter checking. The cost for making that work right would be the addition of script_raised/mod_name to each event on the c++ side. Would that be easier?
You may have misunderstood so far.
Rseding91 wrote:
Mon Nov 04, 2019 3:24 pm
With the Lua event filters coming out in the next experimental version of the game (2~ hours after writing this if the release doesn't blow up) it was brought up that filtering for the script-raised-X events would be nice. But then, it was brought up that we have no way to verify anything given to us from script.raise_event and it got me thinking that it would be nice to change how script.raise_event works.
Rseding91 wrote:
Mon Nov 04, 2019 3:24 pm
2. If we (Wube) don't provide a function for raise_X(...) for a given event then mods just can't raise it.

#2 I have mixed feelings on. Part of me says mods really shouldn't be raising game events anyway and in the (rare) cases they do - we can add in the API.
Rseding91 wrote:
Tue Nov 12, 2019 11:18 am
I was thinking about what it would take to check everything passed through rase_event and it's just not a task that I think any sane human would want to sign up for. There are currently 144 events. The checks for all of them on the C++ side is around 3000 lines of code and that's not counting all of the functions they call themselves. The Lua version is typically 4 times larger due to all the mechanics of interacting with Lua data on the C++ side.

That's (at a guess) around 12'000 lines of code that someone would have to write all just to handle the rotten-eggs case of mods doing things wrong.

That doesn't make sense to me to do.
Rseding91 wrote:
Tue Nov 12, 2019 5:06 pm
If we (Wube) end up doing anything it really only makes sense to make changes that are going to benefit the 99%: the people who aren't passing garbage to events but just listening to game events.

Making sweeping changes that allow the 1% to continue to write bad mods makes no sense to me.
Rseding91 wrote:
Tue Nov 26, 2019 1:37 pm
People seem to be misunderstanding how the entire Lua API works... which Isn't too difficult to do since none of you can see it.

...

So, in summary: no. We can't just use the same checks that the game event uses to validate when mods call raise_event.
Rseding91 wrote:
Tue Nov 26, 2019 1:55 pm
Addendum: I messed up the example function I gave and Bilka pointed out my mistake. That was another 5 minutes to fix that. So... I failed in writing the first of the 144 functions. Meaning: I don't think anyone is going to write those functions and have it be correct.

Meaning: they would all need tests written which is even more time and even more code to write.

It would not be easier.

I like the original proposal to basically remove script.raise_event and replace it with a few simple changes/additions which could make up any lost functionality which i have detailed and it isn't all that much anyway - If anyone can think of why you need to script.raise_event(defines.event.on_surface_deleted) manually, i would look forward to hearing it.
All in all, saves the frames and makes the lua side a bit cleaner, albeit a bit more complicated than a typical lua cache.
Big +1's!

Bilka
Factorio Staff
Factorio Staff
Posts: 2331
Joined: Sat Aug 13, 2016 9:20 am
Contact:

Re: Breaking change/fix: script.raise_event rework - opinions wanted

Post by Bilka » Thu Nov 28, 2019 1:44 pm

mrvn wrote:
Thu Nov 28, 2019 12:24 pm
First: Why don't you store the surface and player position? Looking those up every time in the loop must cost more time than caching them.

Secondly: So what if another mod breaks your mod? If your mod places oil and the other mod deletes all oil then they are obviously incompatible. Set a conflict and be done with it. There are a million ways for one mod to destroy another. Events are nothing special there.

Third: I mentioned events should (must) have a script_raised flag or mod_name. Fix the other mod to not mess with script created entities if that is the right thing to do.

Fourth: As you noticed entity creation can fail or entities can be deleted even without the game emitting events automatically. So I see to fail how having the events would make things worse. You already need those extra lines of code.

Fifth: Why do you create entities one by one? Or rather: Why is there no surface.create_entities()?

Six: Are there more cases of mods raising broken events causing other mods to fail or more mods doing bad things to other mods on correct events? It's a balance.

Seven: Ever tried to place waterfill in a loop like your example under the player? The game can delete the player too.
Damn, you like.. really missed my point.

First: I copied a command from the wiki, because that are the easiest scripts out there. A console command doesnt care about performance, and caching doesnt fix the event problems.

Secondly: That's not how things work in the mod-sphere, and I thought you knew that. Since that seems to no be the case, let me elaborate with the ruins mod example. In this case, I had to add the event to be compatible with the "disco science" mod. However, having the events means that "AAI vehicles" now destroyed the entities in its "prevent turret creeping" code. Being incompatible with disco science doesn't solve that, being incompatible with "AAI vehicles" doesn't solve that. What solved it was talking with Earendel who improved the "prevent turret creeping" code to take into account the entity force, and to make the commit I linked in my last post.

Third: That's already a thing. But it's something you have to handle, just like your surface disappearing because you destroyed a wall. That's a ton of effort. Which is what I aimed to point out - making events raised by default means that mod authors have to put in a ton of effort. Or the mod will slowly die as the mod author doesn't know how to/doesn't want to fix the problems or simply declares incompatibilities. I may be pessimistic here, but most of my previous dealings with mod incompatibilities have shown that being able to work together to fix the problem is the exception. It is much more common that the other mod author is simply someone unskilled "maintaining" the mod who doesnt know how to solve the issue or that the other mod author is hard to find/hard to communicate with/doesn't have time/I am unwilling to talk with them.

Fourth: No, it can't fail. There is nothing in the way and there is no other reason for entity creation to fail (aside from missing prototypes, which I don't support).

Six: There will be a long time of chaos of broken mods and mod interactions if you make events raised by default, even without mods doing "bad things". See AAI + ruins mod above, neither of us did a strictly bad thing, we just both had sup-optimal code, which is to be expected (nobody is perfect).

Seven: Yep, but you expect it and deal with it. Current modA doesn't expect that destroying an entity might delete all entities and ban all players or similar stupid stuff. Dealing with that stupid stuff, and the normal mod interactions would be a huge effort for that modA. Even if it is just declaring all the mod incompatibilities ever, as per your proposed solution. Which again, was my point: "What you propose will create a fuckton of work for mod authors".
I'm an admin over at https://wiki.factorio.com. Feel free to contact me if there's anything wrong (or right) with it.

mrvn
Smart Inserter
Smart Inserter
Posts: 3897
Joined: Mon Sep 05, 2016 9:10 am
Contact:

Re: Breaking change/fix: script.raise_event rework - opinions wanted

Post by mrvn » Thu Nov 28, 2019 3:56 pm

PyroFire wrote:
Thu Nov 28, 2019 1:37 pm
mrvn wrote:
Thu Nov 28, 2019 11:42 am
Why doesn't the game raise the events already?
In most cases, it does! see e.g. game.delete_surface.
It's how we get from 144 down to about 30 to start with.
[\quote]

And I was talking of the few remaining events that i quoted.

PyroFire wrote:
Thu Nov 28, 2019 1:37 pm
mrvn wrote:
Thu Nov 28, 2019 11:42 am
Any action taking by scripts that would normally raise an event when done by the player or C++ code should just raise the event when done by mods. Drop all the "script_raised_*" stuff.
Please note the script_raised stuff is called with surface.create_entity{raise_event=true} for example.
I'm suggesting keep this, give us tile functions, and then give a parameter to credit the action to something and/or add flags.
This could look something like this: surface.create_entity{raise_event={source=game.player}} entity.destroy{raise_event={source=game.player,was_mined=true}} surface.set_tiles{raise_event={source=game.player}} entity.revive{raise_event={source=gameplayer}}
Or simply true to raise it without parameters (backwards compatibility)
Which is what I'm suggesting doing for all of them. Except I think dropping the "script_" part and having that as a flag in the event instead makes more sense. I assume the event filter could then also be used to filter only player event or only script events if so desired.

My question is this: If any action where mods currently have to manually raise events (e.g. bluebuilt reviving a ghost) emits an event automatically what is then left on raise_event() calls?
PyroFire wrote:
Thu Nov 28, 2019 1:37 pm
mrvn wrote:
Thu Nov 28, 2019 11:42 am
So if some script like bluebuilt revives a ghost the C++ code automatically emits the right event. No need for the mod to do anything and therefore no need to have the raise_event with complex parameter checking. The cost for making that work right would be the addition of script_raised/mod_name to each event on the c++ side. Would that be easier?
You may have misunderstood so far.
Rseding91 wrote:
Mon Nov 04, 2019 3:24 pm
With the Lua event filters coming out in the next experimental version of the game (2~ hours after writing this if the release doesn't blow up) it was brought up that filtering for the script-raised-X events would be nice. But then, it was brought up that we have no way to verify anything given to us from script.raise_event and it got me thinking that it would be nice to change how script.raise_event works.
Rseding91 wrote:
Mon Nov 04, 2019 3:24 pm
2. If we (Wube) don't provide a function for raise_X(...) for a given event then mods just can't raise it.

#2 I have mixed feelings on. Part of me says mods really shouldn't be raising game events anyway and in the (rare) cases they do - we can add in the API.
Rseding91 wrote:
Tue Nov 12, 2019 11:18 am
I was thinking about what it would take to check everything passed through rase_event and it's just not a task that I think any sane human would want to sign up for. There are currently 144 events. The checks for all of them on the C++ side is around 3000 lines of code and that's not counting all of the functions they call themselves. The Lua version is typically 4 times larger due to all the mechanics of interacting with Lua data on the C++ side.

That's (at a guess) around 12'000 lines of code that someone would have to write all just to handle the rotten-eggs case of mods doing things wrong.

That doesn't make sense to me to do.
Rseding91 wrote:
Tue Nov 12, 2019 5:06 pm
If we (Wube) end up doing anything it really only makes sense to make changes that are going to benefit the 99%: the people who aren't passing garbage to events but just listening to game events.

Making sweeping changes that allow the 1% to continue to write bad mods makes no sense to me.
Rseding91 wrote:
Tue Nov 26, 2019 1:37 pm
People seem to be misunderstanding how the entire Lua API works... which Isn't too difficult to do since none of you can see it.

...

So, in summary: no. We can't just use the same checks that the game event uses to validate when mods call raise_event.
Rseding91 wrote:
Tue Nov 26, 2019 1:55 pm
Addendum: I messed up the example function I gave and Bilka pointed out my mistake. That was another 5 minutes to fix that. So... I failed in writing the first of the 144 functions. Meaning: I don't think anyone is going to write those functions and have it be correct.

Meaning: they would all need tests written which is even more time and even more code to write.

It would not be easier.

I like the original proposal to basically remove script.raise_event and replace it with a few simple changes/additions which could make up any lost functionality which i have detailed and it isn't all that much anyway - If anyone can think of why you need to script.raise_event(defines.event.on_surface_deleted) manually, i would look forward to hearing it.
All in all, saves the frames and makes the lua side a bit cleaner, albeit a bit more complicated than a typical lua cache.
Big +1's!
I think it would be easier since having it would be some 16000 lines of code and not having it probably just a few missing event calls in the c++ code and no LUA argument checks. Provided there aren't good reasons to raise_event() from LUA other than those where the c++ code fails to raise it itself.

To summarize: I now like the original proposal if the existing raise_event() calls can be obsoleted by the c++ code raising the events (as it already does in several cases).

User avatar
eradicator
Smart Inserter
Smart Inserter
Posts: 3773
Joined: Tue Jul 12, 2016 9:03 am
Contact:

Re: Breaking change/fix: script.raise_event rework - opinions wanted

Post by eradicator » Thu Nov 28, 2019 4:00 pm

Rseding91 wrote:
Tue Nov 26, 2019 5:33 pm
I come from a background of playing with Minecraft mods and talking with the mod authors about the horrible issues they have because of other mods doing things wrong/stupidly and I don't want to stick that on anyone else.
Btw as mentioned by @Bilka too now, being able to manually raise events for things after i'm done with them makes things *less* horrible, not more. Not being able to rely on entities i created myself is - as mentiond - a nightmare. And if the ability to raise on_built *after* i'm done is removed then i and many other people will simply chose to not raise it at all by default, which massively reduces overall cross-mod compatibility.
eradicator wrote:
Sat Nov 09, 2019 7:52 pm
Btw, here's another usecase for manually raising on_build: If i use create_entity{raise_build=true} then another mod could destroy the created entity before i even ever see it. Therefor

Code: Select all

function()
create_entity{raise_build=false}
--dostuff
script.raise_event(defines.events.on_built,...)
end
means i have to care about less shit that other mods might do to my entities.
Bilka wrote:
Thu Nov 28, 2019 11:59 am
TL;DR: Making raising events the default makes you have to handle all the bullshit other mods can do with the events.
Bilka wrote:
Thu Nov 28, 2019 1:44 pm
Seven: Yep, but you expect it and deal with it. Current modA doesn't expect that destroying an entity might delete all entities and ban all players or similar stupid stuff. Dealing with that stupid stuff, and the normal mod interactions would be a huge effort for that modA. Even if it is just declaring all the mod incompatibilities ever, as per your proposed solution. Which again, was my point: "What you propose will create a fuckton of work for mod authors".
Author of: Hand Crank Generator, Screenshot Hotkey 2.0
Mod support languages: 日本語, Deutsch, English
My code in the post above is dedicated to the public domain under CC0.

mrvn
Smart Inserter
Smart Inserter
Posts: 3897
Joined: Mon Sep 05, 2016 9:10 am
Contact:

Re: Breaking change/fix: script.raise_event rework - opinions wanted

Post by mrvn » Thu Nov 28, 2019 4:13 pm

Fourth: Except they can and do fail. Ever tried to build cliffs?

You can check if the cliff collides with anything and the game says no. Then you build it and it doesn't return an entity. Fun.

I think you also failed to get my point. All the examples you bought up are cases that already happen. You already have to cover all the bases. You haven't shown a single case that would only happen with events raised by the actions directly instead of the mod.


Also who said that events should be recursive? You assume so. You say creating an entity would raise the event. Then some other mods event handler would delete the entity and then the entity returned to your mod would no longer exist.

This has a simple fix to prevent that. You have an event queue. Creating an entity would queue up the event but the event would only be processed when the current script returns.

Rseding91
Factorio Staff
Factorio Staff
Posts: 10030
Joined: Wed Jun 11, 2014 5:23 am
Contact:

Re: Breaking change/fix: script.raise_event rework - opinions wanted

Post by Rseding91 » Thu Nov 28, 2019 4:54 pm

mrvn wrote:
Thu Nov 28, 2019 4:13 pm
... This has a simple fix to prevent that. You have an event queue. Creating an entity would queue up the event but the event would only be processed when the current script returns.
This doesn't work. You can't queue "entity died" because after an entity is dead it's deleted - gone from the game forever. You can't queue "entity built" because it may be killed in the same tick and thus invalid to ever send to the queued "built" event.

(most) events are instant and only valid and possible to be done immediately.
If you want to get ahold of me I'm almost always on Discord.

mrvn
Smart Inserter
Smart Inserter
Posts: 3897
Joined: Mon Sep 05, 2016 9:10 am
Contact:

Re: Breaking change/fix: script.raise_event rework - opinions wanted

Post by mrvn » Fri Nov 29, 2019 4:29 pm

Rseding91 wrote:
Thu Nov 28, 2019 4:54 pm
mrvn wrote:
Thu Nov 28, 2019 4:13 pm
... This has a simple fix to prevent that. You have an event queue. Creating an entity would queue up the event but the event would only be processed when the current script returns.
This doesn't work. You can't queue "entity died" because after an entity is dead it's deleted - gone from the game forever. You can't queue "entity built" because it may be killed in the same tick and thus invalid to ever send to the queued "built" event.

(most) events are instant and only valid and possible to be done immediately.
That may be how factorio currently does it but it's hardly the only way. Many event based systems handle events in queues just fine. Nothing says you must free the died entity imediately. The "entity died" event will have a pointer to the entity keeping it alive even if it is removed from the chunk. Then when the event is finished you remove the reference, the refenrence count goes to 0 and the entity gets freed.

Similar the case of having an entity build and then destroyed. In QT for example you call widget.deleteLater() in event handlers. Then some time later in the event queue processing the widget actually gets destroyed. So any event referenceing the widget already in the queue will work just fine. It's still in the same tick but it keeps things well ordered.

Doing events immediately is just one way of doing things. Not the only way.

Post Reply

Return to “Modding discussion”

Who is online

Users browsing this forum: No registered users