[PATCH] Add an alert button (kind of Town Bell)

[Itms]

This feature works like this :

• A new component, named AlertRaiser, allows for example CCs to sound the alarm
• The alert is effective just in a range which can be adjusted in AlertRaiser (I chose initial territory extension for CCs, which are for the moment the only template implementing this component)
• The alarm can be triggered in 'yellow alert' (ie. normal mode) and all female citizens look for a place to garrison. Then, one can trigger the 'end of alert', or pass into 'red alert', in that case all units try to garrison
• The feature uses the back to work behaviour when the alert ends.
• The End of Alert is effective across the entire map, but ungarrisons only the units garrisoned by the ungarrisonning AlertRaiser

I need artwork for the feature, please visit these posts : http://www.wildfiregames.com/forum/index.php?showtopic=17703 http://www.wildfiregames.com/forum/index.php?showtopic=18000

[Marcio]

this ticket is already exist and if you want see the list of button and commands go to. http://www.wildfiregames.com/forum/index.php?showtopic=16932&hl=buttons&fromsearch=1

[Adrián Chaves]

See also #35.

[Itms]

This is the patch I wrote. It works after revision [13987] which implements the back to work behaviour.

Please tell me what you think about it !

[Tom Brewe]

I compiled from the latest svn but couldn't find the city bell buttons on a civic center. Where are they supposed to be?

[Itms]

I am not allowed to commit anything : I attach my testing-buttons to this ticket, they must be placed in binaries/data/mods/public/art/textures/ui/session/icons/

However, these buttons shall be replaced soon by buttons designed by the artwork department. See this thread on the forums : http://www.wildfiregames.com/forum/index.php?showtopic=17703

Then you patch your copy of the svn with the patch I attached and it should work. You don't have to recompile anything since my changes apply to the javascript side.

[sanderd17]

EDIT: closed by mistake

[Itms]

I think this is a mistake in the number of the ticket ref'ed.

[sanderd17]

Sorry, the ticket that should have been closed is #2048.

[Marcio]

Why many levels? And if building have a flag points ignoring that, right?

[sanderd17]

Some comments on the code:

• I don't like most of the GarrisonHolder changes. The TownBellStriker should remember the units it garrisoned, not garrisonHolder. The only thing you might need to change there is a method to ungarrison units without following the ralleypoint, or maybe you can immediately replace the unitAI command.

You can achieve this by listening to the GlobalGarrisonedUnitsChanged message, and checking the entity ids.

• IMO, if you make a difference between alerts, a red alert should garrison all units (including heroes), this is a must for some maps that start with a hero unit.
• You don't need empty DeInit and OnUpdate functions
• why is there a difference in UnitAI on the Garrison command in both cases? (wrt to the data encoding)
• Your code should watch out with moving garrisonholders (like ships, rams ...), it would be best to exclude those.

[Itms]

Hello, I corrected what you pointed out in the code. It allowed me to improve the behaviour of units still not garrisoned when the alert ends. I also added the sounds provided by LAVS, which I attach to the ticket, they must be placed into binaries/data/mods/public/audio/interface/alarm/

Let me know what you think of the code.

I open a thread on the forums to discuss the number of levels needed for the bell : http://www.wildfiregames.com/forum/index.php?showtopic=17807

[Itms]

I'm back ! With a modified version of the patch, without all the hardcoded stuff.

Please let me know what you think.

I also post my decision about the number of levels (I actually didn't change anything) on the forums.

Please place the renamed sound files in binaries/data/mods/public/audio/interface/alarm/ if you want to test it.

[sanderd17]

I didn't test the patch, some small comments and questions on your code though:

• In Input.js, you can just return "state && state.alertRaiser && state.alertRaiser.hasRaisedAlert" instead of putting it in an if clause.
• Line 28 of AlertRaiser.js doesn't need brackets
• Line 33 doesn't need the String() function, the conversion happens automatically when adding with a string
• Line 46, a push operation isn't needed, you can just set the array as it will only contain one element
• Line 54, better use the getter function than directly request the value.
• Line 57, is the animal check needed? What about war dogs, or sheep? What's your reasoning behind it?
• Line 88, the check for UnitAI. Either you can remove the check, or you can show an error message. It should never happen this doesn't have a UnitAI, as it's requested by the RangeManager query. You also don't need to redo this check a few lines later.
• UnitAI line 2405, it's strange that this.garrisoningTarget means it comes from an alert. Isn't there an other way to determine it? This doesn't look too logical.
• Line 2409, you can use the IsFull() method instead
• Line 2781, doesn't need brackets, but needs a ';', next function also doesn't need brackets
• Line 3653, doesn't need a push() as the array will be at most one element long
• Line 3662 can also use the IsFull() fucntion
• Why are those checks in Commands.js? Either, the failure is an error (and the error should be reported), or you need to tell the player why it failed, or you need to just ignore it.

But I must say, the overall structure looks good to me.

[Itms]

Like this ? =)

For animals line 57, I placed it because I thought domestic animals would take useless place in the garrison holders. But indeed, I forgot the war dogs. This way it is fixed.

I also corrected the push < set improvement on several UnitAI functions that actually made me write my patch like this.

[sanderd17]

I still like to see an other name for this.garrisoningTarget. It's perfectly possible someone reads the variable and expects it's set on every garrison command.

Apart from that, I think it's good. I'll test the patch behaviour again in some time.

[Itms]

Okay for the code readability. Is it better like this ?

