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

Place to post guides, observations, things related to modding that are not mods themselves.
Rseding91
Factorio Staff
Factorio Staff
Posts: 9973
Joined: Wed Jun 11, 2014 5:23 am
Contact:

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

Post by Rseding91 » 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.

My proposal:

Change script.raise_event so it only allows custom event IDs. Mods would no longer be allowed to call script.raise_event(defines.events.X, ...). Instead, for key events that make sense that a mod may want to trigger it - there would be script.raise_X(...) for that specific event. The raise_X(...) would accept the exact parameters that the event requires and the C++ logic could make sure that mods never do raise_event(...) with bad data.

There 2 obviously downsides to this:

1. It's a breaking change. So, we wouldn't do it during one of the minor releases but some larger release where it's already known we plan on breaking things.

2. If we (Wube) don't provide a function for raise_X(...) for a given event then mods just can't raise it.

#1 is kind of unavoidable and if we're breaking things I think it would mostly be "ok". #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.

The benefits:

* Mods would never need to worry that other mods pass garbage data in events.

* We (Wube) can add event filtering for the script-raised-X events.


What I want to know is: what does the modding community think about this?
If you want to get ahold of me I'm almost always on Discord.

quyxkh
Filter Inserter
Filter Inserter
Posts: 776
Joined: Sun May 08, 2016 9:01 am
Contact:

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

Post by quyxkh » Mon Nov 04, 2019 4:56 pm

I don't see any material difference between checking the arguments when script.raise_event() gets a predefined event number and refusing to allow scripts to do that but then offering alternate ways for scripts to do exactly that.

User avatar
Deadlock989
Smart Inserter
Smart Inserter
Posts: 1482
Joined: Fri Nov 06, 2015 7:41 pm

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

Post by Deadlock989 » Mon Nov 04, 2019 6:40 pm

Rseding91 wrote:
Mon Nov 04, 2019 3:24 pm
The benefits:

* Mods would never need to worry that other mods pass garbage data in events.

* We (Wube) can add event filtering for the script-raised-X events.


What I want to know is: what does the modding community think about this?
Yes please. Especially the ban on garbage use of garbage events part. Filtering, eh, but might be handy.

justarandomgeek
Fast Inserter
Fast Inserter
Posts: 245
Joined: Fri Mar 18, 2016 4:34 pm
Contact:

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

Post by justarandomgeek » Tue Nov 05, 2019 4:51 pm

I generally like this change, and would propose one further one: if you're changing how custom events work anyway, it would be nice to register them as names so that you can provide an event without having to provide a remote.call to list your events (especially if you otherwise don't need the remote interface), just some docs for the modders who consume your event.

Edit to expand on this:

Have generate_event_name() actually take a string name for the event, and allow consumers to listen for it by passing that string to on_event (which already takes strings anyway for custom-inputs). It probably still makes sense for generate_event_name to return the ID, and to use that with raise_event, just to avoid passing the string back and forth for every event. This would then allow the mod providing the event to just list the names in their dev docs, and factorio would handle matching that up to an ID for the consumer of that event.

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

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

Post by Bilka » Wed Nov 06, 2019 10:33 am

+1 to this change. It would prevent this ridiculous behavior:
Image

Not only does the event not get filtered, so now I have to keep around the old "check name etc" code; the mod is also not including "stack" which is a required field. A system that allows this behavior is complete garbage from the perspective of a mod author that wants to just use an event the way the documentation says it works.
I'm an admin over at https://wiki.factorio.com. Feel free to contact me if there's anything wrong (or right) with it.

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

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

Post by eradicator » Wed Nov 06, 2019 6:34 pm

-1

I don't see why the api name for this has to be changed. If a mod calls script.raise_event with garbage data then just don't raise the event, and preferably throw an error so the broken mod can be fixed. Requiring us mod authors to request a new "raise_x" every time we need one for a weird edge case is a lose-lose situation. I waste time trying to convince you that my edge case is worth it, and you waste time implementing an ugly workaround for something that could just work transparently in the background.
Bilka wrote:
Wed Nov 06, 2019 10:33 am
A system that allows this behavior is complete garbage from the perspective of a mod author that wants to just use an event the way the documentation says it works.
I always assumed it already did this.
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.

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

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

