Page 1 of 5
Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Mon Nov 04, 2019 3:24 pm
by Rseding91
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?
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Mon Nov 04, 2019 4:56 pm
by quyxkh
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Mon Nov 04, 2019 6:40 pm
by Deadlock989
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Tue Nov 05, 2019 4:51 pm
by justarandomgeek
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Wed Nov 06, 2019 10:33 am
by Bilka
+1 to this change. It would prevent this ridiculous behavior:
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Wed Nov 06, 2019 6:34 pm
by eradicator
-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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Wed Nov 06, 2019 6:53 pm
by eradicator
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Thu Nov 07, 2019 12:17 am
by justarandomgeek
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Thu Nov 07, 2019 3:28 am
by quyxkh
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?
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Thu Nov 07, 2019 8:33 pm
by eradicator
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Fri Nov 08, 2019 12:22 am
by Rseding91
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Fri Nov 08, 2019 2:39 am
by justarandomgeek
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!
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Fri Nov 08, 2019 11:05 am
by Optera
-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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Fri Nov 08, 2019 11:13 am
by Rseding91
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Fri Nov 08, 2019 11:15 am
by Klonan
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
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Sat Nov 09, 2019 5:59 pm
by Mylon
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Sat Nov 09, 2019 6:01 pm
by justarandomgeek
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Sat Nov 09, 2019 6:14 pm
by Bilka
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Sat Nov 09, 2019 7:52 pm
by eradicator
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.
Re: Breaking change/fix: script.raise_event rework - opinions wanted
Posted: Sun Nov 10, 2019 9:06 pm
by justarandomgeek
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?