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

[Honktown]

I think one room for confusion is there hasn't been a "full" update/explanation on staff opinions:

Real events:
Aren't validated
Speculated on being removed from being raised

Mod events:
Aren't validated
(not speculated to be removed?)

Both let the player pass in "garbage" (what is the garbage? Is it wrong information, or failing to assign information?)

Code: Select all

raise_event(event, table)
Raise an event.

Parameters
event :: uint: ID of the event to raise
table: Table with extra data. This table will be passed to the event handler.

By "extra data" is it meant the actual data we need to pass for events, because it definitely doesn't sound like there's a monitor that follows what values were changed if it was non-extra data (and builds events from that).

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.

I could use some further clarity on what is and isn't checked, if you would. This is a personal request, because I've ran into values being discarded, values causing errors, etc, so if there were any sense of structure with what's going on it'd make things easier. If some garbage can get through events but others can't, I'd unsurprisingly be confused.

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.

The game is C++. When a C++ "game event" happens the logic goes over the event listeners and lets them know it happened. That eventually makes its way into the "put the C++ data into the Lua state that's listening to this event and run the Lua event handler". For example - the entity-died event does that through this C++ function:

Code: Select all

//? Called when an entity dies.
//? Can be filtered using $ref(LuaEntityDiedEventFilters)
//? $param(entity, LuaEntity)
//? $param(cause, LuaEntity, $optional) The entity that did the killing if available.
//? $param(loot, LuaInventory) The loot generated by this entity if any.
//? $param(force, LuaForce, $optional) The force that did the killing if any.
bool LuaGameScript::pushOnEntityDied(const GameAction& event)
{
  auto& data = event.getEntityDiedData();
  if (data.entity->isToBeDeleted())
    return false;

  // entity
  new LuaEntity(data.entity, this->luaState);
  lua_setfield(this->luaState, -2, "entity");

  // cause
  if (data.cause && !data.cause->isToBeDeleted())
  {
    new LuaEntity(data.cause, this->luaState);
    lua_setfield(this->luaState, -2, "cause");
  }

  // loot
  new LuaInventory(data.inventory, this->getMap(), this->luaState);
  lua_setfield(this->luaState, -2, "loot");

  // force
  if (!data.forceID.isZero() && this->getMap().forceManager.getForces().has(data.forceID))
  {
    new LuaForce(this->getMap(), data.forceID, this->luaState);
    lua_setfield(this->luaState, -2, "force");
  }

  return true;
}

This function uses the C++ entity-died event data struct which looks like this:

Code: Select all

  struct EntityDiedData
  {
    EntityWithHealth* entity = nullptr;
    Entity* cause = nullptr;
    TargetableInventory* inventory = nullptr;
    ForceID forceID;
  };

On the Lua side it gets:

entity = LuaEntity (the entity that died)
cause = LuaEntity (the entity that killed the entity - optional)
loot = LuaInvnetory (the loot generated by this entity dying)
force = LuaForce (the force that killed this entity - optional)

Now, that function takes a C++ struct and creates the Lua wrappers that get passed to the Lua event handler. That is a 1-way trip. You can't just use that same function to "validate the data gotten from lua is correct for this event".

I've written the function that would have to be used to do that validation just for this example. Note the function isn't actually complete because there are parts that simply don't work. Also note it took me 20 minutes to write this and I already knew literally every line I would have to write and where every function was that I would need to use.

Code: Select all

int32_t LuaBootstrap::luaRaiseEntityDied(lua_State* L)
{
  LuaHelper::argCheck(L, 1);
  luaL_checktype(L, 1, LUA_TTABLE);

  GameActionData::EntityDiedData eventData;

  lua_getfield(L, 1, "entity");
  Entity* entity = LuaHelper::parseEntity(L, -1, nullptr);
  lua_pop(L, 1);
  if (!entity->isEntityWithHealth())
    throw ScriptException("Given entity is not an entity with health.");
  eventData.entity = static_cast<EntityWithHealth*>(entity);

  lua_getfield(L, 1, "cause");
  if (!lua_isnil(L, -1))
    eventData.cause = LuaHelper::parseOptionalEntity(L, -1, nullptr);
  lua_pop(L, 1);

  lua_getfield(L, 1, "loot");
  luaL_checktype(L, -1, LUA_TTABLE);
  LuaInventory* luaInventory = dynamic_cast<LuaInventory*>(LuaHelper::parseLuaBindableObject(L, -1));
  lua_pop(L, 1);
  if (!luaInventory)
    throw ScriptException("Given object is not a LuaInventory.");
  if (!luaInventory->valid())
    throw ScriptException("Given inventory doesn't exist anymore.");
  // This doesn't work because the event is setup to store TargetableInventory* and there's currently no way to get that back from LuaInventory
  // I would have to go add a way to get TargetableInventory* from LuaInventory (can be done)
  // eventData.inventory = luaInventory->getTargetableInventory();

  lua_getfield(L, 1, "force");
  if (!lua_isnil(L, -1))
    eventData.forceID = LuaHelper::parseForce(L, this->script->getMap(), -1);
  lua_pop(L, 1);

  this->script->eventDispatcher->run(LuaEventType::ON_ENTITY_DIED,
                                     this->script->getMap().entityTick,
                                     &LuaGameScript::pushOnEntityDied, GameAction(GameAction::EntityDied, eventData));
  return 0;
}

This kind of code is generable. Each type of value has a means of validating it (minus the exceptions when looking for a specific type of property, which makes it a different dictionary value anyway)

Thank you for sharing it actually, it's coded "regularly" but it does give some insights.

I have already written professional code which took a data file, processed it in Perl, and barfed it out in C to get around no dynamic allocation in our context. Fun stuff.

Edits. Of course.

[PyroFire]

This kind of code is generable.

Making sweeping changes that allow the 1% to continue to write bad mods makes no sense to me.

[Honktown]

Letting anything raise an event that's unverified is a "problem", whether it's a built-in event or not. Mods will have to explicitly subscribe to custom events, and trust each custom event is valid. Good for the developers because they can say "go to the mod author, not me", same problems and worse for modders.

I'm going to need a validator anyway until I can be certain events can't fill it with trash. Dare to dream~~

[Rseding91]

Events fired from the game are always validated. Events fired from mods never are. So, validating game events just wastes time because they're always valid. Unless you have bad mods passing bad data... which is the entire point of this thread to fix that.

[Honktown]

Thanks for clarifying ; I started the validator based on known-good definitions/standard definition for events. Extending it to custom events is easy after that, by entering them like the standard definitions. Of course if I upload it as a mod I won't include the scripts I'm using to generate the validator functions. I'll put those on github probably.

[Honktown]

Now to get it to spit out something that looks like the C++ code. And create a function that exports the current dictionary, so I don't need the normal events page any more. And create functions in Lua to make stuff that looks like that (should be easy enough with the dictionary, if they've provided the name, args, and types).

barfaroni.png (153.87 KiB) Viewed 2603 times

For the morbidly curious:

https://drive.google.com/open?id=1-Topt ... 6irTvTtc1Q

[Klonan]

Now to get it to spit out something that looks like the C++ code. And create a function that exports the current dictionary, so I don't need the normal events page any more. And create functions in Lua to make stuff that looks like that (should be easy enough with the dictionary, if they've provided the name, args, and types).

I don't really know what your're doing or what you're saying, but its starting to feel off-topic

[Honktown]

Code: Select all

...//? Called before one or more chunks are deleted using LuaSurface::delete_chunk.
//? $param(surface_index, uint) 
//? $param(positions, array of ChunkPosition)  The chunks to be deleted.
bool LuaGameScript::pushOnPreChunkDeleted(const GameAction& event){
	return true;
}

//? Called when an entity dies. 
//? Can be filtered using $ref(LuaEntityDiedEventFilters)
//? $param(entity, LuaEntity) 
//? $param(cause, LuaEntity, $optional)  The entity that did the killing if available.
//? $param(loot, LuaInventory)  The loot generated by this entity if any.
//? $param(force, LuaForce, $optional)  The force that did the killing if any.
bool LuaGameScript::pushOnEntityDied(const GameAction& event){
	return true;
}

//? Called when LuaGuiElement switch state is changed (related to switches).
//? $param(element, LuaGuiElement)  The switch whose switch state changed.
//? $param(player_index, uint)  The player who did the change.
bool LuaGameScript::pushOnGuiSwitchStateChanged(const GameAction& event){
	return true;
}

//? Called when a surface is renamed.
//? $param(surface_index, uint) 
//? $param(old_name, string) 
//? $param(new_name, string) 
bool LuaGameScript::pushOnSurfaceRenamed(const GameAction& event){
	return true;
}...

No fun allowed huh

Was thinking about the events: if the structure of an event had an ID, fixed-length array of parameter types, and fixed-length array of pointers, then a fixed-size definition of an event can be used, with a const array of function pointers to validate individual parameters. Base-game events can have the function-pointers inlined into functions (the parameter indices are constant). A constant-sized buffer would be usable to further increase performance, with an option for paging if the amount of events generated in one tick exceeds the buffer (allocations would dwarf the performance of the const buffer usage anyway).

If an event is registered without verifiable types, then it gets rejected. Any event that can have verifiable types has the types verified if it wasn't generated by the game.

Benefits: developers get to have an iron fist over what mods can and cannot pass to each-other. Players can expect standardized, valid information, or no event. Event pushes/pulls can be standardized, unless there's janky per-event modifications during a push-pull.

Yes I'm going to keep playing with this, no I don't care about dev opinions. I'm glad rseding spilled the beans on some of the internal code mechanics.

[Koub]

[Koub] This thread seems to have outlived its original purpose. I'm locking it.