-
Notifications
You must be signed in to change notification settings - Fork 397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance Politics Notifications To Allow For Only Target Players (Feudal Japan, etc) #3558
Comments
So perhaps we can add a boolean property that specifies whether the notification should be sent to one player or all. And have the map set that boolean property. This way, there's still compatibility between old/new map versions and old/new engines. |
An "incompatible" way to do this would be to introduce a separate property - PARTICIPANTS_NOTIFICATION_SUCCESS - which only go to the players participating. But the problem with this is if an older engine reads a map with this new property, without the OTHER_NOTIFICATION_SUCCESS property, then there will be no text to responders. |
@asvitkine I'd prefer the second solution here as its much cleaner and fits with the existing properties better. As long as maps wait til this is released in the next stable (should be soon) then as long as both players use the latest stable then they wouldn't have any issues and the worse case is they miss notifications. |
Ok, I wasn't sure what level of compatibility we wanted. If second solution is acceptable, I'll do it that way. I agree it's cleaner. |
So, I guess I'll give some context, since I guess I'm the only one, beside @veqryn, that is still alive to tell the tale. Hopefully @veqryn will correct me, if I get something wrong, as I'm not sure of what I'm saying. As the OTHER_NOTIFICATION_SUCCESS naming implies, that was supposed to give a notification to the target players (usually, but not necessarily, one). At some point, @veqryn changed it so that everyone would receive the notification. However, existent games, like the one you mention or Napoleonic Empires, had the text tailored under the assumption that only the receiver would get the notification; so, that causes jumpscares in all noobs, that think they have been declared war upon, while somebody else actually was, as the notification just says "They declared war on you!", or something like that. It is good to have the possibility to inform everyone about everything politic that's going on, but it would be neat if this is done differently for the target players (usually just one) and anybody else. So, my suggestions are:
I tend to believe that also spectators should, then, get the "onlooker" notification, but I'm not sure about this. I guess it would be better that if the "onlooker" notification is missing, then everyone, but the action maker, receives the "other" one. However, this would not solve the issue for those very old maps, like Feudal Japan, you are mentioning (practically, since (more than 5 years ago) the behaviour was changed, those maps should have been updated to display a notification worded in a way that makes sense in the moment everyone but the maker receives it; just nobody ever updated them). I hope this clarifies something for you. On the other hand, I'm not sure what you actually intend to do (your second solution). In particular, I'm not sure what "participants" mean. If "the players participating" means that this new notification would go only to the target players, but not to the action maker (that already has its own notification) and not to anyone else, then I suggest calling it differently, as "participants" would be understood as either everyone taking part in the specific politic action, thus both the maker and the receivers, or ever all participants to the game (so, just everyone). |
Also, this is sort of a mix of an engine feature request and a map bug report, but it is not an engine bug, as I believe the current behaviour is intended (by @veqryn ), just the maps were never updated to it. On the other hand, I believe this change was bad, as a matter of naming, as the name remained "other", that, especially for being singular, seems to imply that notification is for the receiver only, not everyone else (or, anyways, the name itself gives really no good hint about how it is going to work). |
@Cernelius While that's probably true, I don't want to change the functionality of every map that is currently using the existing behavior of sending notifications to all. Its easier to create a new parameter that sends to only those which are participating then update the couple of maps like Feudal Japan to use it. |
Cool. @ron-murhammer would you mind explaining exactly and in details how the mentioned solution it is going to work, since I'm not understanding what is going to be. Also, I meant it was a bit confusing only as a matter of naming. Aside from this, I agree with the change made by @veqryn, in that I definitely believe it is better everyone gets notified rather than only the targets; if having to choose between these two alternatives only, that is. |
The new option would essentially just bring back the old behavior of only the target(s) if used. |
Ok, this is not very detailed. Thanks anyways. So I guess I'll have to guess into details and hope to be corrected where I'm wrong.
So, if I'm guessing correctly, as I said, that this new notification option, you are adding, would be given only to the target players, but not to the action maker (that already has its own notification) and not to anyone else, while, now, instead, the existent "other" notification would be given only to those being not the action maker nor the target players (so you are changing the current behaviour of "other", in that the target players won't receive it anymore (receiving the new notification, instead)), then, if all the above is correct, I definitely suggest you don't call this new notification as "PARTICIPANTS" but, rather, as "TARGET" (I'd keep singular, since "other" is singular, already). |
Sorry for not being clear. I'll change the name to TARGET as you suggest since that seems to be more clear. Here's how it will work: TARGET_NOTIFICATION_SUCCESS will send to target(s) only. If TARGET_NOTIFICATION_SUCCESS does not exist in the properties, then OTHER_NOTIFICATION_SUCCESS is used for everyone except active player. The Feudal Japan map will be updated to add TARGET_NOTIFICATION_SUCCESS Same thing for _FAILURE notifications. |
@asvitkine Would it be better if you don't want 'OTHER_NOTIFICATION_SUCCESS' to just not have that property included at all? Or is there already existing behavior for other politics text properties around setting to 'NONE'? For example Feudal Japan currently:
Would become:
|
I agree it would be cleaner to not need to set it to "NONE". It's not what the code currently does, though. I think it's a bit orthogonal - I can submit a separate PR to clean that up - basically to treat both "NONE" and the property not existing the same way. |
Ok, thanks for the details @asvitkine. That leaves me with only a couple of remaining doubts.
@ron-murhammer Since this is not a perfect world, I definitely advise against ever having a behaviour of not displaying anything if absent, as that would bury game bugs and be an issue in the map development process, as you normally first code the xml, then add the text for the skin. "NONE" for not having something displayed is the way to go, tho I would personally prefer that is done by having the notification entry present, but left empty, instead of having it as "NONE" (very minor matter, as I don't think you want to display a "NONE" anyways). On the other hand, I believe the behaviour of first searching in TARGET and, if absent, searching in OTHER is acceptable, as long as there is something, at the end of the priority list, that, if absent, gives a default message of some sort, instead of nothing. It may be good having a way to set any kind of notifications to none, instead of having to set each one of them. In this case, Feudal Japan could be updated just adding a couple lines that set all OTHER_NOTIFICATION_SUCCESS and OTHER_NOTIFICATION_FAILURE to none, beside mass changing the existing OTHER to TARGET. Also, are you sure you wouldn't prefer to be notified about what's happening between others, with notifications like "X has declared war on Y, may the fools drown in their own blood!". For Feudal Japan in particular, maybe better not, considering the spam of AI with AI actions, but I'm not sure. |
A last but not least thing, in case you didn't think about it (I cannot follow you guys through your tech stuff). If the same user is playing both the target player and any other ones, then this user should receive the TARGET notification, but not the OTHER notification. The reason for this is obvious: a game in which you are supposed to handle multiple players will have a TARGET notification making you able to understand which one of your players is the target, while you would get the OTHER only if you are not getting the TARGET, that is if that matters not for your alliance (basically, it would be a game with more than 2 alliances, but not of the FFA kind). On the other hand, the TARGET notification for pure FFA games (more than 2 alliances, but all alliances of 1 player only) would be something like "X has declared war on you" while the OTHER notification would be something like "X has declared war on Y", and you won't ever get both anyways, as you are supposed to play only one player at any times. |
* Add ability for notifications to only go to accepters. This allows for fixing #3558 with a corresponding chsange to the Feudal Japan map setting the new property. * Introduce PARTICIPANTS versions of failure/success notifications. * Change terminology to TARGET. * Fix refactoring issue. * Address comment.
Sounds like a problem with the map not the engine. |
@veqryn The issue this is addressing is that "OTHER" goes to everyone but the player taking the political action. There are some maps where that means a lot of spam messages to players. This feature adds the option of "TARGET" which only send the message to those which are affected by the political action rather than all other players. |
For the Feudal Japan case, you would probably want to know what the other human players are doing, with a notification like "X has made an alliance with Y, those weaklings are truly desperate.". The AI with AI actions are tons, but probably the only ones you don't really want to see are the failure ones, also since it is not a big deal spamming the OK button. Probably the most hardcore players might want to know everything in the moment they could get prompted anytime to accept something, and there is no way to put that on hold and go opening the politics tab to see what's what, but I think it would rarely make a difference on your choice. |
Just really don't have the notification not displayed if absent. That needs to be a NONE or an empty text (I tend to prefer not displaying if empty over not displaying if NONE). That needs keep working as it does now, in all cases, getting a default raw notification, because, in the moment you make a map, first you do the xml stuff, then you customize the text that you get, while you can still test them before doing that skin part. Plus, of course, getting a notification with a missing text is a better way to notice you are missing it or named it wrongly. Maybe the default, non customized, notification may be improved, to be fairly readable, making the politicstext.properties something optional like the tooltips.properties, but it's not really important, I think. |
Reading through the logic and trying to remember: triplea/game-core/src/main/java/games/strategy/triplea/delegate/UserActionDelegate.java Lines 215 to 243 in 5e2a3f3
I believe the way it was implemented expected that the text would read something like:
Having to separate the target out from all "others" will be difficult to do when you have multiple players and shifting alliances. |
@veqryn The way it was implemented is that you only ever get 1 notification and TARGET takes precedence over OTHER. So in your example, let's say I wanted more custom messages and did this:
USA would get the first one. |
Soooo.... this is a change only to the map right? |
This is an engine change to support 2 new properties in politics text properties (TARGET_NOTIFICATION_SUCCESS & TARGET_NOTIFICATION_FAILURE). Which allow map makers to send certain messages to only the "TARGET" of the political action. |
Carry on |
@veqryn Nothing will change for the maps that won't be changed themselves. So, yes, this is a mix of a map specific change and an additional feature disposable for new maps.
@veqryn I already touched this matter in a previous post at this issue (and I hope the developers will consider all details as well). In this case, under my suggestions, you would get the TARGET notification but not the OTHER notification. Practically, the same as you would be only in control of country B. You can expect the mapmaker to code the map in a way to make the TARGET notification meaningful, if you are supposed, by that same map, to control both B and C (the notification won't just say "You have been declared war.", assumingly, that would be a TARGET notification only pure FFA games, like Feudal Japan or the FFA mods of Napoleonic Empires, in which you must always be only 1 country, should have (and, in that case, you should just never be both B and C, as said)). Also, the display can be improved. Currently, for example, in Feudal Japan you get OTHER (soon to be TARGET) notifications like this: There is no point having in the title what you have in the description, so you can have in the title rather something "Political notification to Chosokabe". And, off topic, I would also change the name of the in game players from "player" to "country" or "power", to avoid confusion with the users playing the games (as sometimes a player controls multiple countries), but that has been discussed in forum, and nobody agreed with me. |
Also, I prefer if "politics" would be called "diplomacy", as that would be the correct way to call it. So, I would rather have an autodisplay as "Diplomatical notification to Chosokabe", but that would need changing a lot (only of user display, not xml coding, of course) from "Politic" to "Diplomatic", and that's just my preference. Merely a side note here; I know it will stay politic, sadly. https://en.oxforddictionaries.com/definition/politics
(political relations between states being only a part of it) https://en.oxforddictionaries.com/definition/diplomacy
|
Why has this been closed? Has everything been defined and documented? Also, can we please remove that light bulb on the left side, in all notifications. It just pushes the stuff to the right and it has no real value. |
I'm particularly interested to know what is the intended behaviour in the case @veqryn pointed out, and in making it reliably stable, by well documenting it in pos2.
I'm making a game with politics with 5 alliances and 16 players. So, this is relevant to me. Edit: With 16 players I mean 16 countries. The gamers would be 5, each one controlling an alliance. |
@ron-murhammer I have a very complex diplomatic logic in my Bad-Ass 4 Nation FFA. It is most likely the most intricate version out there. Not to mention it also uses point of agreement rather than generic end of round in the logic. This gives every player completely fair diplomacy, meaning the last in turn order does not get burned. So much was said above, that it isn't very clear on final cut. Can someone quickly post a brief description of end result. |
Just reposting this one incase no one noticed it. @Cernelius and @ron-murhammer I have a very complex diplomatic logic in my Bad-Ass 4 Nation FFA. It is most likely the most intricate version out there. Not to mention it also uses point of agreement rather than generic end of round in the logic. This gives every player completely fair diplomacy, meaning the last in turn order does not get burned. So much was said above, that it isn't very clear on final cut. Can someone quickly post a brief description of end result. So I can determine if it affects my game in any negative ways. |
@General-Dru-Zod I'm in your shoes. Still have to understand why this was even closed, since nothing was clarified. |
@General-Dru-Zod @Cernelius This is an engine change to support 2 new properties in politics text properties (TARGET_NOTIFICATION_SUCCESS & TARGET_NOTIFICATION_FAILURE). Which allow map makers to send certain messages to only the "TARGET" of the political action. The way it was implemented is that you only ever get 1 notification and TARGET takes precedence over OTHER. For example, let's say I wanted more custom messages and did this:
USA would get the first one. POS2: https://github.com/triplea-maps/the_pact_of_steel/pull/20/files |
I think I never ended up making the change to Feudal Japan for this. I also noticed that other FFA maps are affected today. In particular, Napoleonic Wars FFA. So I can take a look at making these follow up changes to the maps to take advantage of the new engine feature. |
Politics notifications in Feudal Japan map go to all players.
On map Feudal Japan, when player A makes peace with player B, then every other player C gets a popup with text "We have made peace, they are afraid of our mighty army.".
This text clearly is not meant to be sent to players not involved in the deal. It should only be sent to player B.
Looking how it's set up, it seems it uses the following property:
Peace.OTHER_NOTIFICATION_SUCCESS=We have made peace, they are afraid of our mighty army.
https://github.com/triplea-maps/feudal_japan/blob/f602a7ec216c62acecf5a36448fe6a6e55f96291/map/politicstext.properties
And that property is coded to give notifications to all other players. It seems like the engine should provide another property that would only go to the deal receiver. And the map could use that.
Changes:
This is an engine change to support 2 new properties in politics text properties (TARGET_NOTIFICATION_SUCCESS & TARGET_NOTIFICATION_FAILURE). Which allow map makers to send certain messages to only the "TARGET" of the political action.
Example:
For example Feudal Japan currently:
Should become:
The text was updated successfully, but these errors were encountered: