Re-opened: breaking change/fix: script.raise_event rework

[Rseding91]

Old topic: 77550

So, it has been some time since that last topic and I think enough opinions have changed to warrant re-opening this topic.

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

Additional info

Any time a mod would use one of these script.raise_X(...) handlers the game would automatically include a 'mod_name=*name-of-source-mod*' field + an 'extra' field that can contain what ever the calling mod wants it to contain.

I still want to make this change because I feel like it's one of the key points of the current modding event system that is just "broken". If a mod currently wants their event handling to work correctly in all the existing ways it can operate they can't assume *any* parameters in the event logic are correct and have to re-implement validation on the mod side + never use event filtering. That seems completely broken. With this change all of that would go away.

I want to see what opinions are like today - 5+ months later.

[Mylon]

I originally resisted because I liked the flexibility to do weird stuff. But I've since found ways to do stuff I need without using this function. Well that and now resource.deplete() exists so I'm happy.

I'm okay with removing it now.

[Bilka]

I still support this, now with the following implementation details:

The following additions/changes are made to existing functionality:
- addition of script_raised_set_tile event
- "source" parameter added to script_raised_* credit a script raised action to something (entity/player).

Addition of "custom raise events" for:
-- Simulation events: 3 --
on_player_crafted_item
on_player_used_capsule
on_player_fast_transferred
-- Script raised events: 4 --
script_raised_built
script_raised_destroy
script_raised_revive
(script raised set tile)
-- General purpose events: 2 --
on_biter_base_built
on_market_item_purchased

This proposal is based on viewtopic.php?p=467964#p467964. The general summary of that post is "any events not listed above are raised automatically by the game or have a script_raised_* replacement", so the listed events should be a complete list of the necessary additions to have the rework not remove any mod functionality.

[Optera]

After testing filters for myself, I'm now firmly for this change.

Currently filters feel half baked as we have to always do all the silly checking just in case some mod decided to be smart and use system events instead of script_raised_built.

Code: Select all

script.on_event( defines.events.on_built_entity, Pole_Created, {{filter="type", type="electric-pole"}} )

local function Pole_Created(event)
  local entity = event.created_entity or event.entity
  if entity and entity.valid and entity.type == "electric-pole" then
  
  end
end

Restricting mods to only raise custom and on_script_ events would make filters more useful and we can actually trust data passed in system events.

[jan1i3]

I'm all for this change. Also means i can remove this disgusting Raise Event Protection mod (though it was a fun project)

I currently only rely on raising game events for on_entity_settings_pasted, but i can definitely change this to work without it, with just a tiny bit of behavior change if any.

[Boodals]

I don't like the idea of not being able to manually raise events. In one of my mods, I create an entity, then set some values on it, then manually raise the script_raised_built event. If I had to use the raise_event parameter in the create_entity call, then I would overwrite any values that other mods assign.
Obviously an explicit way to raise the script_raised_built event would solve this problem, but I'm worried that its a more general problem that would need to be solved for all events that will be raised automatically by the game. And I don't think the devs want to make a ton of functions to raise each event.
Theoretically this problem already exists for events that are always automatically raised, but I guess its not a problem because nobody has complained, and most things can be set up in the same call which creates the thing, or they're just so minor that its rare that two mods would even care about the thing, but its hard to know without looking at every mod out there.

Overall I'm for the change, but I predict a lot of backlash from both modders who aren't keeping up-to-date, and mod users who suddenly cant play their games (despite being on unstable). I also predict a lot of requests for functions to raise events in the first couple of weeks after the change - I expect the devs won't implement enough to cover every existing usage of raise_event, and some probably shouldn't be implemented at all, but requests will come nevertheless.



But I think the best solution is to disable (throw an error when calling) raise_event for any event other than ones which have new explicit data verification. Think of this as an alternative version of the script.raise_X idea which doesn't break mods other than: ones which are already broken by raising invalid event data, and mods which raise events which will no longer be supported (which will be a problem either way).

Compared to script.raise_X:

• Roughly the same number of lines of code (or less).
• Less mods breaking (thus less users/modders complaining).
• No need for however-many extra functions cluttering the API documentation, although it would need some way to indicate which events can be raised by raise_event instead.

Rereading the last thread, it seems this was proposed, but there wasn't any responses from any of the devs.

[Bilka]

