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: 3310
Joined: Sat Aug 13, 2016 9:20 am
Contact:

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

Post by Bilka »

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: 5911
Joined: Mon Sep 05, 2016 9:10 am
Contact:

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

Post by mrvn »

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
Filter Inserter
Filter Inserter
Posts: 356
Joined: Tue Mar 08, 2016 8:18 am
Contact:

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

Post by PyroFire »

mrvn wrote: ↑Thu Nov 28, 2019 11:42 amWhy 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: 3310
Joined: Sat Aug 13, 2016 9:20 am
Contact:

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

Post by Bilka »

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: 5911
Joined: Mon Sep 05, 2016 9:10 am
Contact:

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

Post by mrvn »

PyroFire wrote: ↑Thu Nov 28, 2019 1:37 pm
mrvn wrote: ↑Thu Nov 28, 2019 11:42 amWhy 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: 5207
Joined: Tue Jul 12, 2016 9:03 am
Contact:

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

Post by eradicator »

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: Belt Planner, Hand Crank Generator, Screenshot Maker, /sudo and more.
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: 5911
Joined: Mon Sep 05, 2016 9:10 am
Contact:

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

Post by mrvn »

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: 14360
Joined: Wed Jun 11, 2014 5:23 am
Contact:

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

Post by Rseding91 »

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: 5911
Joined: Mon Sep 05, 2016 9:10 am
Contact:

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

Post by mrvn »

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.
Honktown
Smart Inserter
Smart Inserter
Posts: 1042
Joined: Thu Oct 03, 2019 7:10 am
Contact:

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

Post by Honktown »

I read over the first page and skimmed the last page:
One can already traverse through game.defines to look for things. It seems like it'd be simple enough to register a new event (is it a read-only table?). Mods can go looking in the defines to see if an event is available to be raised, and maybe try again in a few ticks, for dependency order issues.

For validating, a validation function can be provided by the mod. It wouldn't be good in 100% of cases obviously, but a simple validator would at least check if all the values are of the right type(). A smarter validator would run an entity.valid and what not, since most things have "safety" properties. In a way, this is the most sensible way to do it, because the one who creates an event flag should be the one who is responsible for guaranteeing the event is safe to use. There will never be a way to guarantee a value isn't bad unless the game explicitly holds hostage all the logic leading up to that value (even providing the wrong player index is a safety bug). I can definitely pass the wrong things to Factorio, even if they're not purely garbage, and I'll get the wrong results back. It's a "Halting Problem" to suggest you can make everything safe.

A validation could be the first function in line when an event is raised, and perhaps is the only function which has permission to un-raise the event.

Edit: Leaving the validation up to the register-er would also open up mod options: If I have a non-valid entity do I let the event go forward? Should I replace it with nil and continue the event functions? One may want to act on a nil situation or non-valid entity, so leaving the devs up to the decision is not something I'd prefer to do [because I often don't agree].
Last edited by Honktown on Thu Dec 19, 2019 5:27 am, edited 1 time in total.
I have mods! I guess!
Link
Honktown
Smart Inserter
Smart Inserter
Posts: 1042
Joined: Thu Oct 03, 2019 7:10 am
Contact:

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

Post by Honktown »

PyroFire wrote: ↑Wed Nov 27, 2019 3:44 pm Unless you can name an event and give a purpose or reason for needing script.raise_event, i doubt any of them will actually be kept because it seems like the devs are already intent on removing them or changing how it works because, as it has been stated,
For raising an event, anything in the base game should be able to raise it by calling an appropriate matching function: if I want something damaged, I should be able to call something like event_function(on_entity_damaged, my_damage_function(arguments)), and my damage function returns the table that should be passed to other functions because an entity was damaged. At present, mods have to create a virtual damage source and do damage that way, or edit an entity's health and not raise the event, which causes a minor issue for my regeneration mod.

I can think of reasons for adding custom events and raising them: I add pets/followers to the game, and I want them to stay with you if you teleport. I either check every random tick if the entity is too far away, or I use an "on_player_teleported" event exactly once when it is needed.

More:
on_player_used_blueprint doesn't exist. I can create a fake event and use that, and if other people find it interesting, subscribe to my event that I have the responsibility of raising. If I can't add it that way, we have to go into remote interfacing and a bunch of bullshit workarounds.

If I can create a practical implementation and use of a simulated event, I can ask the devs about actually implementing it, instead of coming to them with one example use, in a half-assed workaround way, which they'll immediately dismiss.

