Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#1719 closed enhancement (fixed)

[PATCH] Attack notification

Reported by: Erik Johansson Owned by: zoot
Priority: Must Have Milestone: Alpha 15
Component: UI & Simulation Keywords: gui minimap patch
Cc: Patch:

Description

Display a notification on the minimap as well as play a sound when one of the players units is attacked.

Related to #57 , #1607 and #1425

Attachments (3)

AttackNotificationApr062013.patch (18.2 KB ) - added by madmax 11 years ago.
The patch file for adding attack notification to 0AD
AttackNotificationApr072013.patch (15.7 KB ) - added by madmax 11 years ago.
New Patch file
AttackNotificationApr112013.diff (22.2 KB ) - added by madmax 11 years ago.
New patch with better attack code from zoot

Download all attachments as: .zip

Change History (36)

comment:1 by Erik Johansson, 11 years ago

Keywords: simple gui minimap added

comment:2 by Jonathan Waller, 11 years ago

Once a notification is sent this should prevent other attacks which happen nearby from causing a notification for a certain length of time. It would be nice if the further attacks would also cause some suppression so an ongoing battle doesn't keep pinging after every timeout.

by madmax, 11 years ago

The patch file for adding attack notification to 0AD

comment:4 by madmax, 11 years ago

Ok, so thanks to zoot and me :), we seem to have a preliminary implementation of the attack notification on the minimap working. Hopefully I made the patch correctly. Please let us know if you find any errors after applying the patch.

comment:5 by leper, 11 years ago

Keywords: patch review added
Milestone: BacklogAlpha 14
Summary: Attack notification[PATCH] Attack notification

Please read SubmittingPatches. (mostly the part about adding the review keyword so that the ticket shows up in our review query)

Some small issues (not a real review, but those should be addressed):

  • Some strange includes: Minimap.cpp (the one with the TODO next to it), Minimap.h (use lib/types.h)
  • Why the addition of whitespace near EOL in Minimap.cpp (line 664)
  • Try to keep the syntax of the switch, that makes the changes a lot easier to read and switch statements are written that way according to our Coding Conventions.

by madmax, 11 years ago

New Patch file

comment:6 by madmax, 11 years ago

Sorry about that. In the new patch I have hopefully fixed the 3 issues. Not sure how to add the review keyword though. It seems to have been already added for this one.

Last edited 11 years ago by madmax (previous) (diff)

by madmax, 11 years ago

New patch with better attack code from zoot

comment:7 by historic_bruno, 11 years ago

Keywords: review removed

After reviewing the latest patch, I have some general comments:

  • Blinking red is confusing with red being a typical player color, white might work better (and some other rendering enhancements in the future)
  • The overlap with many entities is annoying but mostly a symptom of a larger problem: the minimap needs serious aesthetic work.
  • I don't hear the attacked sound for entities off screen, does the sound group need a distanceless flag set or something?
  • It seems like there could be almost constant attacked sounds if there were numerous attacks on different parts of the map, which would be annoying and not really more informative (would it?). I think it's a problem with sound only, the minimap pinging works well, so the sounds may need a separate cool off or timeout. This isn't the only situation like that, battle sounds also come to mind, maybe stwf can implement that in the sound manager.

About the code:

  • The interface of ICmpMinimap::GetRemainingPingCount()/SetRemainingPingCount() is awkward and only used in one place, could that be replaced by a single Ping() function that checks and decrements the counter?
  • More importantly, pinging is tied into the framerate, so at high framerates it will blink too fast, on low framerates it will appear to not blink at all, and also it will blink for a much shorter time at high framerates and much longer at low framerates. I think it should use time instead and measure time between minimap draw calls.
  • The chat message on attack isn't working, you need to add a call to addChatMessage() in handleNotifications(). To be more consistent with other messages, it should say "You are under attack" and it would be nice if it named the attacking player for clarity.
  • I think an attack notification should be sent even if a unit is invulnerable (it's important for the player to know, as it may be a temporary effect) and indeed whether or not damage is done, but we already have the MT_Attacked message, why not avoid passing the extra parameter to TakeDamage( or handling it in the damage receiver? The AttackDetection component already handles MT_Attacked messages - seems like a good fit and minimizes the interface other components need to use.
  • In AttackDetection could you use squared-distance instead of Math.sqrt? Should be faster and you could also use e.g. x*x instead of Math.pow for squaring.
  • handleAttack() is only called one place, it could be moved into or replace AttackAlert() to avoid another function call
  • When the player attacks their own entities (slaughtering animals), it will register as a detected attack.
  • In HandleTimeout(), you should cache the value of cmpTimer.GetTime() before the loop to avoid calling it too often. It's too bad CmpTimer doesn't already pass in useful info like time and ID to the timeout handler.
  • EntityAttacked message is never used so that can be cleaned up.
  • Can't the "attacked" sound group be added to the parent template_unit.xml instead of all the inheriting templates?
  • CCmpMinimap::m_PingEntity is a confusing name because it's type bool not entity_id_t that might be expected. Could it be named m_IsPinging instead? Similarly, IsEntityPinging() could be simplified to IsPinging(), since it's a component it will of course be an entity :)

