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

[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?

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

[Deadlock989]

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]

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]

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

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

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.

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

[justarandomgeek]

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]

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?

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

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.

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.

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

[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!

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

[Rseding91]

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

[Klonan]

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]

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]

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]

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.

[eradicator]

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.

[justarandomgeek]

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?