I also predict a lot of requests for functions to raise events in the first couple of weeks after the change - I expect the devs won't implement enough to cover every existing usage of raise_event, and some probably shouldn't be implemented at all, but requests will come nevertheless.

The current list of "functions to be added for events that aren't raised automatically" is 9 functions, I posted it above. Could you explain which events specifically you are worried about? JanSharp mentions entity_settings_pasted, but beyond that there are no examples for other events that would be missing.
This "please list the specific events" request also extends to the "I want to raise my event after the function that does the thing" point that you make - which events would be affected by this? I cannot think of any, because (1) you cannot surpress the automatic events that the game *already* generates and (2) the only events you raise by function parameter are the script_raised_*, which are included in the above post as "events to be able to raise manually".

But I think the best solution is to disable (throw an error when calling) raise_event for any event other than ones which have new explicit data verification.

This would move the error from "starting a game" to "calling the function", thus making the error partially hidden and hard to test for. This will make it much harder to migrate mods, so I disagree with this approach.

[Boodals]

Could you explain which events specifically you are worried about?

Download every mod ever and do a search for "raise_event", those will be the ones requested. Maybe it's all of them, maybe its just the ones you've already listed.

This "please list the specific events" request also extends to the "I want to raise my event after the function that does the thing" point that you make - which events would be affected by this? I cannot think of any, because (1) you cannot surpress the automatic events that the game *already* generates and (2) the only events you raise by function parameter are the script_raised_*, which are included in the above post as "events to be able to raise manually".

I can't think of every possible use of every event, but if another modder does something similar to what I do (create/do a thing that other mods might want to know about, apply the default settings to the thing, raise an event for the thing), and the event is one of the "any events not listed above are raised automatically by the game", then they can no longer apply the settings before raising the event, so any changes they make will overwrite what other mods have done.
Yeah its a pretty specific and minor problem, but it is still a problem. The right answer is probably just to not worry about it and deal with it if anyone ever brings it up.

But I think the best solution is to disable (throw an error when calling) raise_event for any event other than ones which have new explicit data verification.

This would move the error from "starting a game" to "calling the function", thus making the error partially hidden and hard to test for. This will make it much harder to migrate mods, so I disagree with this approach.

The error will be in "calling the function" either way. Its not an error until lua executes the script.raise_event, as there's no way for lua to know whether the script table will contain a function called raise_event until that line is executed. Just to double check, I put "script.this_isnt_checked_when_the_function_loads_only_when_its_called()" into my mod and it loads just fine, it's only when the function is called does it throw an error.

[raiguard]

I'm all for this change, 100%. Though I do agree that it would probably be better if it was still raise_event, but would error if trying to raise an unsupported event. That way any mods that aren't already doing things wrong won't require any changes to work on the new system.

[Rseding91]

I'm all for this change, 100%. Though I do agree that it would probably be better if it was still raise_event, but would error if trying to raise an unsupported event. That way any mods that aren't already doing things wrong won't require any changes to work on the new system.

After talking about this on stream today, I'd be ok with that.

[Optera]

I'm all for this change, 100%. Though I do agree that it would probably be better if it was still raise_event, but would error if trying to raise an unsupported event. That way any mods that aren't already doing things wrong won't require any changes to work on the new system.

Too many negations in that sentence.
Does that mean mods already using only script.raise_event(script_raised_built, {entity = entity}) will not have to be changed?
While mods using script.raise_event(on_built_entity, {created_entity = entity}) would get an error?

Sounds like best of both worlds to me. Not breaking mods already behaving well while forcing naughty children to finally use best practice.

[Mylon]

What was the roadblock preventing that kind of event validate on script.raise_event() before?

[Rseding91]

What was the roadblock preventing that kind of event validate on script.raise_event() before?

Manually writing all of the validation for each event vs just saying 90% of the events you can't raise.

[justarandomgeek]

If I've understood correctly, the proposal has basically evolved into "raise_event can only be used for script_raised_* and custom mod events", which sounds pretty good to me.

[jan1i3]

I have (finally) downloaded all (latest 0.18) mods.
I wrote a query to get all raise_event calls with game events (it might miss some if people use weird event systems/handlers)
the result is... interesting, but less useful than i was hoping it would be, however if you think it can be useful i can post the result here.

i specifically have

1. All mod names that raise game events
2. The total count of event X being raised
3. The total counts of mod X raising Y
4. The exact locations of where they are being raised (like this: modname / relative path - line number: event name)