Post by eradicator » Wed Nov 06, 2019 6:53 pm

The main problem here is: If i fail at convincing you that my edge case is worth implementing an extra "raise_x" then my only option would be to not raise anything at all. This is imho similar to the "/editor mode does not raise on_destroyed" problem from a few days back. Sure mods can (and should) be coded to be resilient to missed events. But that resilience is never going to look as good to the user as a properly handled event would.

Let's try a (hopefully weird) example:
on_gui_click: Can be used to create a hotkey for a button in another mods gui without the other mod having to do a thing to make it work.
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.

justarandomgeek
Fast Inserter
Fast Inserter
Posts: 245
Joined: Fri Mar 18, 2016 4:34 pm
Contact:

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

Post by justarandomgeek » Thu Nov 07, 2019 12:17 am

eradicator wrote:
Wed Nov 06, 2019 6:53 pm
The main problem here is: If i fail at convincing you that my edge case is worth implementing an extra "raise_x" then my only option would be to not raise anything at all.
No, your option is to raise your own event.

quyxkh
Filter Inserter
Filter Inserter
Posts: 776
Joined: Sun May 08, 2016 9:01 am
Contact:

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

Post by quyxkh » Thu Nov 07, 2019 3:28 am

I don't understand
we have no way to verify anything given to us from script.raise_event
at all. It can't mean "we have no access to the arguments", it can't mean "we can't read or check the contents of lua tables". What can you do with burst parameters that you can't do with table entries?

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

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

Post by eradicator » Thu Nov 07, 2019 8:33 pm

justarandomgeek wrote:
Thu Nov 07, 2019 12:17 am
eradicator wrote:
Wed Nov 06, 2019 6:53 pm
The main problem here is: If i fail at convincing you that my edge case is worth implementing an extra "raise_x" then my only option would be to not raise anything at all.
No, your option is to raise your own event.
That is not an option at all. Because it requires every other mod to know what my special event is called and to actively support it. And for everyone to agree on magically sharing the same event id for the same edge case. We both know neither of these is ever going to happen.
quyxkh wrote:
Thu Nov 07, 2019 3:28 am
I don't understand
we have no way to verify anything given to us from script.raise_event
at all. It can't mean "we have no access to the arguments", it can't mean "we can't read or check the contents of lua tables". What can you do with burst parameters that you can't do with table entries?
This. I mean, i could do it on the lua side if i wanted. Just hardcode a table of expected event content for every event. Would need manual updating yea, but on the modding side that's not really new.
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.

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

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

Post by Rseding91 » 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.
If you want to get ahold of me I'm almost always on Discord.

justarandomgeek
Fast Inserter
Fast Inserter
Posts: 245
Joined: Fri Mar 18, 2016 4:34 pm
Contact:

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

Post by justarandomgeek » Fri Nov 08, 2019 2:39 am

Makes me wish (again) for full-text search of all mods published on the portal, to find out who's actually doing raise_event and where/why!

User avatar
Optera
Smart Inserter
Smart Inserter
Posts: 2275
Joined: Sat Jun 11, 2016 6:41 am
Contact:

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

Post by Optera » Fri Nov 08, 2019 11:05 am

-1

If I had to convince you (Wube) each time I needed an event for LTNs API developing it would've taken months instead of days.
With the current free system when Eduran needed more data or a completely new event all we had to do is start a discord conversation and we had the api changes worked out in hours.
With the proposed system we'd have to figure out the changes pass them to Wube and hope they get implemented at all.

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

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

Post by Rseding91 » Fri Nov 08, 2019 11:13 am

Optera wrote:
Fri Nov 08, 2019 11:05 am
-1

If I had to convince you (Wube) each time I needed an event for LTNs API developing it would've taken months instead of days.
With the current free system when Eduran needed more data or a completely new event all we had to do is start a discord conversation and we had the api changes worked out in hours.
With the proposed system we'd have to figure out the changes pass them to Wube and hope they get implemented at all.
It wouldn't have any impact on what you describe: custom events would be unchanged and you could still pass anything you want through them.
If you want to get ahold of me I'm almost always on Discord.