Minor coding conventions:

  • Braces should go on line by themselves, see lines 476,483 in MiniMap.cpp
  • Braces aren't needed if they enclose only one line of code, as on line 84 of messages.js
  • Functions should begin with an uppercase letter, some of them in AttackDetection.js do not. The distance() helper function could be a global in that file since it doesn't access the instance's state.
  • For consistency, the function blocks for distance(),addSuppression() should end with a semicolon.
  • I like to see units documented on e.g. time and range, in the comments.

in reply to:  7 ; comment:8 by zoot, 11 years ago

A few comments for my part:

Replying to historic_bruno:

  • It seems like there could be almost constant attacked sounds if there were numerous attacks on different parts of the map, which would be annoying and not really more informative (would it?).

I'm not sure if it would be annoying. But I do suspect it would be annoying to receive a notification for some minor attack, and then missing the notification for a subsequent major attack due to the cooldown. (I haven't tested it, though.)

  • The chat message on attack isn't working, you need to add a call to addChatMessage() in handleNotifications().

True, I actually left it non-fuctional semi-intentionally. I personally would not want the text message to clutter my screen, but I figured it would be handy in some situations, like if you have no sound (or if you are deaf). So I would suggest making it toggleable with some configuation value, only I am not sure how to go about obtaining such a value from JS. Any suggestions/pointers?

  • I think an attack notification should be sent even if a unit is invulnerable (it's important for the player to know, as it may be a temporary effect) and indeed whether or not damage is done, but we already have the MT_Attacked message, why not avoid passing the extra parameter to TakeDamage( or handling it in the damage receiver? The AttackDetection component already handles MT_Attacked messages - seems like a good fit and minimizes the interface other components need to use.

This is what the implementation did in the first patches madmax submitted. The problem was that if a unit triggered an attack notification, and then moved out of the 'suppression radius', and then was attacked again, that same unit would trigger a repeat attack notification. This was annoying for units that flee when hit - you get notifications again and again for the same little skirmish.

So I decided to add a cooldown. Adding it per-unit to TakeDamage() was the cleanest way I could think of to do this. The alternative would be to maintain some huge array of cooldown timers (one per entity) in the AttackDetection component. Would that be the preferred solution or is there some other option I am missing?

I agree the attack notification should trigger for invulnerable units too. This could be fixed by simply moving the invulnerablity check below the cooldown check.

  • In AttackDetection could you use squared-distance instead of Math.sqrt? Should be faster and you could also use e.g. x*x instead of Math.pow for squaring.

My math-foo is a bit weak there - what exactly is squared-distance? Just the Pythagorean distance without sqrting it? In that case, I would have to convert all figures to squared-distance "internally" before comparing them? (Not that that would be a problem, just need to be sure that is what is being suggested.)

I'll try and fix the other items that I am familiar with and push the patch to my repo.

Last edited 11 years ago by zoot (previous) (diff)

in reply to:  8 ; comment:9 by historic_bruno, 11 years ago

Something else I thought about, CCmpMinimap should serialize its pinging state. That way saving and reloading won't lose active notifications. I forgot to test this when I had the patch applied, but looking at the code it seems that is missing. Can you confirm? Try quicksave/quickload (Shift+F5, Shift+F8)

Replying to zoot:

I'm not sure if it would be annoying. But I do suspect it would be annoying to receive a notification for some minor attack, and then missing the notification for a subsequent major attack due to the cooldown. (I haven't tested it, though.)

Thinking about it again this morning, it's not such a big deal and could be more useful than not in some situations.

True, I actually left it non-fuctional semi-intentionally. I personally would not want the text message to clutter my screen, but I figured it would be handy in some situations, like if you have no sound (or if you are deaf). So I would suggest making it toggleable with some configuation value, only I am not sure how to go about obtaining such a value from JS. Any suggestions/pointers?

I don't know if that's currently possible, I mean we could find a way to do it, but it would be a hack without a proper config system. If we do leave it non-functional, there should be a TODO comment added with an explanation.

Speaking of configuration, it occurred to me that adjusting how long the attack notifications ping on the minimap would be a useful option, and that is certainly possible (in C++). I believe the function to do that is CFG_GET_VAL.

So I decided to add a cooldown. Adding it per-unit to TakeDamage() was the cleanest way I could think of to do this. The alternative would be to maintain some huge array of cooldown timers (one per entity) in the AttackDetection component. Would that be the preferred solution or is there some other option I am missing?

It seems AttackDetection already has a list of every entity currently being suppressed (the event objects) that gets scanned on every new attack and its elements exist for the same suppression time, wouldn't it be possible to reuse that info? In fact one of the functions that searches the list, handleAttack(), is being called indirectly by TakeDamage().

In other words, if the attacked entity is in the suppressed list, ignore it, otherwise add a new entry and follow the rest of the logic currently in handleAttack(). You might be able to avoid iterating the the whole list too often by using an associative array (i.e. object) keyed by the attacked entity ID, you could also pass the ID to the timer handler, instead of iterating the list there. But it would be less efficient in having to handle every MT_Attacked message and filter them out by owning player, so it's a tradeoff and maybe not worth it :/

