yairm210 / Unciv

Open-source Android/Desktop remake of Civ V
Mozilla Public License 2.0
8.13k stars 1.52k forks source link

Feature request: Clearer friendlier Battle strength comparison #9205

Closed SomeTroglodyte closed 1 week ago

SomeTroglodyte commented 1 year ago

@Ouaz presented this in an otherwise unrelated issue discussion - and suggestion three is way cool. Quote:

*Currently: current

1st suggestion: HP numbers on/off setting (simplest fix ^^) A setting in Display > Interface: [ ] Show HP numbers in battle table. If unchecked, no numbers, so people with OCD are happy again.

2nd suggestion: Detailed data As people finds 65 -> 35-20 is not a logic way to write the HP (despite the bar above showing exactly that), detailed data is used instead: detailed_hp

3rd suggestion: Big Bang overhaul (designed by @letstalkaboutdune ) big_bang

The 1st suggestion would make my day, if the other ones are too much a hassle to implement. :smiley:

--- End-Quote.

It was also mentioned that the current version is extremely jarring to certain minorities, and if that can be alleviated with something as nice as that, I'd say yes please. (no guarantee I'll take the bait myself though, so if anyone wants to take a shot, "assign yourself" up there top right)

SomeTroglodyte commented 1 year ago

I repeat: way cool...

But say - is it intentional that the "current" and 1st/2nd have the Rifleman start with a full bar, while 3rd has the initially full one on the right?

As for 1st - boss fears options is already overloaded. I can both see that clearly, and at the same time think there's more stuff that could use some user control. Maybe a slider "simple-expert" with 3-5 levels, some tabs appearing only >= certain levels, maybe even individual options, and max setting of the slider is the current "secret debug" page... That way, we could by default also hide modchecker (though not far), and next-to-last would be the translation generator and similar stuff... And/or, use expanders with different starts-opened settings. Someone with a knack for order could maybe...

itanasi commented 1 year ago

I don't think it was intentional. It was a mock up and likely just swapped which started full.

letstalkaboutdune commented 1 year ago

It's been quite a while, but I'm pretty sure it's just a mockup. No meaning to which side is full vs. not, just an example.

yairm210 commented 1 year ago

We had this discussion endlessly on Discord, we did a gazillion polls. People want to see the numbers. If anyone wants to work on the 3rd option then go for it but I'm not doing any more healthbar changes, I'm sick of it. Ouaz knows he's in the minority regarding number direction, since there was a clear poll on the matter.

Ouaz commented 1 year ago

:disappointed:

I really tried to see the numbers disconnected from how the bar depletes, like this: current health -> health range after battle. It seems that's how the majority of people sees it. No problem.

But I can't. I just mentally link the numbers to the colored parts and the health bar direction, making me read it this way: reading

To make an analogy, take the "rabbit shape" on the Moon. Once you see it, it's impossible to "unsee" it.

It's the same thing for me with this bar and these numbers, it's very annoying each time I watch it, because I know these numbers do not match the bar direction just above, and I have to read them this weird way. It even hinders somewhat my gaming experience.

That's why I solemnly ask to add the 1st suggestion: A setting in Display > Interface: [x] Show HP numbers in battle table The setting would be activated by default, so users who have not problem with these numbers, won't even notice the difference.

Thank you for reading.

SomeTroglodyte commented 1 year ago

Look, I'm a lazy guy. And if I plan on someday trying Nr.3, Nr.1 seems like a waste of time. I opened this here also as bookkeeping device... One of the problems is, can't simply "write" that UI from visuals top-down - not because that would be hard in a void, but because it's Unciv and Gdx.

That should be all options - unless there's code painting such a thing but named differently... So first. battle plan how to minimize code duplication. Efficient design would be Widget-based just as ProgressBar with draw and layout overrides - but abusing HorizontalGroup (or VerticalGroup) might be easier and still more efficient than Table. Finish other things first.

And I don't want the Moons to be Rabbit-shaped... Must. Not. Look. Up.

SomeTroglodyte commented 1 year ago

Aside - which of these progress bars in the city button looks best? Marginal diff, and all are the same total logical size. image

itanasi commented 1 year ago

Lower left followed by upper left imo

letstalkaboutdune commented 1 year ago

I think upper left is the best overall. You get the clarity of having some of the black showing up on the sides, but it's wide enough to see the bar well.

SomeTroglodyte commented 1 year ago

Lower left followed by upper left