(if you only want some parts of that, well it's a numbered list, so point to those specifically)

[jan1i3]

I thought about it more, and also with some external input i've decided i don't want to post any of this in here.
except 2), which is this:

Code: Select all

2) total events: 138

40  on_built_entity
 6  on_console_chat
 1  on_entity_cloned
 1  on_entity_died
 3  on_entity_settings_pasted
 3  on_entity_spawned
 2  on_gui_click
 1  on_player_armor_inventory_changed
 4  on_player_built_tile
 2  on_player_cursor_stack_changed
 2  on_player_driving_changed_state
10  on_player_mined_item
 1  on_player_placed_equipment
 1  on_player_respawned
 1  on_player_rotated_entity
 8  on_pre_player_mined_item
 3  on_resource_depleted
 3  on_robot_built_entity
 5  on_robot_built_tile
 2  on_robot_mined
 1  on_robot_pre_mined
21  script_raised_built
17  script_raised_destroy

if somebody wants the whole list and i can trust them to not misuse it, PM me and i'll send it to you

[Optera]

Your list is nice, but skews toward the following conclusion:
28% already raise script_raised and wouldn't be affected
29% can easily replace on_built_entity with script_raised_built
43% might actually break but can probably work around the change

A more useful list would be how many mods use only script_raised vs how many still raise on_x.

PS: not sure how you interpret misuse of information anyone can generate by themselves with some effort.

[justarandomgeek]

A more useful list would be how many mods use only script_raised vs how many still raise on_x.

Most of the ones using script_raised_ don't do anything else, so it does indeed look like it's a before/after split and just old mods not fully updated to raise the new events. Within the ones using script_raised_*, two raise on_entity_settings_pasted, one on_player_placed_equipment, one on_player_rotated_entity.

PS: not sure how you interpret misuse of information anyone can generate by themselves with some effort.

Probably the same concern i expressed when asking for the full list: It could be seen as "publicly shaming" the named mods potentially

[Optera]

Probably the same concern i expressed when asking for the full list: It could be seen as "publicly shaming" the named mods potentially

That's what it's called now? A few years ago we called it "raising awareness to shoddy coding".

Most of the ones using script_raised_ don't do anything else, so it does indeed look like it's a before/after split and just old mods not fully updated to raise the new events. Within the ones using script_raised_*, two raise on_entity_settings_pasted, one on_player_placed_equipment, one on_player_rotated_entity.

That's the interesting data, showing which events should be made available to mods without waiting for moders to file api requests.

[jan1i3]

That's what it's called now? A few years ago we called it "raising awareness to shoddy coding".

We don't have to call it that, or see it that way. the thing is that there is potential for people seeing that way, as i'll go over more below

PS: not sure how you interpret misuse of information anyone can generate by themselves with some effort.

You are correct. I am just worried that it could be seen like me saying "all these mods and the modders making them are doing it wrong".
However, that is not my intention, it is just showing which mods currently use these features, which may be required to achive what they were trying to achive, so there is no way to say that it is "right or wrong" from just this data.
And even if something turns out to be replaceable by a non raise event way of doing it, that still doesn't make it a bad mod or person.

However anybody else could take that data and be like "haha, look at this popular mod that uses raise event 10 billion times, that dev must be extra stupid" (or worse, i'm bad at insults), there is nothing i can do to prevent that from happening. While it's not my fault for somebody doing that, it has the potential to cause all kinds of damage, including makeing this thread unproductive.

And i don't want that, i want people working together and finding a solution to this raise event issue.

So please keep in mind that we are not alanyzing people.
We are analyzing code, which is also why the only data used to identify where the data came from is the mod name, file name and line number.

script_raised_flags_occurrances_total.txt

(25.09 KiB) Downloaded 97 times

results for the C# regex(s) "raise_X[^=]*=\s*true"

raise_event_occurrances_total.txt

(15.44 KiB) Downloaded 117 times

results for the C# regex

Code: Select all

new Regex(@"^(?<prefix>.*?)raise_event(?<event>[^,]*).*$",
    RegexOptions.Multiline | RegexOptions.Compiled)

and then removing all results where 'prefix' contains "--", and searching 'event' for all event names
(so for example this still includes raise_event calls in multiline comments, or might miss some if event systems are used which use a different syntax for raising events)