If the events interface is transparent, if an event was ported from a mod to real use, mods which previously used my event would work 100% the same as before.
I have mods! I guess!
Link
Bilka
Factorio Staff
Factorio Staff
Posts: 3310
Joined: Sat Aug 13, 2016 9:20 am
Contact:

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

Post by Bilka »

A note for people looking for modding help here: The two posts before this one are not how the game works right now or how the game will work. I recommend checking the documentation or other mods for how events are raised right now and for what can be done with them.

@Honktown, considering that you don't seem to know that raise_event and generate_event_name exist, your contribution to this thread is rather worthless. Consider informing yourself on existing systems before proposing new ones.
I'm an admin over at https://wiki.factorio.com. Feel free to contact me if there's anything wrong (or right) with it.
Honktown
Smart Inserter
Smart Inserter
Posts: 1042
Joined: Thu Oct 03, 2019 7:10 am
Contact:

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

Post by Honktown »

Bilka wrote: ↑Thu Dec 19, 2019 5:49 am A note for people looking for modding help here: The two posts before this one are not how the game works right now or how the game will work. I recommend checking the documentation or other mods for how events are raised right now and for what can be done with them.

@Honktown, considering that you don't seem to know that raise_event and generate_event_name exist, your contribution to this thread is rather worthless. Consider informing yourself on existing systems before proposing new ones.
As I said before; the more I think about this the more it makes sense to just disable mods from ever calling game events and being done with it. It shouldn't have existed in the first place.
You even responded to him. [Moderated by Koub : unnecessarily disrespectful]
I have mods! I guess!
Link
Bilka
Factorio Staff
Factorio Staff
Posts: 3310
Joined: Sat Aug 13, 2016 9:20 am
Contact:

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

Post by Bilka »

Honktown wrote: ↑Thu Dec 19, 2019 6:08 am You even responded to him. [Moderated by Koub : unnecessarily disrespectful]
Please be respectful to others.

The proposal is about removing raise_event for game events, it doesn't change generate_event_name or that custom mod events can be raised with raise_event. Furthermore, the proposal does not change that:
At present, mods have to create a virtual damage source and do damage that way, or edit an entity's health and not raise the event, which causes a minor issue for my regeneration mod.
is wrong and
game.defines ... a read-only table? ... Mods can go looking in the defines to see if an event is available to be raised, and maybe try again in a few ticks, for dependency order issues.
is not how the game works currently, despite you representing both as the current feature set. I purposefully didnt say what you were wrong about because I didnt want this thread to turn into yet another "explain Honktown how they are wrong while they don't accept it" thread. Since I still don't want that to happen I will refrain from replying in the future. This proposal has been dead for a while now and I'm on vacation anyway. Good day.
I'm an admin over at https://wiki.factorio.com. Feel free to contact me if there's anything wrong (or right) with it.
Koub
Global Moderator
Global Moderator
Posts: 7787
Joined: Fri May 30, 2014 8:54 am
Contact:

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

Post by Koub »

[Koub] Did a bit of moderation here, please behave and don't show disrespect.
Koub - Please consider English is not my native language.
Honktown
Smart Inserter
Smart Inserter
Posts: 1042
Joined: Thu Oct 03, 2019 7:10 am
Contact:

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

Post by Honktown »

Hope your vacations going well. Looking through the API, it seems a mod can currently raise a transparent event as I suggested it couldn't. I was wrong on that. (it's not clear how you pass the information needed for an event in https://lua-api.factorio.com/latest/Lua ... aise_event , so I'm not sure how one would use it).

Currently a script can raise script_did_whatever events, and I definitely think nobody wants scripts only to be able to generate script_raised events, which is an option (i.e. only game can raise "real" events, scripts generate on_script* events).

As always, a critique
you are on vacation you shouldn't be working
It is a valid criticism that there are no validity functions. Nearly the rest of the game does have them: accessing parameters not explicitly disclosed in game hard-exits the current game, category-specific definitions that result in the game error'ing in the data stage, mod folder version agreeing with mod info (BUT ONLY WHEN IT'S NOT ZIPPED), etc.
I have mods! I guess!
Link
PyroFire
Filter Inserter
Filter Inserter
Posts: 356
Joined: Tue Mar 08, 2016 8:18 am
Contact:

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

Post by PyroFire »

Honktown wrote: ↑Thu Dec 19, 2019 1:48 am I read over the first page and skimmed the last page:
It seems like it'd be simple enough to register a new event (is it a read-only table?).
"registering new events" (custom events) is completely unrelated to this discussion.
Honktown wrote: ↑Thu Dec 19, 2019 1:48 am Mods can go looking in the defines to see if an event is available to be raised, and maybe try again in a few ticks, for dependency order issues.
... What? This sounds like nonsense.
Honktown wrote: ↑Thu Dec 19, 2019 1:48 am For validating, a validation function can be provided by the mod. It wouldn't be good in 100% of cases obviously, but a simple validator would at least check if all the values are of the right type(). A smarter validator would run an entity.valid and what not, since most things have "safety" properties. In a way, this is the most sensible way to do it, because the one who creates an event flag should be the one who is responsible for guaranteeing the event is safe to use.

There will never be a way to guarantee a value isn't bad unless the game explicitly holds hostage all the logic leading up to that value (even providing the wrong player index is a safety bug). I can definitely pass the wrong things to Factorio, even if they're not purely garbage, and I'll get the wrong results back. It's a "Halting Problem" to suggest you can make everything safe.
A validation could be the first function in line when an event is raised, and perhaps is the only function which has permission to un-raise the event.
It's almost like you're posting without actually reading the thread or informing yourself of what's being discussed just to get your post count up for the sake of having a post count:
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.
----
Honktown wrote: ↑Thu Dec 19, 2019 1:48 am Edit: Leaving the validation up to the register-er would also open up mod options: If I have a non-valid entity do I let the event go forward? Should I replace it with nil and continue the event functions? One may want to act on a nil situation or non-valid entity, so leaving the devs up to the decision is not something I'd prefer to do [because I often don't agree].
You seem to not understand what's being discussed here (the removal of script.raise_event), nor why it is being discussed.
I could explain it here again, but i'm not going to because it has been discussed and explained at length over the past few pages.
Honktown wrote: ↑Thu Dec 19, 2019 2:14 am
PyroFire wrote: ↑Wed Nov 27, 2019 3:44 pm Unless you can name an event and give a purpose or reason for needing script.raise_event, i doubt any of them will actually be kept because it seems like the devs are already intent on removing them or changing how it works because, as it has been stated,
You didn't name any events that you needed script.raise_event for -- why did you respond to me again?
Honktown wrote: ↑Thu Dec 19, 2019 2:14 am For raising an event, anything in the base game should be able to raise it by calling an appropriate matching function: if I want something damaged, I should be able to call something like event_function(on_entity_damaged, my_damage_function(arguments)), and my damage function returns the table that should be passed to other functions because an entity was damaged. At present, mods have to create a virtual damage source and do damage that way, or edit an entity's health and not raise the event, which causes a minor issue for my regeneration mod.
entity.damage raises on_entity_damaged internally.
What's the issue?
Honktown wrote: ↑Thu Dec 19, 2019 2:14 am I can think of reasons for adding custom events and raising them: I add pets/followers to the game, and I want them to stay with you if you teleport. I either check every random tick if the entity is too far away, or I use an "on_player_teleported" event exactly once when it is needed.
What's wrong with on_player_changed_position and on_player_changed_surface?
Again, you have not given any reason why those events need to be script.raise_event'd.
Honktown wrote: ↑Thu Dec 19, 2019 2:14 am on_player_used_blueprint doesn't exist.
Yes it does.

The following events are raised when you place a blueprint:
https://lua-api.factorio.com/latest/eve ... n_put_item
https://lua-api.factorio.com/latest/eve ... ilt_entity
Honktown wrote: ↑Thu Dec 19, 2019 2:14 am I can create a fake event and use that, and if other people find it interesting, subscribe to my event that I have the responsibility of raising. If I can't add it that way, we have to go into remote interfacing and a bunch of bullshit workarounds.
No one needs your custom events - it's already possible.
Honktown wrote: ↑Thu Dec 19, 2019 2:14 am If I can create a practical implementation and use of a simulated event
Thats what i asked for.
Give me an example of a practical implementation for raising an event manually with script.raise_event.
You've posted 4 times and we're all still waiting for it.
Honktown wrote: ↑Thu Dec 19, 2019 2:14 am I can ask the devs about actually implementing it, instead of coming to them with one example use, in a half-assed workaround way, which they'll immediately dismiss.
This is why this thread exists - the devs are asking for a reason to keep script.raise_event.
If you can't give a good reason to keep it, you really didn't need to post at all.
Heck you even quoted me asking for the one thing that will change the devs mind, and you failed to respond to even that.
Honktown wrote: ↑Thu Dec 19, 2019 7:12 am (it's not clear how you pass the information needed for an event in https://lua-api.factorio.com/latest/Lua ... aise_event , so I'm not sure how one would use it).
Your credibility in this discussion starts and stops there friend.
Honktown wrote: ↑Thu Dec 19, 2019 7:12 am I definitely think nobody wants scripts only to be able to generate script_raised events
You clearly didn't read the thread. Mods like bluebuild need script.raise_event to simulate certain player actions.