2/1 are the origaminal width/background pad values, and upper right only looks different because that's a cleanly drawn bar while the left is done using nested Table() stuff. Yup, 3/0.5 looks quite good, but I fear it might not scale so well in physical screen sizes & settings. 4/0 has that material look, it would just need a fix to the vertical padding, and would scale well... But I concur 100% otherwise.

Say, was there discussion that there's an empty black bar there when a stat conversion is queued? I'm 60/40 sure just omitting it would look better... Or it's needed as optical balance? Can't screenie now, Studio locked up again, and cleaning caches + gradle sync takes a long time.

upper left is the best overall

Really? Even with the slight asymmetry in the food bar? That changes with zooming and panning, and the alt implementation doesn't do that so visibly..

letstalkaboutdune commented 1 year ago

It's hard to tell what's "real" and what are rendering/compression artifacts. Also I have no idea what happens with zoom/pan when given only a static image. But actually I'm gonna duck out of this convo because UI in this game gets me tilted.

Ouaz commented 1 year ago

I slightly prefer 3/0.5 due to the bars being wider and therefore kind of "brighter", but 2/1 is very acceptable too.

4/0 is disqualified. ^^ Same for master, now I see the asymetry (I didn't notice it at first... again XD ).

SomeTroglodyte commented 1 year ago

Anyone want to give this a try? Unciv.BattleTable.demo.jar Far from acceptable (try long unit names, or alignments with a city involved), but I'm out of patience for now.

Ouaz commented 1 year ago

Going to try it now! :fire:

SomeTroglodyte commented 1 year ago

I fixed the squashed size with long unit names, the unit/city misalignment and a few other things since then (helpers taking a size parameter and producing an actor - one gives an actor obeying the given size for layout but painting way outside that box, the other simply adding 10% to what the doctor ordered....), but haven't refreshed the jar.

Problem why not production ready - unclean code. Won't push until I've really analyzed almost two pages of (old) code I think are completely redundant; plus that air sweep / nuke - those are code duplication and need to be integrated or given their own treatment. Especially air sweep at the moment has a health bar with zero useful information...

Ouaz commented 1 year ago

That simply looks awesome! What a wizard you are. :shipit: (don't mind the crappy rendering, old laptop with java 1.8, I play only on the Android version)

wow

I don't know what you plan to do for the buff/debuff lines. I think descriptions can be removed (only keep name (type) +/- value% e.g. Shock II (Promotion) +15%, 'Nationalism (Policy) +15%, and therefore the font size could be increased a bit? It does not have to be necessarily linked to their civilopedia articles, if it's too much hassle to implement. It will push players to learn their unit attributes only by their names. :book: But maybe this have to be discussed first (I already see users saying "Where are the buff descriptions!?")

To be annoying until the end, could it be possible to make the red bar "flash" like in the current battle table? It's a nice visual touch, especially when removing a good chunk of the enemy HP bar.

Oh, and I almost forgot... MERCI!

PS: Is it possible to define a fixed height for the bars (as it changes depending the numbers of listed attributes)? Or maybe you have already fixed that. height

SomeTroglodyte commented 1 year ago

play only on the Android version

So the jar was not useful after all? Huh? The branch itself is local only so far... Nevermind.

fixed height for the bars

Nah, I thought it cute they can grow if the text gets long. There is a minimum height, but I set it pretty low - I think the last screenie is that. But attacking a General - didn't notice that icon breaks layout too... Will test. Attacking civilians gives weird results anyway. A 60:1 ratio for a military victin means certain death, but civilians get a different calculation, which should be displayed. Didn't find it yet.

only keep names

I just used what was already available. I didn't take apart BattleDamage.getAttack/DefenseModifiers, only BattleTable.simulateBattle. Uh, wow, good thing, too. A complex function, 50 lines, called twice in succession, double the overhead.. Dampens enthusiasm.

Oh, and I almost forgot... De rien. Parfois les puzzles me intriguent, parfois pas - c'est de la chance. D'ailleurs, ca peut trainer, car il y a trop des details imparfaits, et je perds "l'intrigue". Maintenant, par exemple, je m'en vais lire un livre.

Oh - and if you want to help, could you produce saves where one can quickly test air sweep and nuke attacks? Don't know If I kept any late game. Also, look at the UI and feel free to suggest changes. Nuke, par exemple, could probably do with just a single column - the missile as header like above, and a victim list. Maybe improve by showing collateral damage somehow - filtered by what the aggressor could find out manually...

Ouaz commented 1 year ago

So the jar was not useful after all? Huh?

No, no, useful. I use the PC version to test things (translation, assets, visual mods) and take screenshots/do mockups. I "play" on the android version.

But attacking a General - didn't notice that icon breaks layout too...

I thought it was the the "unit/city misalignment" you mentioned, and that you fixed in the last jar (the white bar and the attributes being shifted)

Uh, wow, good thing, too. Dampens enthusiasm.

:laughing:

Oh - and if you want to help, could you produce saves where one can quickly test air sweep and nuke attacks?

I think I have a late game save file with that somewhere.

Ouaz commented 1 year ago

could you produce saves where one can quickly test air sweep and nuke attacks?

Here it is: AirSweep_Nuke_test.zip Go to Mosul in the lower right corner of the map. airsweep_nuke

Nuke, par exemple, could probably do with just a single column - the missile as header like above, and a victim list.

nuke

I agree. The header showing the missile, and maybe sort the victims in two colums: allied casualties | enemy casualties, with Cities on top.

                Atomic Bomb
[Allied Casualties]    [Enemy Casualties]
        Mosul               Fustat
        Tank                Machine Gun
        Tank                Great General
                            Mobile Sam
                            Sheeps
SomeTroglodyte commented 1 year ago

Here it is:

I forgot you can start in a later era, so producing one is easier than I thought, but thanks anyway. That casualties mockup - feels like a good idea, except if you're nuking "nice", one side is lopsidedly empty... Say, does "Hey those Sheep are a modded unit, right?" translate to "Dis, les Moutons là, c'est une unité moddifiée, n'est-ce pas?" avec double "d"?

Ouaz commented 1 year ago

except if you're nuking "nice", one side is lopsidedly empty...

Ach, I didn't think of that. :grimacing:

Let's keep the single column then. And also the sorting then, as it is based on the bomb radius: units in 1-tile range from ground zero, then units in 2-tile range.

More sheeps. "Sheeps" listed here just to have fun. And because "Sheep" has become an Unciv meme on Discord (https://github.com/RealBamboolord/Sheep-Meta), as it seems map RNG goes sometimes mad with sheeps everywhere at the start. (if I understood correctly the whole story).
SomeTroglodyte commented 1 year ago

map RNG goes sometimes mad with sheeps everywhere

I think that's gone, and was originally not random but the starting code thinking a place was too poor - and it took the first bonus resource to improve. It's Stone getting special treatment nowadays I think.

SomeTroglodyte commented 1 year ago

Let's keep the single column then.

maybe... maybe count first then decide

And also the sorting then, as it is based on the bomb radius: units in 1-tile range from ground zero, then units in 2-tile range.

??? Is that status quo or a wish? If desired, why? Is damage less in outer radius or somesuch? I guess you mean as-is and not helpful. Yo, there's no sorting at the moment and getTilesInDistance goes by radius then two points along each radius "rim" clockwise... And keeping the radius as sort criterion once I start sorting costs extra.

SomeTroglodyte commented 1 year ago

Another jar if you like (outdated)

![image](https://user-images.githubusercontent.com/63000004/234134497-ed9966c8-d831-489e-86c9-5fe26b823e23.png) ... Not sure about that summary text ... The attempt to "intelligently" do two columns or not may be dumb ... the button should be red (regression in master) ... not even looked at better paddings ... Info about expected vegetation destruction or amount of fallout? Nah, later if at all. Now what exactly should air sweep actually show, and how?
Ouaz commented 1 year ago

Looks nice! I will test it tomorrow, I don't have my PC at the moment. The summary text is a nice touch in my opinion.

there's no sorting at the moment and getTilesInDistance goes by radius then two points along each radius "rim" clockwise...

That's what I meant actually.^^ Sorry, my english is sometimes weak, that's why I use mockups most of the time. :p

For the Air Sweep battle table, I have to think about it... Show the numbers of covered tiles by the sweep action? I have no idea actually. XD

itanasi commented 1 year ago

By default for Air Sweep you have no idea if anyone even Can Intercept on a tile. And I'm pretty sure you can't check to see if enemy units have spare moves to use.

SomeTroglodyte commented 1 year ago

Another thing - inherited - it displays only one nuke victim per tile. In your sample save, you can hit two green generals at once, you see only one. Now how to solve that...

For the Air Sweep :broom: - a simple 16-liner (down from 57) could look like this, and I'm tempted to leave it at that: image

The right align of the modifiers is due to the simple code reuse... Otherwise, style of all three attack types would match.

SomeTroglodyte commented 1 year ago

This is The Last Jar.

Time to round this off. Translation templates - so what do we do about that "nuke summary text". In hindsight, I mainly wanted a place to explain the colours used for the victims listed. The main problem with it is - most of the time most of the numbers will be zero. Simply not mentioning a category when there's no victims leads to translation template hell - tons of templates or inflexibility (meaning xlt parts of the summary separately)?

Ideas?

Ouaz commented 1 year ago

Tested, it looks very nice. :+1: For the "0 innocent bystanders, and 0 allies" listed most of the time... well, let's say we care about collateral damages. XD What the colors mean is straightforward anyway, and players will test various target to see the difference (targeting their own cities for example).

Weeeelll... Mr. Nitpick is coming again:

buff_order

Is it possible to sort the promotion buff list? In the example above, by alphabetical order?

Charge
Drill I
Drill II
Drill III

On the other hand, it seems that all the buffs all already sorted by type (good thing) :

Promotion
Policy
Special (Great General, National ability, etc)
Tile attributes
Debuffs

Sorry to ask again things with all the work you already did!

SomeTroglodyte commented 1 year ago

Nitpick

Nah, you're saving me from having to pay attention to everything at once.

I believe I didn't even think of sort order for the modifiers. Currently this project should show the same order as master, 'cuz I didn't touch but reused that part. I remember there's a Counter involved, which would mean... LinkedHashMap preserves order of addition... It's code order of tested uniquetypes, then "Special" ones last like Boarding Flanking,... Within a UniqueType it gets complicated. Drill and Charge are the same UniqueType.Strength, which 42 steps further down come from a HashMap which practically guarantees random retrieval order...

Prettiest might be a lot of code - sort the "randomized" getMatchingUniques sequences one by one for each source before inserting them into the Counter. Sorting later, and the "Source"/"Type" is lost. But - the modifiers functions get called repeatedly, and the majority don't need the order.

I might be tempted to code an animation that makes them change places randomly all the time - wouldn't be any worse...

You know, I also remember you suggesting "make them shorter" and me thinking "OK but pack the verbose explanation into a tooltip" then saying something like - that level of detail should come later, not the first two PR's... I just looked, and that's all nicely contained in one source file I haven't touched here at all!

Oh I just noticed - how do we remove the german flag from those health bars (black for xenophobia, red for bloodlust and gold for greed)?

Ouaz commented 1 year ago

LinkedHashMap preserves order of addition...

That's what I assumed... on most of the units, except this tank on the screenshot. How the hell did I promote it first to Drill II and Drill I in last? :rofl: Answered by you (if I understood well): "Drill and Charge are the same UniqueType.Strength, which 42 steps further down come from a HashMap which practically guarantees random retrieval order..."

I might be tempted to code an animation that makes them change places randomly all the time - wouldn't be any worse...

Save you the trouble. Let's keep the actual code, this "random" sorting isn't a big deal anyway.

You know, I also remember you suggesting "make them shorter" and me thinking "OK but pack the verbose explanation into a tooltip" then saying something like - that level of detail should come later, not the first two PR's... I just looked, and that's all nicely contained in one source file I haven't touched here at all!

THIS is more interesting. Though... will tooltips work on Android? (I never tested touch&hold on Unciv actually, to see if some tooltips are displayed... I don't have my tablet at hand to check). Nota bene: I requested clickable buff lines leading to their civilopedia articles otherwise, and make the font size a bit bigger if only buff names are displayed. :p hum hum annoying guy asking too much

how do we remove the german flag from those health bars (black for xenophobia, red for bloodlust and gold for greed)

Oh geez, never noticed too. :laughing: Another "Once you see it, you can't unsee it".

SomeTroglodyte commented 1 year ago

tooltips work on Android

Sad story. I had a Note 3 once - the S5 with stylus - running some CyanogenMod where "hover (<=~2mm)", was working really well - though no App supported tooltips. But web pages did! Today, that capability seems to be lost.

github-actions[bot] commented 4 months ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.

SeventhM commented 4 months ago

Unstaling with a comment, might give a label if Yairm approves. Personal opinion: the vertical bars looks nice, but I still really want the exact numbers listed somewhere. Should probably get a poll on discord if this goes anywhere

itanasi commented 4 months ago

The arguments about this on Discord were long, fraught, and partially led to letstalkaboutdune leaving

SeventhM commented 4 months ago

Well, I'm not aware of the history here, tbh ¯\_(ツ)_/¯. Probably wasn't paying attention entirely when it was discussed, especially since it wasn't through the polls channel. And, if this would go somewhere that improves the UI, might as well. If you're saying this is a wontfix scenario, then this can be closed then. Just seems at a glance that most of those involved with this issue seems to like the vertical healthbars, which means to me the next step would be polling

itanasi commented 4 months ago

Went looking and found one of the straw polls https://discord.com/channels/586194543280390151/633733497277775884/1064981827544170566

SeventhM commented 4 months ago

The art channel and the polls channel are two very different channels. One gets a ping and most people check, the other... Not my focus

itanasi commented 4 months ago

My comment was less "this is Won't Fix" and more this is a very contentious topic.

Personally adding vertical and keeping the current numbers, perhaps at the bottom of the table, would be an improvement. I think all other additions have been done already at this point.

SeventhM commented 4 months ago

Looking there (had to copy the link to one of my 6 blank test discord servers then click from there... Don't ask why I have 6, I hardly use more than 2 anyways), I seems like the discussion is whether to have hp numbers or not. Maybe members of the art channel doesn't like the numbers, but I definitely do and would just want the information cleaned up. And Yairm's previous message implies the answer is, yes, we should have the numbers and would need to ping if we did a poll.

As for making it vertical, this still doesn't seem to be contentious. Mostly everyone is ok with the idea of vertical bars like in Civ 5. If that can't be done because of a complaint about numbers, then maybe this should be closed because it won't ever move towards a phase where it could be polled

yairm210 commented 4 months ago

Pretty accurate - numbers have to stay, bars can change

Ouaz commented 4 months ago

If the bars are made vertical (which I personally like, alongside the new battle table layout), the numbers won't be a problem anymore and they can stay as they are (i.e current HP -> Min-Max remaining HP range)

The fuss about the numbers was not about the numbers themselves, but the wrong order, which is counter-intuitive based on what HP projection the horizontal bar shows just above (`current HP -> Max-Min remaining HP range )

Horizontal bar (wrong):

With the vertical bar, the discrepancy is solved, as the bar will be read independently from the remaining HP points projection range. verticalbar

SeventhM commented 4 months ago

The fuss about the numbers was not about the numbers themselves

Ehhhhh, you say that, but reading through that link to discord itanasi sent clearly shows there's also a subfaction that wants no numbers whatsoever, preferring to rely on standardized tick marks at most. Even the first suggestion mentioned in the issue's OP specifically is just a setting to remove the numbers with no other changes. I can totally see flipping the order of the numbers, but removing them outright is a nogo from me

Also, unless the new layout actually shows the numbers alongside the graph as your mockup shows, The whole concern about the numbers being backwards would still inevitably be there, so it's not like the move to a vertical graph really solves it

Ouaz commented 4 months ago

there's also a subfaction that wants no numbers whatsoever

Haha, yes, they wanted no number (me included) or at least make them optional, once the request to modify the order has been rejected (mostly because people didn't understand that these numbers represent the color ranges in the HP bar). It's really annoying to read these numbers while they're not matching the way the bar depletes JUST above. Currently: image

unless the new layout actually shows the numbers alongside the graph as your mockup shows

The new layout will show predefined step numbers only (100, 75, 50, 25, 0), not the exact HP values for the given battle.

So for me, the vertical bar solves it, as the HP numbers (Current -> Min-Max) will be far from it, not just next to it and "parallel", like with the horizontal bar, which causes the reading discrepancy.

The bar depletes vertically, the number projection shows the current HP -> remaining HP, and are read independently from the bar.

vertical2

itanasi commented 4 months ago

Part of the disagreement on convention was numbers aligning to the bars.

In English normally you do Max - Min for a range of values. For whatever reason some RPGs do Min - Max, which is what we have now. And when we talked about horizontal bars going to center/outside, there was even worse weirdness of misalignment between the panels.

Vertical bars avoid all that, and so just keep the numbers consistent on both panels. We can argue about Max-Min v Min-Max in a separate Issue/Discord.

SeventhM commented 3 months ago

once the request to modify the order has been rejected

Huh, didn't realized it got polled already

In English normally you do Max - Min for a range of values. For whatever reason some RPGs do Min - Max, which is what we have now

Eh... depends. I mean, for code, I feel like min <= x && x < max is more common than max > x && x >= min, mostly as a side effect of stuff like for loops, and min - max does make more sense for ranges in some cases ("from 10 down to 5" sounds like downplaying as opposed to "from 5 up to 10"). The main reason imo it should go max-min here is more so the context of the arrow beforehand makes max-min look cleaner. Tbh, I don't really care how it looks next to the bar because the bar is itself hard to read being as small as it is and with no extra markers. So while everyone else might say vertical bars fix it, I really don't agree. I view that as a separate discussion altogether

github-actions[bot] commented 3 weeks ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.

github-actions[bot] commented 1 week ago

This issue was closed because it has been stalled for 5 days with no activity.