#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: |
Attachments (3)
Change History (36)
comment:1 by , 11 years ago
Keywords: | simple gui minimap added |
---|
comment:2 by , 11 years ago
comment:3 by , 11 years ago
See this thread for a possible starting point: http://www.wildfiregames.com/forum/index.php?showtopic=17099&#entry263732
by , 11 years ago
Attachment: | AttackNotificationApr062013.patch added |
---|
The patch file for adding attack notification to 0AD
comment:4 by , 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 , 11 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 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.
comment:6 by , 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.
by , 11 years ago
Attachment: | AttackNotificationApr112013.diff added |
---|
New patch with better attack code from zoot
follow-ups: 8 11 comment:7 by , 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 singlePing()
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()
inhandleNotifications()
. 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 toTakeDamage(
or handling it in the damage receiver? TheAttackDetection
component already handlesMT_Attacked
messages - seems like a good fit and minimizes the interface other components need to use. - In
AttackDetection
could you use squared-distance instead ofMath.sqrt
? Should be faster and you could also use e.g. x*x instead ofMath.pow
for squaring. handleAttack()
is only called one place, it could be moved into or replaceAttackAlert()
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 ofcmpTimer.GetTime()
before the loop to avoid calling it too often. It's too badCmpTimer
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 typebool
notentity_id_t
that might be expected. Could it be namedm_IsPinging
instead? Similarly,IsEntityPinging()
could be simplified toIsPinging()
, 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. Thedistance()
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.
follow-ups: 9 26 comment:8 by , 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()
inhandleNotifications()
.
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 toTakeDamage(
or handling it in the damage receiver? TheAttackDetection
component already handlesMT_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 ofMath.sqrt
? Should be faster and you could also use e.g. x*x instead ofMath.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.
follow-up: 10 comment:9 by , 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 theAttackDetection
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)
comment:10 by , 11 years ago
Replying to historic_bruno:
It seems
AttackDetection
already has a list of every entity currently being suppressed (theevent
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 byTakeDamage()
.
Actually, no, the event
list is a quite short list of recent attacks. (Notifications for new attacks are suppressed if near a recent attack.)
follow-up: 12 comment:11 by , 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? :)
comment:12 by , 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.
comment:13 by , 11 years ago
Here are my changes:
https://github.com/zootzoot/0ad/compare/46b04cd...3a86a3b (diff)
comment:14 by , 11 years ago
Owner: | set to |
---|
comment:15 by , 11 years ago
Keywords: | review added |
---|
follow-up: 17 comment:16 by , 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.
comment:17 by , 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:19 by , 11 years ago
No, haven't heard anything from him apart from what is here and on the forum.
follow-ups: 21 22 comment:20 by , 11 years ago
Ugh!! I totally forgot about this. Sorry! I ll check out the comments and post an update today.
comment:22 by , 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 , 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 , 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 , 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
comment:26 by , 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 , 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:28 by , 11 years ago
Our latest patch can be found here:
https://github.com/zootzoot/0ad/compare/attack-notification.diff
comment:30 by , 11 years ago
Milestone: | Alpha 14 → Alpha 15 |
---|---|
Type: | task → enhancement |
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.