[sanderd17]

Yes, looks better to me.

Thanks for working on this.

[leper]

Please use for each (var a in b), else you add global variables.

[Itms]

Oh, sorry, thanks for pointing that out !

[Yves]

There's a problem with the worker elephant (on Acropolis 1 for example). It never gets in garrison range and changes between "GARRISONED" and "APPROACHING" state. The town bell behaviour seems to be messed up too, probably because of this worker elephant.

[sanderd17]

Should be fixed when #2190 is in.

Though it's still worth checking the problem, some strange things might happen (like it might try to garrison a fishing boat, but find no place to garrison it).

[Marcio]

The fishing Boat don't have a garrison?

It's necessary see what's units can be garrison and what units can't , see the Siege Weapons for example, cavalry don't have in all buildings etc. Is necessary fix that at first. All units in the game. Even in the Atlas.

How I can test this in SVN ? I updated manually paste the code or diff one for one?

[sanderd17]

@itms, I like the patch behaviour, when there are some icons, it can be included immediately IMO.

@Lion: to apply the patch, you should download the patch file, then you should apply the patch to your SVN, if you use tortoise SVN, use this guide: ​http://tortoisesvn.net/docs/release/TortoiseSVN_en/tsvn-dug-patch.html (the second part of the page is about applying patches).

Music is never included in patches, so you need to copy the music files in this ticket to the right directories (binaries/data/mods/public/audio/interface/alarm/alarmalertX.ogg). Else it will crash because the requested music isn't found.

[Yves]

I've tested it again, the issue actually wasn't related to the worker elephant.

1. Click the "alert" button and wait until all female citizens are garrisoned
2. Click "unload all" (not the bell)
3. Click the "un-alert" button
4. Click the "alert" button again. Nobody will garrison.

[sanderd17]

Yves, you're right. Removing the check of IsUnderAlert in AlertRaiser.js line 57 fixes the problem, and I think it has no bad side effects (when units listen to two alerts, they just listen to the second one given instead of the first, but that's not a problem IMO).

[Itms]

Yeah, we can remove the check, it is something I kept from the previous patch design (with yellow and red alert), it was necessary then but not any more.

I also update the comment for this block of code !

[Marcio]

@Sanderd17 Thanks You, I'll try when come to my house in my country. So we have two alarm sounds? You Thinks is finish/ ready this for Alpha 15?

[sanderd17]

Line 683 of UnitAI can set the target to undefined, and this can cause some warnings. The if-else structure should be different, as the setting of the target may not be executed after this.FinishOrder is called.

I think the following works better:

this.alertRaiser = this.order.data.raiser;
		
// Find a target to garrison into, if we don't already have one
if(!this.alertGarrisoningTarget)
    this.alertGarrisoningTarget = this.FindNearbyGarrisonHolder();
	
if (this.alertGarrisoningTarget)
    this.ReplaceOrder("Garrison", {"target": this.alertGarrisoningTarget});

Maybe with an "else this.FinishOrder()" at the end, I don't know if it's needed.

[Marcio]

So its needed other patch for this can work properly?

[sanderd17]

We just need icons (doesn't have to be final, if Itms prefers the placeholders for now, that's fine too), and a decision on the sounds.

And the problem I mentioned above.

[Itms]

Hello everyone, and Merry Christmas ! Now that Osiris is released I take some time to work on the patch and, I hope, to make it committable. After my exams in January I'll be able to work on other patches !

This new version of the patch to correct the problem Sanders pointed out. I think this.FinishOrder isn't absolutely necessary (since without it we just end the order), but the code seems clearer to me this way. I also regenerated the patch with current revisions to avoid useless conflicts in the testers' branches.

I open a new thread in the forums to make the need for icons more noticeable... http://www.wildfiregames.com/forum/index.php?showtopic=18000

[sanderd17]

We'd still need new icons, but if you would like to have it in with placeholders, that's ok for me too. We've got plenty of time to commit replacements.

Can you give a comment about what sounds or icons you want for now? Then the code can already be comitted.

[Itms]

This is what I use for my tests. ​https://www.dropbox.com/sh/kdkns63z1uce0hz/eawlmwt83g

For the sounds, I have to answer LAVS on the forums.

Thanks for accepting to commit it anyway :-)

[sanderd17]

In 14392:

Implement town bell

• the icons are temporary, and can be replaced later
• The sounds are created by LAVS, but may also need some polishing, so the current ones are placeholders too
• The patch code is created by Itms

fixes #2154

[mimo]

In GuiInterfaceCall, it's may-be better to move the changes to GetEntityState instead of GetExtendedEntityState (it's computed only for cc and its computation is fast, so won't slow down the gui even when lots of units selected). Otherwise, you should change a bit the gui code to also call GetExtendedEntityState when needed.

[sanderd17]

In 14405:

fix tests (fixes #2326)
+ small code cleanup
+ move guiInterface call to GetEntityState
refs #2154

[mimo]

Sorry, but I forgot also a small possible change. What would you think of changing the alert tooltip in the button command and make it level depenpent ?

Present tooltip is "raise the alert ! or raise it again to protect more units"

We could change it to something like: when at level 0: "raise an alert" and when at level 1: "increase the level of the alert to protect more units"

[Michael]

Replying to mimo:

We could change it to something like: when at level 0: "raise an alert" and when at level 1: "increase the level of the alert to protect more units"

I think this would make the function more clear.

[Michael]

I found a little bug and wrote a small patch. see #2340