My math-foo is a bit weak there - what exactly is squared-distance? Just the Pythagorean distance without sqrting it? In that case, I would have to convert all figures to squared-distance "internally" before comparing them? (Not that that would be a problem, just need to be sure that is what is being suggested.)

Right, that's done in some other places as well, the AIs use it. Since you set the distance "constants", you can use whatever units you want :) (goes in hand with the comment about documenting units though)

in reply to:  9 comment:10 by zoot, 11 years ago

Replying to historic_bruno:

It seems AttackDetection already has a list of every entity currently being suppressed (the event objects) that gets scanned on every new attack and its elements exist for the same suppression time, wouldn't it be possible to reuse that info? In fact one of the functions that searches the list, handleAttack(), is being called indirectly by TakeDamage().

Actually, no, the event list is a quite short list of recent attacks. (Notifications for new attacks are suppressed if near a recent attack.)

in reply to:  7 ; comment:11 by zoot, 11 years ago

Replying to historic_bruno:

  • I like to see units documented on e.g. time and range, in the comments.

About this: I actually don't know what the units of range are. Any ideas? :)

in reply to:  11 comment:12 by historic_bruno, 11 years ago

Replying to zoot:

Replying to historic_bruno:

  • I like to see units documented on e.g. time and range, in the comments.

About this: I actually don't know what the units of range are. Any ideas? :)

If it's world units (from e.g. CmpPosition) we usually call that meters. There are 4 meters to a tile, at least horizontally, vertically is some other scale (defined here). The important distinction is meters vs. tiles, I think we do use tiles in some code, but I'd expect range is almost always meters.

Last edited 11 years ago by historic_bruno (previous) (diff)

comment:14 by historic_bruno, 11 years ago

Owner: set to zoot

comment:15 by historic_bruno, 11 years ago

Keywords: review added

comment:16 by !!Chaos!!, 11 years ago

Might be too late for this suggestion, but I'd suggest having the system check if the location of the attack is within the players current view, and if so forgo the announcement. This should stop the system from alerting you to attacks within ongoing battles most of the time. This should be run on top of current setups to avoid alert spam rather than instead of since the battle may not fully fit within the view.

in reply to:  16 comment:17 by zoot, 11 years ago

Replying to chaosislife:

We've discussed it, but it will probably not be done for the first iteration of this feature. Maybe it will be added later, if people see a need for it.

comment:18 by historic_bruno, 11 years ago

zoot, do you know if madmax is still working on this or planning to?

comment:19 by zoot, 11 years ago

No, haven't heard anything from him apart from what is here and on the forum.

comment:20 by madmax, 11 years ago

Ugh!! I totally forgot about this. Sorry! I ll check out the comments and post an update today.

in reply to:  20 comment:21 by zoot, 11 years ago

Replying to madmax:

Great!

in reply to:  20 comment:22 by historic_bruno, 11 years ago

Replying to madmax:

Ugh!! I totally forgot about this. Sorry! I ll check out the comments and post an update today.

Any progress on this?

comment:23 by madmax, 11 years ago

Just resumed work on it and straight away I have messed something up : http://www.wildfiregames.com/forum/index.php?showtopic=17099&st=180

comment:24 by madmax, 11 years ago

ok, so this CFG_GET_VAL. I see its used in CSimulation2Impl().

Do I use CFG_GET_VAL directly in CCmpMinimap::Init() to read the ping duration ?

comment:25 by madmax, 11 years ago

Also is there any other place in C++ where time has been used to control the UI. Maybe I can add code to control the blinking similarly. Otherwise I ll simply use the std c++ functions.

Maybe lib/timer.h

Last edited 11 years ago by madmax (previous) (diff)

in reply to:  8 comment:26 by historic_bruno, 11 years ago

Replying to zoot:

True, I actually left it non-fuctional semi-intentionally. I personally would not want the text message to clutter my screen, but I figured it would be handy in some situations, like if you have no sound (or if you are deaf). So I would suggest making it toggleable with some configuation value, only I am not sure how to go about obtaining such a value from JS. Any suggestions/pointers?

leper reminded me that we have g_ConfigDB, if you're still interested in this, it's used some places in the GUI: ps/trunk/binaries/data/mods/public/gui/session/input.js#L1518

comment:27 by madmax, 11 years ago

Yes, I was able to read the config value using CFG_GET_VAL. Thanks!

Now for the javascript errors.....seems like some kind of git messup.

comment:29 by zoot, 11 years ago

madmax, any progress on the last issue?

comment:30 by Kieran P, 11 years ago

Milestone: Alpha 14Alpha 15
Type: taskenhancement

comment:31 by ben, 10 years ago

Resolution: fixed
Status: newclosed

In 13951:

Implements attack notifications based on patch by madmax and zoot, fixes #1719

comment:32 by historic_bruno, 10 years ago

Keywords: simple review removed

Thanks for the patch.

comment:33 by ben, 10 years ago

In 15163:

Removes pointless minimap state change during attack notification, it broke synchronized simulation state and caused some OOS errors. Fixes #2525. Refs #1719, #2526.

Note: See TracTickets for help on using tickets.