[PATCH] Trigger system

[Stuart Walpole]

• General-purpose trigger system

See this topic for forum discussion.

Here is the recent topic: http://www.wildfiregames.com/forum/index.php?showtopic=18211

[(none)]

Milestone Multiplayer Demo deleted

[O.Davoodi]

Map file

[O.Davoodi]

One important note before anything else: To implement the triggers, we need a way to serialize and deserialize js functions; something not present in out current engine. For the patch to work, I've made a temporary hack so that it doesn't crash the game but save games will be broken. So, to really implement this, we should first fix the serializer.

This patch is work in progress. It implements a basic trigger system. Triggers are scripts that are loaded after the simulation, but before the actual start of the game. They have full access to simulation components. To create a trigger, an "action" function must be made. Then, a trigger is registered using RegisterTrigger function, which binds the "action" to an "event". During the game, when those "events" occur, the "action" functions are called. Currently these are the implemented events:

OnEntityTookDamage

OnEntityKilled

OnStructureBuilt

OnConstructionStarted

OnTrainingFinished

OnTrainingQueued

OnResearchFinished

OnResearchQueued

Always (Special case, describes bellow)

OnUnitRangeFromEntity

Always event occurs every "updateInterval" defined by the creator of the triggers. It is an integer which describes the miliseconds interval by which the "Always" event occurs. As this could be performance heavy, it is disabled by default, so if a map maker really needs to check for something (for eg.) each second, it should enable this event manually. If his/her work is done with it, it can be disabled again.

OnUnitRangeFromEntity is another special case. For one, it "needs" to be updated. So for this to work, the update interval should be set.

It handles three closely related "events":

"OnUnitEnteredRangeFromEntity" which occurs when an entity moves into a radius from a specific entity.

"OnUnitLeftRangeFromEntity" which occurs when an entity that moves out of a radius from a specific entity.