User avatar
Klonan
Factorio Staff
Factorio Staff
Posts: 3932
Joined: Sun Jan 11, 2015 2:09 pm
Contact:

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

Post by Klonan » Fri Nov 08, 2019 11:15 am

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.
Since we added the function call raising to the create_entity, revive_entity, and destroy_entity things, I haven't needed to raise any base game events manually

Mylon
Filter Inserter
Filter Inserter
Posts: 456
Joined: Sun Oct 23, 2016 11:42 pm
Contact:

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

Post by Mylon » Sat Nov 09, 2019 5:59 pm

Klonan wrote:
Fri Nov 08, 2019 11:15 am
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.
Since we added the function call raising to the create_entity, revive_entity, and destroy_entity things, I haven't needed to raise any base game events manually
I don't like the create_entity, revive_entity, or destroy_entity because these events aren't fired by anything in the base game. The context of these events is nearly identical to on_entity_built and on_entity_died, but the likelihood of mods supporting these events is reduced and thus the chance of compatibility bugs is far higher than if there were a solution to shape these as vanilla events.

justarandomgeek
Fast Inserter
Fast Inserter
Posts: 245
Joined: Fri Mar 18, 2016 4:34 pm
Contact:

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

Post by justarandomgeek » Sat Nov 09, 2019 6:01 pm

Mylon wrote:
Sat Nov 09, 2019 5:59 pm
I don't like the create_entity, revive_entity, or destroy_entity because these events aren't fired by anything in the base game. The context of these events is nearly identical to on_entity_built and on_entity_died, but the likelihood of mods supporting these events is reduced and thus the chance of compatibility bugs is far higher than if there were a solution to shape these as vanilla events.
The bug in this case is the mod that is not appropriately handling the script_raised_thing events.

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

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

Post by Bilka » Sat Nov 09, 2019 6:14 pm

Mylon wrote:
Sat Nov 09, 2019 5:59 pm
I don't like the create_entity, revive_entity, or destroy_entity because these events aren't fired by anything in the base game. The context of these events is nearly identical to on_entity_built and on_entity_died, but the likelihood of mods supporting these events is reduced and thus the chance of compatibility bugs is far higher than if there were a solution to shape these as vanilla events.
So you raise on_built_entity with missing parameters and break mods subscribing to it normally. I really prefer subscribing to "script_raised_built" over having my mods broken by mod authors raising bad events.
I'm an admin over at https://wiki.factorio.com. Feel free to contact me if there's anything wrong (or right) with it.

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

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

Post by eradicator » Sat Nov 09, 2019 7:52 pm

Bilka wrote:
Sat Nov 09, 2019 6:14 pm
Mylon wrote:
Sat Nov 09, 2019 5:59 pm
I don't like the create_entity, revive_entity, or destroy_entity because these events aren't fired by anything in the base game. The context of these events is nearly identical to on_entity_built and on_entity_died, but the likelihood of mods supporting these events is reduced and thus the chance of compatibility bugs is far higher than if there were a solution to shape these as vanilla events.
So you raise on_built_entity with missing parameters and break mods subscribing to it normally. I really prefer subscribing to "script_raised_built" over having my mods broken by mod authors raising bad events.
I have to admit that i always thought the whole script_raised_x system was an ugly hotfix for a legacy grown system. Imho it would be much better if there was exactly one event "on_entity_created" and one "on_entity_removed" that contain a bunch of optional parameters. If someone actually only wants to know when a "player" builds something they can filter the event (now even with built-in filters). Having to deal with 7 different events is meh.

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.
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.

justarandomgeek
Fast Inserter
Fast Inserter
Posts: 245
Joined: Fri Mar 18, 2016 4:34 pm
Contact:

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

Post by justarandomgeek » Sun Nov 10, 2019 9:06 pm

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.
Why bother wasting computing time configuring an entity that's just going to be destroyed?

Post Reply

Return to “Modding discussion”

Who is online

Users browsing this forum: No registered users