So over the 4 posts you have made in this thread, you have not only failed to see why this thread exists, you have also failed to contribute anything of meaning or value to the discussion and have not addressed anyones raised points (beyond berating the devs for their decisions and judgement) because what i can only assume is you not understanding what is being discussed, so in liu of that understanding, well, i don't actually know what you're trying to achieve, but it's not going to result in script.raise_event being kept that much we know for certain.

Bilka wrote: ↑Thu Dec 19, 2019 6:28 am I purposefully didnt say what you were wrong about because I didnt want this thread to turn into yet another "explain Honktown how they are wrong while they don't accept it" thread. Since I still don't want that to happen I will refrain from replying in the future.
Nuff said, i think i'll do the same.
And I have already layed out my arguments about this discussion in detail, there is little more for me to say.
Remove script.raise_event - we don't really need it except for a few very niche use-cases which can easily be worked around by adding some alternative methods for what will be lost (which i have already defined clearly).
Honktown
Smart Inserter
Smart Inserter
Posts: 1042
Joined: Thu Oct 03, 2019 7:10 am
Contact:

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

Post by Honktown »

PyroFire wrote: ↑Thu Dec 19, 2019 9:32 am ... What? This sounds like nonsense.

Code: Select all

  defines = {
  ...
    events = {
      on_ai_command_completed = 113,
      on_area_cloned = 118,
      on_biter_base_built = 55,
      on_built_entity = 6,
      on_cancelled_deconstruction = 21,
      on_cancelled_upgrade = 115,
      on_character_corpse_expired = 90,
      on_chart_tag_added = 135,
      on_chart_tag_modified = 136,
      on_chart_tag_removed = 137,
      on_chunk_charted = 98,
      on_chunk_deleted = 106,
      on_chunk_generated = 12,
      on_combat_robot_expired = 80,
      on_console_chat = 71,
      on_console_command = 72,
      on_cutscene_waypoint_reached = 33,
      on_difficulty_settings_changed = 60,
      on_entity_cloned = 117,
      on_entity_damaged = 97,
      on_entity_died = 4,
      on_entity_renamed = 57,
      on_entity_settings_pasted = 31,
      on_entity_spawned = 129,
      on_force_cease_fire_changed = 143,
      on_force_created = 27,
      on_force_friends_changed = 142,
      on_forces_merged = 101,
      on_forces_merging = 28,
      on_game_created_from_scenario = 119,
      on_gui_checked_state_changed = 3,
      on_gui_click = 1,
      on_gui_closed = 84,
      on_gui_confirmed = 138,
      on_gui_elem_changed = 67,
      on_gui_location_changed = 139,
      on_gui_opened = 83,
      on_gui_selected_tab_changed = 140,
      on_gui_selection_state_changed = 58,
      on_gui_switch_state_changed = 141,
      on_gui_text_changed = 2,
      on_gui_value_changed = 85,
      on_land_mine_armed = 100,
      on_lua_shortcut = 34,
      on_marked_for_deconstruction = 20,
      on_marked_for_upgrade = 114,
      on_market_item_purchased = 53,
      on_mod_item_opened = 82,
      on_picked_up_item = 5,
      on_player_alt_selected_area = 50,
      on_player_ammo_inventory_changed = 36,
      on_player_armor_inventory_changed = 35,
      on_player_banned = 108,
      on_player_built_tile = 45,
      on_player_cancelled_crafting = 96,
      on_player_changed_force = 56,
      on_player_changed_position = 81,
      on_player_changed_surface = 51,
      on_player_cheat_mode_disabled = 89,
      on_player_cheat_mode_enabled = 88,
      on_player_configured_blueprint = 70,
      on_player_crafted_item = 13,
      on_player_created = 24,
      on_player_cursor_stack_changed = 29,
      on_player_deconstructed_area = 69,
      on_player_demoted = 76,
      on_player_died = 41,
      on_player_display_resolution_changed = 93,
      on_player_display_scale_changed = 94,
      on_player_driving_changed_state = 26,
      on_player_dropped_item = 54,
      on_player_fast_transferred = 124,
      on_player_gun_inventory_changed = 37,
      on_player_joined_game = 43,
      on_player_kicked = 109,
      on_player_left_game = 44,
      on_player_main_inventory_changed = 32,
      on_player_mined_entity = 65,
      on_player_mined_item = 8,
      on_player_mined_tile = 46,
      on_player_muted = 86,
      on_player_pipette = 92,
      on_player_placed_equipment = 38,
      on_player_promoted = 75,
      on_player_removed = 73,
      on_player_removed_equipment = 39,
      on_player_repaired_entity = 123,
      on_player_respawned = 42,
      on_player_rotated_entity = 19,
      on_player_selected_area = 49,
      on_player_setup_blueprint = 68,
      on_player_toggled_alt_mode = 122,
      on_player_toggled_map_editor = 116,
      on_player_trash_inventory_changed = 102,
      on_player_unbanned = 110,
      on_player_unmuted = 87,
      on_player_used_capsule = 74,
      on_post_entity_died = 128,
      on_pre_chunk_deleted = 125,
      on_pre_entity_settings_pasted = 30,
      on_pre_ghost_deconstructed = 91,
      on_pre_player_crafted_item = 95,
      on_pre_player_died = 40,
      on_pre_player_left_game = 103,
      on_pre_player_mined_item = 11,
      on_pre_player_removed = 130,
      on_pre_robot_exploded_cliff = 126,
      on_pre_surface_cleared = 105,
      on_pre_surface_deleted = 63,
      on_put_item = 9,
      on_research_finished = 18,
      on_research_started = 17,
      on_resource_depleted = 25,
      on_robot_built_entity = 14,
      on_robot_built_tile = 47,
      on_robot_exploded_cliff = 127,
      on_robot_mined = 16,
      on_robot_mined_entity = 64,
      on_robot_mined_tile = 48,
      on_robot_pre_mined = 15,
      on_rocket_launch_ordered = 111,
      on_rocket_launched = 10,
      on_runtime_mod_setting_changed = 59,
      on_script_path_request_finished = 112,
      on_sector_scanned = 7,
      on_selected_entity_changed = 52,
      on_string_translated = 144,
      on_surface_cleared = 104,
      on_surface_created = 61,
      on_surface_deleted = 62,
      on_surface_imported = 120,
      on_surface_renamed = 121,
      on_technology_effects_reset = 99,
      on_tick = 0,
      on_train_changed_state = 23,
      on_train_created = 66,
      on_train_schedule_changed = 107,
      on_trigger_created_entity = 22,
      on_trigger_fired_artillery = 134,
      on_unit_added_to_group = 132,
      on_unit_group_created = 131,
      on_unit_removed_from_group = 133,
      script_raised_built = 77,
      script_raised_destroy = 78,
      script_raised_revive = 79
    },
    ...
I can act on an event being present or not present.
It's almost like you're posting without actually reading the thread or informing yourself of what's being discussed just to get your post count up for the sake of having a post count:
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.
As long as the events API page is accurate, I'm already writing a parser to convert it to LUA, so each "Contains" requirement does actually contain what it does, only what it is supposed to, and all the types match. Pretty easy to do, tbh. Work smarter not harder. One a skeleton is done, anyone can c+p for their own validation library (out and in), and it's proof it's not a stupid concept to validate your damn data when they already do it all over the place.
You seem to not understand what's being discussed here (the removal of script.raise_event), nor why it is being discussed.
I could explain it here again, but i'm not going to because it has been discussed and explained at length over the past few pages.
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.
Unless that is not what the topic was, correct me.
entity.damage raises on_entity_damaged internally.
What's the issue?

What's wrong with on_player_changed_position and on_player_changed_surface?
Again, you have not given any reason why those events need to be script.raise_event'd.
damage has a useful function for it (not near the define at all of course), but what about movement? built? Does every single event have a corresponding function we can use to trigger the raising of that event?

on_player_changed_position will be triggered constantly, and on_player_changed_surface is not necessary for a character to be teleported. You're the warptorio2 dev and you can't imagine any reason someone would want to know when someone teleported without having to check every single movement? Ironically, on_player_changed_position doesn't fire if the player moved surface but stayed at the same position. Funny.
Yes it does.

The following events are raised when you place a blueprint:
https://lua-api.factorio.com/latest/eve ... n_put_item
https://lua-api.factorio.com/latest/eve ... ilt_entity
These always fire for non-character actions too, and without explicitly checking for a blueprint in their cursor, I can't act on it. Even when I know they had a blueprint tool, I have to check where they aimed it, go through the blueprint, procedurally determine the bounds of the blueprint, map it to a real area, and work from there, instead of a single event that tells me where they pointed it and the area it consumed immediately. An example for this for is when people don't want cliffs deconstructed with blueprints. I have to use janky workarounds, and this is only for cliff entities. If someone wanted to act on blueprinting without jumping through all the hoops, I could raise an on_character_used_blueprint event, and pass the information to them, without remote interfacing or them having to implement any of my code.
You clearly didn't read the thread. Mods like bluebuild need script.raise_event to simulate certain player actions.

Code: Select all

      script_raised_built = 77,
      script_raised_destroy = 78,
      script_raised_revive = 79
Are you srs. Do you think bluebuild should be explicitly forbidden from raising base-game events like on_put_item, entity.destroy, and entity.revive? I already have enough arguments with the devs implementing things consistently, and as-is, we can generate base events, and if that is taken away, we're either stuck with raising no events, using what will be a limited selection of actions to trigger events, or raising custom events that nobody is going to listen to.
I have mods! I guess!
Link
PyroFire
Filter Inserter
Filter Inserter
Posts: 356
Joined: Tue Mar 08, 2016 8:18 am
Contact:

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

Post by PyroFire »

Honktown wrote: ↑Thu Dec 19, 2019 11:46 am Unless that is not what the topic was, correct me.
Correction:
Rseding91 wrote: ↑Fri Nov 08, 2019 12:22 am The more I think about it the more I think it just doesn't make sense to me for mods to really ever raise game events.
--------------
Honktown wrote: ↑Thu Dec 19, 2019 11:46 am damage has a useful function for it (not near the define at all of course), but what about movement? built? Does every single event have a corresponding function we can use to trigger the raising of that event?
I have already been through every single event and listed the ones that do, and do not have a triggering function, along with trying to come up with use-cases for each event that does not have one. This is my contribution, what is yours?

For the second time,
PyroFire wrote: ↑Thu Dec 19, 2019 9:32 am entity.damage raises on_entity_damaged internally.
What's the issue?
What's wrong with on_player_changed_position and on_player_changed_surface?
Again, you have not given any reason why those events need to be script.raise_event'd.
------------------
Honktown wrote: ↑Thu Dec 19, 2019 11:46 am Are you srs. Do you think bluebuild should be explicitly forbidden from raising base-game events like on_put_item, entity.destroy, and entity.revive? I already have enough arguments with the devs implementing things consistently, and as-is, we can generate base events, and if that is taken away, we're either stuck with raising no events, using what will be a limited selection of actions to trigger events, or raising custom events that nobody is going to listen to.
I have already been through every single event and listed the ones that do, and do not have a triggering function, along with trying to come up with use-cases for each event that does not have one. This is my contribution, what is yours?

I don't want this thread to turn into a "explain to Honktown how they are wrong while they don't accept it" thread, so i'll leave it at that, at least until you or someone else has something concrete or substantial to contribute to the discussion.
PyroFire
Filter Inserter
Filter Inserter
Posts: 356
Joined: Tue Mar 08, 2016 8:18 am
Contact:

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

Post by PyroFire »

Considering the difficulty in coming up with use-cases for script.raise_event, it is reasonable to consider that any mod authors that are currently using it for its intended purpose (cross-mod compatability) are doing something very specific that was resorted to after some level of tinkering, and already involves multiple mods to start with, and therefore, by now, at least one of those authors would have seen or been alerted to the discussion by now / community (and has the ability to fix the problem or request api additions). Anyone who is using it for unintended purposes (re-raising an event because they didn't want to self-contain their program, aka just do DoMyBuildFunctionAgain(stuff)), well... As seen with Honktown, the different contexts of the events can be confusing, and that, should any mod be relying on this method to function, it literally deserves to be broken for the same reason that event filters were added in the first place, as if to say "You are doing something so horrifyingly inefficient that we actually consider such behaviour as a bug. And as such, this bug is now fixed".
Locked

Return to β€œModding discussion”