"OnUnitMovedInsideRangeFromEntity" which occurs when an entity that moves inside of a radius from a specific entity. (doesn't leave it, but has already entered it).

One of the other important events to implement would be an "OnUnitRangeFromPoint" event that uses a point instead of an entity as the center of the radius.

It is going to be a big patch so comments and suggestions are welcome.

[O.Davoodi]

WIP patch for triggers

[historic_bruno]

Replying to Spahbod:

One important note before anything else: To implement the triggers, we need a way to serialize and deserialize js functions; something not present in out current engine. For the patch to work, I've made a temporary hack so that it doesn't crash the game but save games will be broken. So, to really implement this, we should first fix the serializer.

I don't see why serializing functions would be necessary for anything. If you're wanting to pass functions around the simulation state as data, I would suggest doing that by name instead and having whatever data they need as a simple object.

[O.Davoodi]

Replying to historic_bruno:

..., I would suggest doing that by name instead and having whatever data they need as a simple object.

Pardon me but I really don't understand what you mean. Could you elaborate a little more?

[sanderd17]

Replying to Spahbod:

Replying to historic_bruno:

..., I would suggest doing that by name instead and having whatever data they need as a simple object.

Pardon me but I really don't understand what you mean. Could you elaborate a little more?

He means the map scripts don't get unloaded after the map is loaded. The easiest thing would be to have a global object "triggerActions", and define all trigger functions under that global object, like

triggerActions.trigger2_Action = function(data)
{ /* body */ }

Then you can register the trigger via

cmpTrigger.RegisterOnUnitRangeFromEntityTrigger("entered", "trigger2", 273, 30, "trigger2_Action", [0, 1, 2]); 

And the trigger component can just call

triggerActions[actionName]

[historic_bruno]

Replying to Spahbod:

Replying to historic_bruno:

..., I would suggest doing that by name instead and having whatever data they need as a simple object.

Pardon me but I really don't understand what you mean. Could you elaborate a little more?

sanderd17 probably gave a more relevant example, but JS has some nice properties, like functions are objects, so instead of storing functions in the simulation state in a way that would require serializing, you could store the function name and look them up in a table:

var functions = {
  "foo": function(data) { ... do something with data, return an object ... },
  "bar": function(data) { ... do something different ... },
 ...
};

Then you don't need to serialize the actual foo and bar functions, only the strings "foo" and "bar" to look them up in the functions object together with some data to pass into them, e.g. functions["foo"]({x: 123, y:456}). I haven't looked at the patch to give an exact context, but I'm pretty sure whatever you want to do can be solved without serializing functions.

[O.Davoodi]

Added a new event: "OnUnitIssuedOrder" which is called by UnitAI orders.

[O.Davoodi]

Latest patch. Some minor fixes and a bunch of new helper functions.

[O.Davoodi]

Newest patch. Ready for review.

[O.Davoodi]

A simple tutorial map made by triggers.

[O.Davoodi]

It might be enough for now. The "Basic Tutorial" map covers camera movements, unit orders and resource gathering. We certainly need more advanced tutorials but that is independent of this ticket.

[sanderd17]

Tested the patch, and read the code quite well now. Here are some of my remarks on the basic_tutorial code, and the triggers component:

IMO, it would be better to have the scripts closer to the other map files (so you have a basic_tutorial.xml, .pmp and .js in the same directory). It makes map distribution easier.

In the tutorial you provided, you also don't count the number of units you moved, but the number of times you moved them. The action also succeeds when you only move one unit multiple times, but it doesn't succeed when you move a single formation one time (e.g. the group of archers).

I also tried the saved game. I saved the game after the movement tutorial, and when loading the saved game, it got stuck in the "resource gathering" mode. No matter how many times I tasked a unit to go and get resources berries, I wouldn't get a different task. I think this is because you used global variables that don't get serialised. All variables you want to use should be part of the g_TriggerActions (in which case we might want to rename it to g_TriggerData or simply g_Trigger).

Also, the tutorial gives an error about g_TriggerActionsaction_TutorialOnGatheringTreasure not being a function. That's because you forgot to register that function. As a result, I think it's best to just define the functions as

g_TriggerActions.action_TutorialOnGatheringTreasure = function(data)

That makes registering the function afterwards unneeded.

I also think that spawning a unit just like that in the tutorial isn't very representative for the game. So it would be best if the cavalry unit would already be there.

For the coding style (as the basic tutorial example needs to have nice coding style), I suggest to not make the block comments (with all those slashes) longer than 80 chars. We don't have a general 80 chars limit, but those comments get really ugly when they're wrapped.

I'd also don't use IID_DamageReceiver for the range trigger with that market you used. I think 0 would be good here. As people will copy this function, and not care about that variable for sure. So it's good if our example can show something that always works (but might be a bit slower). The documentation might explain it more in detail.

Now, for the triggers component itself:

You have to watch out with the DoAfterDelay function. The data argument you give actually becomes data.data in the ultimate function evaluation. That's rather unintuitive.

For the StartUpdateTimer, I'd make the "delay" argument a required argument. So remove that undefined check. And I'd also switch the order of the arguments to match the order in the Timer component.

Same for RegisterTrigger, I'd also make the "enabled" argument a required argument. And IMO, the following would be nicer for that function:

Trigger.prototype.RegisterTrigger = function(name, event, action, enabled)
{
	var eventString = "event" + event + "Actions";
	if (this[eventString])
	{
		this[eventString][name] = action;
		this.triggerEnabled[name] = enabled
	}
	else
		error("Invalid trigger event \"" + event + "\".");
};

You don't have to define all possible actions twice (in the init and in that function). Once is enough.

When using g_TriggerActions. It would be nice if you could check if those functions exist, and give a clear error message when they don't exist. I had to use more extra logging than wanted to find that the treasure action wasn't defined.

You can route the OnGlobalConstructionFinished, OnGlobalTrainingFinished and TimerUpdate back to CallEvent (with the right event name). This would save a bit of work on the checking of g_TriggerActions.

I'd also simplify the OnGlobalRangeUpdate function. As currently there are some bugs in it. Instead, I'd just keep track of the current collection (note that there's a bug in the removal of entities, they get added again). Then add that collection to the msg you get from the RangeManager, and just send it to the right action. With the added, removed, and current list, the actions can decide for themselves if they need to do something. Rather than working with those events.

Btw, the "moved" even won't work. The RangeUpdate message is only send when an entity gets inside the range, or leaves it. Not when an entity moves.

[O.Davoodi]

Thanks for the review. Will fix the issues as soon as possible.

[O.Davoodi]

I have to work on the patch again. But there are some points that should be discussed before uploading a new patch:

Replying to sanderd17:

IMO, it would be better to have the scripts closer to the other map files (so you have a basic_tutorial.xml, .pmp and .js in the same directory). It makes map distribution easier.

Can be easily done, but it prevents the usage of the same file from different categories of maps.

In the tutorial you provided, you also don't count the number of units you moved, but the number of times you moved them. The action also succeeds when you only move one unit multiple times, but it doesn't succeed when you move a single formation one time (e.g. the group of archers).

True. Going to fix it.

I also tried the saved game. I saved the game after the movement tutorial, and when loading the saved game, it got stuck in the "resource gathering" mode. No matter how many times I tasked a unit to go and get resources berries, I wouldn't get a different task. I think this is because you used global variables that don't get serialised. All variables you want to use should be part of the g_TriggerActions (in which case we might want to rename it to g_TriggerData or simply g_Trigger).

Fixed.

Also, the tutorial gives an error about g_TriggerActions.action_TutorialOnGatheringTreasure not being a function. That's because you forgot to register that function. As a result, I think it's best to just define the functions as g_TriggerActions.action_TutorialOnGatheringTreasure = function(data) That makes registering the function afterwards unneeded.

Good idea. Although I don't understand why it worked in the first run.

I also think that spawning a unit just like that in the tutorial isn't very representative for the game. So it would be best if the cavalry unit would already be there.

I have had two reasons for that. First, is that cavalry can't gather, and I didn't want the player confused about it in the gathering tutorial. Also I wanted the possible trigger maker to know how to use the spawning function.

For the coding style (as the basic tutorial example needs to have nice coding style), I suggest to not make the block comments (with all those slashes) longer than 80 chars. We don't have a general 80 chars limit, but those comments get really ugly when they're wrapped.

Done. Although some of the current lines exceed that limit too (because they contain a lot of text). Breaking those lines might confuse people more.

I'd also don't use IID_DamageReceiver for the range trigger with that market you used. I think 0 would be good here. As people will copy this function, and not care about that variable for sure. So it's good if our example can show something that always works (but might be a bit slower). The documentation might explain it more in detail.

Good idea. Done.

Now, for the triggers component itself: You have to watch out with the DoAfterDelay function. The data argument you give actually becomes data.data in the ultimate function evaluation. That's rather unintuitive.

Fixed.

For the StartUpdateTimer , I'd make the "delay" argument a required argument. So remove that undefined check. And I'd also switch the order of the arguments to match the order in the Timer component.

I don't get the reason for this one. Why should delay be required? It is not always going to be used. Changing the order of arguments is understandable if we decide to make delay required.

Same for RegisterTrigger , I'd also make the "enabled" argument a required argument.

Same as above:

And IMO, the following would be nicer for that function: Trigger.prototype.[wiki:RegisterTrigger] = function(name, event, action, enabled) { var eventString = "event" + event + "Actions"; if (this[eventString]) { this[eventString][name] = action; this.triggerEnabled[name] = enabled } else error("Invalid trigger event \"" + event + "\"."); };

Fixed.

When using g_TriggerActions. It would be nice if you could check if those functions exist, and give a clear error message when they don't exist. I had to use more extra logging than wanted to find that the treasure action wasn't defined.

You can route the OnGlobalConstructionFinished , OnGlobalTrainingFinished and TimerUpdate back to CallEvent (with the right event name). This would save a bit of work on the checking of g_TriggerActions.

Makes sense.

I'd also simplify the OnGlobalRangeUpdate function. As currently there are some bugs in it. Instead, I'd just keep track of the current collection (note that there's a bug in the removal of entities, they get added again). Then add that collection to the msg you get from the RangeManager , and just send it to the right action. With the added, removed, and current list, the actions can decide for themselves if they need to do something. Rather than working with those events.

Can you explain a bit about the bug when removing? The way you suggest might be too hard for a map maker, and certainly too hard for us if we want to implement an Atlas GUI for triggers.

Btw, the "moved" even won't work. The RangeUpdate message is only send when an entity gets inside the range, or leaves it. Not when an entity moves.

That is an artifact of the previous code. Might as well remove the "moved" event altogether.

[sanderd17]

Replying to Spahbod:

I have to work on the patch again. But there are some points that should be discussed before uploading a new patch:

Replying to sanderd17:

IMO, it would be better to have the scripts closer to the other map files (so you have a basic_tutorial.xml, .pmp and .js in the same directory). It makes map distribution easier.

Can be easily done, but it prevents the usage of the same file from different categories of maps.

I just wonder how reusable the scripts would be. I think the current scripts are quite tied to the maps itself, so distributing it together with the map seems logical.

I also think that spawning a unit just like that in the tutorial isn't very representative for the game. So it would be best if the cavalry unit would already be there.

I have had two reasons for that. First, is that cavalry can't gather, and I didn't want the player confused about it in the gathering tutorial. Also I wanted the possible trigger maker to know how to use the spawning function.

I see. It's not that important, so I'm ok with it either way.

For the coding style (as the basic tutorial example needs to have nice coding style), I suggest to not make the block comments (with all those slashes) longer than 80 chars. We don't have a general 80 chars limit, but those comments get really ugly when they're wrapped.

Done. Although some of the current lines exceed that limit too (because they contain a lot of text). Breaking those lines might confuse people more.

Yes, you don't need to cram the other lines into the 80 char limit.

For the StartUpdateTimer , I'd make the "delay" argument a required argument. So remove that undefined check. And I'd also switch the order of the arguments to match the order in the Timer component.

I don't get the reason for this one. Why should delay be required? It is not always going to be used. Changing the order of arguments is understandable if we decide to make delay required.

Just coding style. Most methods in our code don't have "defaulting" of parameters. And if there are defaults, it's usually an implicit default to undefined.

I'd also simplify the OnGlobalRangeUpdate function. As currently there are some bugs in it. Instead, I'd just keep track of the current collection (note that there's a bug in the removal of entities, they get added again). Then add that collection to the msg you get from the RangeManager , and just send it to the right action. With the added, removed, and current list, the actions can decide for themselves if they need to do something. Rather than working with those events.

Can you explain a bit about the bug when removing? The way you suggest might be too hard for a map maker, and certainly too hard for us if we want to implement an Atlas GUI for triggers.

After removing, you add it again. Quite a simple typo.

For making it hard for the map maker, I think the triggers component should just have to keep track of the state of the triggers, while the helper functions have to take care of "making it easy for the map maker". When you just give the added, removed and current entities in range list, the helper functions can do the rest. Splitting that up in the triggers creates more fluff than necessary where it's not needed IMO.

[sanderd17]

I worked a bit on the triggers myself. I tried to make it less fragile.

I removed the "keys" by which you could enable or disable triggers. Instead the action name directly functions as key (in combination with the event when needed). This means you don't have to invent yet another name, and also that you have to type a bit less (less chance for typos).

I also made the script for basic_tutorial a bit less fragile by putting all operations on one trigger together, and also stop working with timers, but only with delays (IMO, timers are better if you don't have to change the interval).

Also note, saved games are working with this coding style in the map script. Though having cmpTriggers as a global is still dangerous, we should find a safe solution for that.

[O.Davoodi]

I had been working on this. But University exams started sooner and I couldn't work on the patch as fast as before. Fortuantely they end in a week and I'd be able to do some more work on this.

You fixed most of the points above except the one that was making the most problem, the one about counting the number of units moving. It seems that to handle this, we must create another event called "OnUnitsIssuedOrder" (Note the plural sign in "Units") that is called directly from Command.js. Because the UnitAI itself doesn't have sufficient data on this.

Also note, saved games are working with this coding style in the map script. Though having cmpTriggers as a global is still dangerous, we should find a safe solution for that.

I have addressed that in my newer patch by making it a property of g_TriggersData.

I also made the script for basic_tutorial a bit less fragile by putting all operations on one trigger together, and also stop working with timers, but only with delays (IMO, timers are better if you don't have to change the interval).

That has the drawback of us having to disable the timers manually when the player has done the tutorial. Actually I'm quite sure your current code prints the messages of the previous tutorials infinitely. To have it work, we should stop that timer every time the tutorial is finished, which was automatically done in "StopUpdateTimer()" before.

I removed the "keys" by which you could enable or disable triggers. Instead the action name directly functions as key (in combination with the event when needed). This means you don't have to invent yet another name, and also that you have to type a bit less (less chance for typos).

I'm not sure about this one. Note that we might want to enable and disable a single trigger multiple times. To re-enable a trigger, we have to reassign the action, which isn't very understandable IMO, and could make problems for when we want to create the Atlas GUI (and its parser).

[sanderd17]

Replying to Spahbod:

I had been working on this. But University exams started sooner and I couldn't work on the patch as fast as before. Fortuantely they end in a week and I'd be able to do some more work on this.

Np, it's not that urgent.

You fixed most of the points above except the one that was making the most problem, the one about counting the number of units moving. It seems that to handle this, we must create another event called "OnUnitsIssuedOrder" (Note the plural sign in "Units") that is called directly from Command.js. Because the UnitAI itself doesn't have sufficient data on this.

That's only a detail IMO. The most important thing is that we get the maps readable. Adding extra events will have to happen in the future anyway. And maybe commands.txt is a better place for it than UnitAI.

Also note, saved games are working with this coding style in the map script. Though having cmpTriggers as a global is still dangerous, we should find a safe solution for that.

I have addressed that in my newer patch by making it a property of g_TriggersData.

There's no guarantee that Engine.QueryInterface(SYSTEM_ENTITY, IID_Trigger) will return the same object after loading a saved game. I don't know how it works for system components, but it certainly doesn't work for normal components. So in general, this is not safe. The best thing to do would be making helper functions for when cmpTrigger is needed, those helper functions can be global and can be re-routed to the cmpTrigger functions.

I also made the script for basic_tutorial a bit less fragile by putting all operations on one trigger together, and also stop working with timers, but only with delays (IMO, timers are better if you don't have to change the interval).

That has the drawback of us having to disable the timers manually when the player has done the tutorial. Actually I'm quite sure your current code prints the messages of the previous tutorials infinitely. To have it work, we should stop that timer every time the tutorial is finished, which was automatically done in "StopUpdateTimer()" before.

The old one had the problem that, if you completed the action before it was asked (f.e. you tasked some units to the bushes in the first few seconds), there were problems with the map asking you to do stuff you had already done. It could be solved with checks, but not with less checks than I have done without timer.

I removed the "keys" by which you could enable or disable triggers. Instead the action name directly functions as key (in combination with the event when needed). This means you don't have to invent yet another name, and also that you have to type a bit less (less chance for typos).

I'm not sure about this one. Note that we might want to enable and disable a single trigger multiple times. To re-enable a trigger, we have to reassign the action, which isn't very understandable IMO, and could make problems for when we want to create the Atlas GUI (and its parser).

The most important thing, if the key should be unique, we must make it unique by design. Because I ask the action+event as replacement for a key, makes it always unique. Weather we just delete it from a list, or set an "enabled" flag is just an implementation detail. By requiring the action+event, it was easier to delete it completely.

The other way we could achieve uniqueness by design, is by not giving the key as a parameter to the RegisterTrigger function, but as a return value from that function. Though it may complicate the map scripts a bit (you don't know the key from the start, so you have to always store it).

[O.Davoodi]

Newest patch. Some notes:

Added the "OnUnitsIssuedOrder" event for multiple entity orders.

I didn't change the "UpdateTimer" into "DoAfterDelay" in the tutorial. The issue you mentioned isn't that important (The map is played by beginner people after all and they are supposed to do the actions step by step)

As you can see, the new "key" consisting of event and action makes the code ugly. I'm in mind of reverting that back to the original "name" key. Have it in mind that in the Atlas GUI, the map maker will almost certainly want to "name" his triggers, and access it using that "name".

But we can retain the event+action key by removing the "action_" part from the "definition" of the trigger (for example in RegisterTrigger" function), and handle it internally (Note that the functions themselves will still have the "action_" prefix). This will effectively make the "action" part act as the "name" we wanted. If we are going to go for event+action, I'm more of a mind for this one.

There's no guarantee that Engine.QueryInterface?(SYSTEM_ENTITY, IID_Trigger) will return the same object after loading a saved game. I don't know how it works for system components, but it certainly doesn't work for normal components. So in general, this is not safe. The best thing to do would be making helper functions for when cmpTrigger is needed, those helper functions can be global and can be re-routed to the cmpTrigger functions.

I have not changed this. It needs more testing but I didn't encounter any problems with it. I think as global system components are defined in the C++ side, the code you posted will always return the same object unless the order of defining the system components is changed in Simulation2.cpp

[O.Davoodi]

The latest patch covers the decisions made in IRC, and it also fixes an unnoticed bug when saving the games.

In the previous patches, after loading a game, the "variables" were removed because of the line "var g_TriggerData = {};" in the beginning of the trigger script and were then assigned their "default" values. So, to make it sound, I created a "triggerVatiables" object in the trigger component itself, so that the previous values are not lost after a load. Also, I moved all of the initializations to an action called "InitTriggers" that is called in the beginning of the simulation itself. So that for every trigger script, this action should be defined.

[sanderd17]

Are you sure? I tested a lot with saved games, and it worked with my patch (when I loaded the saved game, it proceeded where I left of, so it must be that the true/false flags were kept as original).

I did define the triggerdata inside the helper script, and registered it to the engine there. So you don't have to do this per script.

Also, I don't think it reads the trigger script when loading a saved game, as then it should start the tutorial again with the welcome message (as that's just set with a timer by running the trigger script).

[O.Davoodi]

Replying to sanderd17:

Are you sure? I tested a lot with saved games, and it worked with my patch (when I loaded the saved game, it proceeded where I left of, so it must be that the true/false flags were kept as original).

I did define the triggerdata inside the helper script, and registered it to the engine there. So you don't have to do this per script.

Also, I don't think it reads the trigger script when loading a saved game, as then it should start the tutorial again with the welcome message (as that's just set with a timer by running the trigger script).

I'm certainly sure. To reproduce, play the map, do the moving tutorial, save and exit. Load again and move your units. The message about moving units appears.

About the helper script, I don't know how it worked for you, but it tells me it is "read-only" and cannot be changed in another script.

About the welcome message: That's why I didn't expect the save games to be buggy. But they turned out to be buggy for some reason. I've been trying to fix this for about two days and it worked only with the current design.

[sanderd17]

Checking the code a bit. And I see some problems with the new timer implementation. If you disable an active timer, all information about it is lost, so you can't enable it again (I assume that was the purpose of disabling and enabling).

I suggest to always use the same object/array schema for this.eventOnIntervalData[action]. Some schema that contains all info + the timer ID. You can set the timer id to 0 if disabled (the id is guaranteed to be non-zero). Then you can change those "typeof" selections with checks for the timer id being zero.

I also found a bug in the RangeManager. When you assign a range query to a non-existing id (or one that's killed), the rangeManager segfaults (it should never segfault from stuff you can do from a script). You can test it by deleting the market and running the tutorial.

It might also be nice if the SpawnUnits helper could return a list of ids of the spawned units. This will require a trivial change in the ProductionQueue code.

And also, the Triggers component should probably listen to "Unit Renamed" messages. Which are send when units are replaced by other units (f.e. when a unit gets promoted). It should update the range queries, and maybe also send the message on as an event. Note that replacing one unit by another one causes three messages: a created message, a destroyed message and a renamed message.

The basic tutorial in itself can also be a bit more robust. It's perfectly possible some user right clicks on a berry bush instead of an empty part in the terrain. Then the tutorial becomes quite a mess. Other possibility is moving to the metal instead of the stone.

[sanderd17]

New proposal. Extending the Triggers prototype to give the trigger makers less hassle calling components

[sanderd17]

Some improvements.

Now the Basic Tutorial exends the Triggers prototype. So immediately gets access to all methods defined in the triggers component, without going through the Engine. This simplifies the code a lot IMO.

But there are also other proposals.

I excluded the listener to the Global range messages, but made an TriggerEntity component that listens to those messages. This should probably lessen the mesages overhead a bit, as listening to something globally is really expensive. This does mean that only selected entities may respond to range queries. Currently all units and structures. But in a map-as-mod theory, it might be nice to restrict them to the trigger entities (as described below), and people can still modify templates in their mods.

The TriggerEntity component is also used to register trigger entities. I imagine them as a certain reference to a point. F.e. you have a 3D model of an A in atlas, you place that model somewhere, and you can ask the Triggers component to the position of A to spawn units, check range ... This already works, only the models and templates are missing.

Apart from those things, not a lot changed.

[sanderd17]

Comments from leper:

CallEvent function body isn't well structured

All functions in Triggers.js need to be end with a ;

[Yves]

Some first input after the general discussions about the scope of triggers and flexible missions... I'd like to make a test with a multiplayer mission now and maybe I have some more feedback from that. In addition there are some other open points to check on my list.

Title	Description	Part	Type
Generic missions, flexible missions, victory conditions	Not implemented yet, but an approach is described and should work as far as I can tell: http://www.wildfiregames.com/forum/index.php?showtopic=18764&p=293512 This can be fully implemented as a second commit after the main trigger system commit in my opinon.	Trigger System	TODO
Triggers for random maps	I have no doubt that there's at least one sensible way to use triggers from random map scripts with the current system. I think there are even many ways it could be done, so it would make sense to figure it out and add an example implementation.	Trigger System	TODO
TriggerHelper.SpawnUnits: Error handling	if !cmpPosition, it prints warnings but still creates all the entities. Shouldn't that be considered an error and shouldn't it abort the function?	Trigger System	TODO
Separate examples	Please remember to separate the examples from the main commit.	Trigger System	TODO
Trigger.prototype.RemoveRegisteredTriggerPoint: Error handling	Some error handling would be good instead of silently ignoring if there's no matching ref or ent	Trigger System	TODO
Documentation on the wiki	Good documentation is especially important in this case if we want many new mission scripts from map designers and modders! :)	Trigger System	TODO
Translation support for the trigger scripts in general	TriggerHelper.PushGUINotification does not support translated messages. If we extend it to support that too, it basically becomes a wrapper around PushNotification from the GUIInterface component with the only difference that the mission scripter doesn't have to query the interface for a GUIInterface component. Is it even worth a helper function in this case? Should we keep the current helper function for untranslated messages?	Trigger System	TODO
Translation support for basic tutorial	If not in the first patch, we should at least add translation support for the tutorial later.	Basic tutorial	TODO

Stuff solved:

Title	Description	Part	Type
Typo	Trigger.prototype.DisableTrigger: Warning message should say „Disabling“ instead of „Enabling“	Trigger System	DONE
Style	Extra line in Damage.js	Trigger System	DONE
Rider in basic tutorial	There's no rider in the basic tutorial (and horseman would probably be the better term... but I'm not sure)	Basic tutorial	INVALID
Typo	„the“ should be removed from „Spawn units from the all trigger“	Trigger System	DONE
trigger templates not properly included in patch	"The following files aren't properly included in the patch; binaries/data/mods/public/simulation/templates/special/trigger_point_A.xml, binaries/data/mods/public/simulation/templates/special/trigger_point_B.xml, binaries/data/mods/public/simulation/templates/template_trigger_point.xml Please add them for the review and make sure they have the correct properties set before committing"	Trigger System	DONE
Tutorial text	„Food, Wood, Stone and metal“: wood and stone should start with a lower case letter. „Resource gathering play a central role“ should be „plays“	Basic tutorial	DONE

[sanderd17]

Fix simple typos style stuff.

[Yves]

I have created a multiplayer example mission to test the trigger system in practice. It requires a "TreasureCollected" event which I have implemented. Please add this event to the next version of the trigger patch, it could be useful to have in the general interface. I'll post some more feedback soon, but not today.

In addition to the attached patch, you need to add the "TreasureCollected" event to Trigger.prototype.eventNames.

[Yves]

Small patch to add a TreasureCollected event (needs the event name definition in Trigger.js in addition to the patch)

[Yves]

Treasure Quest Multiplayer Mission (a demo multiplayer mission used for testing)

[O.Davoodi]

@Yves: We can already use triggers in random maps. We should put the relevant property in the map's json file.

[Yves]

Replying to Spahbod:

@Yves: We can already use triggers in random maps. We should put the relevant property in the map's json file.

Yes, Sander has said that on IRC too. I didn't know that the code for loading the maps settings is the same for all types of maps.

Anyway, would you mind creating a demo mission with random maps? I'm not used to the random map code but I think it would be nice to have a demo for all major use cases. Trying to do something in practice is also a good way to find problems in the system. Preferably the random map would place some trigger points and use them in scripts. You could also reuse some of the existing scripts/ideas in random maps (like collecting treasures or spawning invading armies).

[Yves]

Some more input. I'm still checking some more things on my list. We'll have to define what needs to be in the first commit and what not after I'm through with the review.

Error handling: SpawnUnitsFromTriggerPoints	It should print an error if no trigger point for the specified ref can be found.	Trigger System	TODO
Notifications to multiple players and all players	As you see in my „Treasure Quest“ example, I had to duplicate all the PushGUINotification calls that go to more than one player. We need a way to specify to which players a notification goes, including a sensible way to address all players (I've checked it now – arrays of playerIds can't be used for that currently). The mission script should not have to query the number of players and build an array with player numbers for addressing all palyers. It should just flag „all players“ somehow or call a helper function that returns an array of all players (probably a bit less clean though). I'm not quite sure yet if this functionality should be added to the GUIInterface (messages.js et al.) or just built ontop of that in the TriggerHelper. This probably also depends on how Timed notifications are implemented.	Trigger System	TODO
Timed notifications	Sander brought that up and I think it's a useful and important extension of the notification helper function(s) an add it to this list. Sometimes a notification should stay active for a longer period of time or until it's removed again by an event. There's a similar functionality in GUIInterface.js (the TimeNotifications functions).	Trigger System	TODO
Basic tutorial is skirmish currently	„Basic Tutorial.xml“ specifies to basic_trigger.js script to be in the skirmishes folder. IMO this is clearly a scenario.	Basic tutorial	TODO
Filtering on gamesetup screen	Players may want to identify: maps with special missions (1), historical accurate missions (2) and maps with completely changed gameplay (3). The difference between scenario and skirmish maps is currently only if the setup is fixed or not (players, victory conditions starting resources etc.). For both triggers can make sense and for both it would be interesting for the player to be able to indentify maps with triggers and special behaviour easily (scenarios will probably use triggers in most cases in the future though). Suggestion: Add a "mission" keyword to skirmish maps, scenario maps and random maps (1). Add the keywords "Historical" for scenario maps (2). Use the map description to show how much gameplay changes due to the triggers (3) because the definiton is too blurry for a filter/keyword. I'm not sure if having different keywords for scenarios and skirmish maps is currently supported. "Historical" only really makes sense with a fixed map setup IMO, so it doesn't fit well for skirmish maps or random maps.	General	TODO
Multiplayer/Singleplayer compatibility	Some missions are meant for multiplayer and others not. This starts getting a real issue with triggers. Some mission scripts might be completely untested and broken for multiplayer games, others just aren't meant for that conceptually. The same applies the other way around (missions meant for multiplayer not working well in singleplayer), especially because the AI might not understand special victory conditions and behave completely stupid from that point of view. Other missions might work well for both multiplayer and singleplayer missions. Suggestion: IMO this should not be solved with map keywords (at least not how they work currently). Incompatible maps should not be shown at all on the gamesetup screen. I think we need to add another property to the map JSON data that defines if the map is compatible with singleplayer and multiplayer. This needs to be checked on the gamesetup screen and only supported maps should be displayed.	General	TODO
clone the data to be sure it's only modified locally (comment in Trigger.js)	This way of cloning only works for non-object properties like strings or numbers (check the example below).	Trigger System	TODO
RegisterTriggerPoint, RemoveTriggerPoint error handling	It would be nice to ensure some assumptions like that a trigger point doesn't get added twice or removed when it's not in the triggerPoints array. You check if it's not in the array and just return in this case. However, as far as I understand this should never happen in practice and if it still happens, there's an issue somewhere. IMO it's generally better to reveal such issues by printing an error. It might be worth considering the use of a set instead of an array (because sets are meant to contain UNIQUE elements) but on the other hand they are not stable in the spec and we didn't want to use them too often.	Trigger System	TODO
Simulation2.cpp: LOGMESSAGE	Use %hs instead of %s for char*	Trigger System	TODO

Example - Incorrect cloning of object properties:

var data = { "sub1": { "sub1prop1":"original" }, "prop1": "original" };
var newData = {};
for (var key in data)
	newData[key] = data[key];

newData["sub1"]["sub1prop1"] = "modified"; // also modifies original because sub1prop1 was added by reference
newData["prop1"] = "modified"; // only modifies the new object because string properties get copied
print("data" + uneval(data));
print("newData" + uneval(newData));

Output:

data({sub1:{sub1prop1:"modified"}, prop1:"original"})
newData({sub1:{sub1prop1:"modified"}, prop1:"modified"})

[sanderd17]

In 15394:

Clean up notifications. Merge the timed and untimed text notifications, allow passing a list of players to all notifications, make modification types moddable. Refs #52

[O.Davoodi]

Replying to Yves:

Replying to Spahbod:

@Yves: We can already use triggers in random maps. We should put the relevant property in the map's json file.

Yes, Sander has said that on IRC too. I didn't know that the code for loading the maps settings is the same for all types of maps.

Anyway, would you mind creating a demo mission with random maps? I'm not used to the random map code but I think it would be nice to have a demo for all major use cases. Trying to do something in practice is also a good way to find problems in the system. Preferably the random map would place some trigger points and use them in scripts. You could also reuse some of the existing scripts/ideas in random maps (like collecting treasures or spawning invading armies).

OK. I'm on it.

[Yves]

I'm through with the review. I have not checked every line in the patch very carefully, but I'm confident that I found most of the issues.

I have reported all the issues I found including minor ones like typos. So let's sum it up on a higher level.

The main thing we wanted to avoid is including many new maps that all have to be changed or completely redone later because the trigger system changes too much. I think we can be confident enough that this will not happen with the trigger system as a whole because this solution is very flexible and we have prove of concept implementations or at least rough design ideas for all known uses cases. Some additional thoughts on that topic:

• An easy to use interface for Atlas should be doable with this approach. It's missing at the moment, but the scripting interface is developing well and is quite easy to use. That should be enough for now.
• The script interface still lacks a lot of functionality. We should provide implementations for the common tasks to avoid that each map/mission designer has to reinvent the wheel again and because it's easier if they have everything they need in the "Trigger API" (predefined events in the trigger component, functions in TriggerHelper.js etc.) instead of diving into the simulation code too much. It should be possible to provide most maps and missions without changes to simulation components (except the Trigger extensions). In my opinion we should not and cannot provide all that functionality in the first version of the trigger system. Instead, we should cooperate closely with users of the trigger system to add that functionality in the coming weeks and months.
• IMO flexible mission support (or custom victory conditions) should be added as soon as possible (not necessarily in the first commit though). Otherwise it could happen that many generic mission are implemented for specific maps. It's also one part of the trigger system we haven't completely figured out yet (although there are some good ideas how it should work).

Some other general points summed up:

• A good documentation on the wiki and some examples are important. We already have some examples and should get another one for random maps from Spahbod, but the wiki article is still missing AFAIK.
• Compatibility checks for singleplayer/multiplayer and proper filtering on the gamesetup screen will become more important with an increasing number of maps using triggers. This should also be implemented as soon as possible. It would be nice to have in the first commit IMO.
• Generally the "Trigger API" needs to handle errors especially well because (hopefully) a lot of less experienced developers will work with it and the system should help them finding potential problems as good as possible. I've mentioned some places where error handling could be improved, but please check the functions once again by yourself because I have probably missed something too.
• It would be nice to keep the currently included "Trigger API" functionality as stable as possible. We should try to improve parts where we already know that they are missing something (the translation topic for example). On the other hand we're not tied to a "stable API" because it shouldn't be too much work to adjust some maps (at least in the beginning when there aren't too many of them). I'm just saying we should care about this but we don't have to be extremely careful to avoid changes.

Btw. I've added two additional lines to the last table.

[sanderd17]

r15421 adds the components and the Engine support.

Demo triggers and examples will follow.

[O.Davoodi]

In 15468:

Added "Survival of the Fittest", a demo random map with triggers. Refs #52

[O.Davoodi]

The map is nearly complete except that it doesn't yet prevent players from building farm/civic-center as that requires some tweaks in the simulation code itself and is WIP.

[O.Davoodi]

This patch completes the "Survival of the fittest" map and adds the ability to disable certain templates for players.

A minor issue that the patch currently has is that if the player has selected a trainer/builder and something happens to the disabled templates, the GUI will not be updated until the entity is selected again.

[leper]

There's no need for the message to be defined on the c++ side.

[O.Davoodi]

New patch that also informs the GUI that the change has happened. Fixes issues pointed by leper and sanderd17

[O.Davoodi]

Adding translation marks for basic tutorial.

[O.Davoodi]

In 15614:

Added the ability to disable training/building of entities by triggers. Changes survival of the fittest random map to work with this. Refs #52

[wraitii]

In 25373:

Simplify trigger event naming scheme.

Reduces confusion and string manipulation.

Refs #52.

Differential Revision: ​https://code.wildfiregames.com/D3913