Charts improvements (#9168)

* Let spectator see civ groups in VictoryScreen

* Display defeated players normally for charts. Otherwise it's difficult to see which line belongs to them.

* Don't show a 0 for defeated civs.

* Delete dead code.

* Show the civ icon on the last data point within the chart, not next to it and simplify some computations with now obsolete paddings.

* Support negative values in the chart

* Remove TODO for negative values

core/src/com/unciv/ui/components/LineChart.kt

@@ -4,9 +4,7 @@ import com.badlogic.gdx.graphics.Color
 import com.badlogic.gdx.graphics.g2d.Batch
 import com.badlogic.gdx.graphics.glutils.ShapeRenderer
 import com.badlogic.gdx.math.Matrix4
-import com.badlogic.gdx.math.Rectangle
 import com.badlogic.gdx.math.Vector2
-import com.badlogic.gdx.scenes.scene2d.Actor
 import com.badlogic.gdx.scenes.scene2d.ui.Image
 import com.badlogic.gdx.scenes.scene2d.ui.Label
 import com.badlogic.gdx.scenes.scene2d.ui.Widget
@@ -14,18 +12,14 @@ import com.badlogic.gdx.utils.Align
 import com.unciv.logic.civilization.Civilization
 import com.unciv.ui.components.extensions.surroundWithCircle
 import com.unciv.ui.screens.victoryscreen.VictoryScreenCivGroup
+import com.unciv.ui.screens.victoryscreen.VictoryScreenCivGroup.DefeatedPlayerStyle
 import kotlin.math.ceil
 import kotlin.math.log10
+import kotlin.math.max
 import kotlin.math.pow
 
 private data class DataPoint(val x: Int, val y: Int, val civ: Civilization)
 
-// TODO: This currently does not support negative values (e.g. for happiness or gold). Adding this
-//  seems like a major hassle. The question would be if you'd still want the x axis to be on the
-//  bottom, or whether it should move up somewhere to the middle. What if all values are negative?
-//  Should it then go to the top? And where do the labels of the x-axis go anyways? Or would we just
-//  want a non-zero based y-axis (yikes). Also computing the labels for the y axis, so that they are
-//  "nice" (whatever that means) would be quite challenging.
 class LineChart(
     data: Map<Int, Map<Civilization, Int>>,
     private val viewingCiv: Civilization,
@@ -49,12 +43,12 @@ class LineChart(
      * as `0` is not counted. */
     private val maxLabels = 10
 
-    private val paddingBetweenCivs = 10f
-    private val civGroupToChartPadding = 10f
-
     private val xLabels: List<Int>
     private val yLabels: List<Int>
 
+    private val hasNegativeYValues: Boolean
+    private val negativeYLabel: Int
+
     private val dataPoints: List<DataPoint> = data.flatMap { turn ->
         turn.value.map { (civ, value) ->
             DataPoint(turn.key, value, civ)
@@ -62,11 +56,21 @@ class LineChart(
     }
 
     init {
+        hasNegativeYValues = dataPoints.any { it.y < 0 }
         xLabels = generateLabels(dataPoints.maxOf { it.x })
         yLabels = generateLabels(dataPoints.maxOf { it.y })
+        val lowestValue = dataPoints.minOf { it.y }
+        negativeYLabel = if (hasNegativeYValues) -getNextNumberDivisibleByPowOfTen(-lowestValue) else 0
     }
 
     private fun generateLabels(maxValue: Int): List<Int> {
+        val maxLabelValue = getNextNumberDivisibleByPowOfTen(maxValue)
+        val stepSize = ceil(maxLabelValue.toFloat() / maxLabels).toInt()
+        // `maxLabels + 1` because we want to end at `maxLabels * stepSize`.
+        return (0 until maxLabels + 1).map { (it * stepSize) }
+    }
+
+    private fun getNextNumberDivisibleByPowOfTen(maxValue: Int): Int {
         val numberOfDigits = ceil(log10(maxValue.toDouble())).toInt()
         val maxLabelValue = when {
             numberOfDigits <= 0 -> 1
@@ -78,54 +82,26 @@ class LineChart(
                 ceil(maxValue.toDouble() / oneWithZeros).toInt() * oneWithZeros.toInt()
             }
         }
-        val stepSize = ceil(maxLabelValue.toFloat() / maxLabels).toInt()
+        return maxLabelValue
-        // `maxLabels + 1` because we want to end at `maxLabels * stepSize`.
-        return (0 until maxLabels + 1).map { (it * stepSize) }
     }
 
     override fun draw(batch: Batch, parentAlpha: Float) {
         super.draw(batch, parentAlpha)
 
         // Save the current batch transformation matrix
-         val oldTransformMatrix = batch.transformMatrix.cpy()
+        val oldTransformMatrix = batch.transformMatrix.cpy()
         // Set the batch transformation matrix to the local coordinates of the LineChart widget
         val stageCoords = localToStageCoordinates(Vector2(0f, 0f))
         batch.transformMatrix = Matrix4().translate(stageCoords.x, stageCoords.y, 0f)
 
-/* +++ WhoIsJohannes's code drew all the CivGroups on the right - I replaced that with just a
-   +++ Nation symbol at the selected civ's line end
-
-        // We draw civilization labels first, because they limit the extension of the chart to the
-        // right. We want to draw orientation lines together with the labels of the y axis and
-        // therefore we need to know first how much space the civilization boxes took on the right.
-        var yPosOfNextCiv = chartHeight
-        val civGroups = lastTurnDataPoints.toList().sortedByDescending { (_, v) -> v.y }.map {
-            VictoryScreenCivGroup(it.first, " - ", it.second.y.toString(), currentPlayerCiv)
-        }
-        val largestCivGroupWidth = civGroups.maxOf { it.width }
-        civGroups.forEach {
-            it.setPosition(
-                chartWidth - largestCivGroupWidth + (largestCivGroupWidth - it.width) / 2,
-                yPosOfNextCiv - it.height
-            )
-            it.draw(batch, 1f)
-            // Currently we don't really check whether y is overflowing to the bottom here.
-            yPosOfNextCiv -= it.height + paddingBetweenCivs
-        }
- */
         val lastTurnDataPoints = getLastTurnDataPoints()
-        val selectedCivIcon: Actor? =
-            if (selectedCiv !in lastTurnDataPoints) null
-            else VictoryScreenCivGroup(selectedCiv, "", viewingCiv).children[0].run {
-                (this as? Image)?.surroundWithCircle(30f, color = Color.LIGHT_GRAY)
-                    ?: this
-            }
-        val largestCivGroupWidth = if (selectedCivIcon == null) -civGroupToChartPadding else 33f
 
         val labelHeight = Label("123", Label.LabelStyle(Fonts.font, axisLabelColor)).height
         val yLabelsAsLabels =
                 yLabels.map { Label(it.toString(), Label.LabelStyle(Fonts.font, axisLabelColor)) }
-        val widestYLabelWidth = yLabelsAsLabels.maxOf { it.width }
+        val negativeYLabelAsLabel =
+                Label(negativeYLabel.toString(), Label.LabelStyle(Fonts.font, axisLabelColor))
+        val widestYLabelWidth = max(yLabelsAsLabels.maxOf { it.width }, negativeYLabelAsLabel.width)
         // We assume here that all labels have the same height. We need to deduct the height of
         // a label from the available height, because otherwise the label on the top would
         // overrun the height since the (x,y) coordinates define the bottom left corner of the
@@ -138,34 +114,39 @@ class LineChart(
                 xAxisLabelsHeight + axisToLabelPadding + axisLineWidth / 2 - zeroYAxisLabelHeight / 2
         val yAxisLabelYRange = yAxisLabelMaxY - yAxisLabelMinY
 
-        // We draw the y-axis labels second. They will take away some space on the left of the
+        // We draw the y-axis labels first. They will take away some space on the left of the
         // widget which we need to consider when drawing the rest of the graph.
-        yLabelsAsLabels.forEachIndexed { index, label ->
+        var yAxisYPosition = 0f
-            val yPos = yAxisLabelMinY + index * (yAxisLabelYRange / (yLabels.size - 1))
+        val negativeOrientationLineYPosition = yAxisLabelMinY + labelHeight / 2
+        val yLabelsToDraw = if (hasNegativeYValues) listOf(negativeYLabelAsLabel) + yLabelsAsLabels else yLabelsAsLabels
+        yLabelsToDraw.forEachIndexed { index, label ->
+            val yPos = yAxisLabelMinY + index * (yAxisLabelYRange / (yLabelsToDraw.size - 1))
             label.setPosition((widestYLabelWidth - label.width) / 2, yPos)
             label.draw(batch, 1f)
 
-            // Draw y-axis orientation lines
+            // Draw y-axis orientation lines and x-axis
-            if (index > 0) drawLine(
+            val zeroIndex = if (hasNegativeYValues) 1 else 0
+            drawLine(
                 batch,
                 widestYLabelWidth + axisToLabelPadding + axisLineWidth,
                 yPos + labelHeight / 2,
-                chartWidth - largestCivGroupWidth - civGroupToChartPadding,
+                chartWidth,
                 yPos + labelHeight / 2,
-                orientationLineColor,
+                if (index != zeroIndex) orientationLineColor else axisColor,
-                orientationLineWidth
+                if (index != zeroIndex) orientationLineWidth else axisLineWidth
             )
+            if (index == zeroIndex) {
+                yAxisYPosition = yPos + labelHeight / 2
+            }
         }
 
         // Draw x-axis labels
         val xLabelsAsLabels =
                 xLabels.map { Label(it.toString(), Label.LabelStyle(Fonts.font, axisLabelColor)) }
-        val firstXAxisLabelWidth = xLabelsAsLabels[0].width
         val lastXAxisLabelWidth = xLabelsAsLabels[xLabelsAsLabels.size - 1].width
         val xAxisLabelMinX =
-                widestYLabelWidth + axisToLabelPadding + axisLineWidth / 2 - firstXAxisLabelWidth / 2
+                widestYLabelWidth + axisToLabelPadding + axisLineWidth / 2
-        val xAxisLabelMaxX =
+        val xAxisLabelMaxX = chartWidth - lastXAxisLabelWidth / 2
-                chartWidth - largestCivGroupWidth - paddingBetweenCivs - lastXAxisLabelWidth / 2
         val xAxisLabelXRange = xAxisLabelMaxX - xAxisLabelMinX
         xLabels.forEachIndexed { index, labelAsInt ->
             val label = Label(labelAsInt.toString(), Label.LabelStyle(Fonts.font, axisLabelColor))
@@ -173,41 +154,31 @@ class LineChart(
             label.setPosition(xPos - label.width / 2, 0f)
             label.draw(batch, 1f)
 
-            // Draw x-axis orientation lines
+            // Draw x-axis orientation lines and y-axis
-            if (index > 0) drawLine(
+            drawLine(
                 batch,
                 xPos,
                 labelHeight + axisToLabelPadding + axisLineWidth,
                 xPos,
                 chartHeight,
-                orientationLineColor,
+                if (index > 0) orientationLineColor else axisColor,
-                orientationLineWidth
+                if (index >0) orientationLineWidth else axisLineWidth
             )
         }
 
-        // Draw y-axis
-        val yAxisX = widestYLabelWidth + axisToLabelPadding + axisLineWidth / 2
-        val xAxisY = labelHeight + axisToLabelPadding + axisLineWidth / 2
-        drawLine(batch, yAxisX, xAxisY, yAxisX, chartHeight, axisColor, axisLineWidth)
-
-        // Draw x-axis
-        val xAxisRight = chartWidth - largestCivGroupWidth - civGroupToChartPadding
-        drawLine(batch, yAxisX, xAxisY, xAxisRight, xAxisY, axisColor, axisLineWidth)
-
-
         // Draw line charts for each color
         val linesMinX = widestYLabelWidth + axisToLabelPadding + axisLineWidth
-        val linesMaxX =
+        val linesMaxX = chartWidth - lastXAxisLabelWidth / 2
-                chartWidth - largestCivGroupWidth - civGroupToChartPadding - lastXAxisLabelWidth / 2
+        val linesMinY = yAxisYPosition
-        val linesMinY = labelHeight + axisToLabelPadding + axisLineWidth
         val linesMaxY = chartHeight - labelHeight / 2
         val scaleX = (linesMaxX - linesMinX) / xLabels.max()
         val scaleY = (linesMaxY - linesMinY) / yLabels.max()
+        val negativeScaleY = if (hasNegativeYValues) (linesMinY - negativeOrientationLineYPosition) / -negativeYLabel else 0f
         val sortedPoints = dataPoints.sortedBy { it.x }
         val pointsByCiv = sortedPoints.groupBy { it.civ }
         // We want the current player civ to be drawn last, so it is never overlapped by another player.
         val civIterationOrder =
-        // By default the players with the highest points will be drawn last (i.e. they will
+                 // By default the players with the highest points will be drawn last (i.e. they will
                 // overlap others).
                 pointsByCiv.keys.toList().sortedBy { lastTurnDataPoints[it]!!.y }
                     .toMutableList()
@@ -221,24 +192,38 @@ class LineChart(
             for (i in 1 until points.size) {
                 val prevPoint = points[i - 1]
                 val currPoint = points[i]
-                // See TODO at the top of the file. We currently don't support negative values.
+                val prevPointYScale = if (prevPoint.y < 0f) negativeScaleY else scaleY
-                if (prevPoint.y < 0) continue
+                val currPointYScale = if (currPoint.y < 0f) negativeScaleY else scaleY
-                if (currPoint.y < 0) continue
                 drawLine(
                     batch,
-                    linesMinX + prevPoint.x * scaleX, linesMinY + prevPoint.y * scaleY,
+                    linesMinX + prevPoint.x * scaleX, linesMinY + prevPoint.y * prevPointYScale,
-                    linesMinX + currPoint.x * scaleX, linesMinY + currPoint.y * scaleY,
+                    linesMinX + currPoint.x * scaleX, linesMinY + currPoint.y * currPointYScale,
                     civ.nation.getOuterColor(), chartLineWidth
                 )
-            }
-        }
 
-        // Draw the selected Civ icon to the right of its last datapoint
+                // Draw the selected Civ icon on its last datapoint
-        selectedCivIcon?.run {
+                if (i == points.size - 1 && selectedCiv == civ && selectedCiv in lastTurnDataPoints) {
-            val yPos = linesMinY + lastTurnDataPoints[selectedCiv]!!.y * scaleY
+                    val selectedCivIcon =
-            setPosition(chartWidth, yPos, Align.right)
+                            VictoryScreenCivGroup(
-            setSize(33f, 33f) // Dead Civs need this
+                                selectedCiv,
-            draw(batch, parentAlpha)
+                                "",
+                                viewingCiv,
+                                DefeatedPlayerStyle.REGULAR
+                            ).children[0].run {
+                                (this as? Image)?.surroundWithCircle(30f, color = Color.LIGHT_GRAY)
+                                    ?: this
+                            }
+                    selectedCivIcon.run {
+                        setPosition(
+                            linesMinX + currPoint.x * scaleX,
+                            linesMinY + currPoint.y * currPointYScale,
+                            Align.center
+                        )
+                        setSize(33f, 33f) // Dead Civs need this
+                        draw(batch, parentAlpha)
+                    }
+                }
+            }
         }
 
         // Restore the previous batch transformation matrix

core/src/com/unciv/ui/screens/victoryscreen/VictoryScreenCharts.kt

@@ -16,6 +16,7 @@ import com.unciv.ui.images.ImageGetter
 import com.unciv.ui.screens.basescreen.BaseScreen
 import com.unciv.ui.screens.worldscreen.WorldScreen
 import com.unciv.ui.screens.newgamescreen.TranslatedSelectBox
+import com.unciv.ui.screens.victoryscreen.VictoryScreenCivGroup.DefeatedPlayerStyle
 
 class VictoryScreenCharts(
     worldScreen: WorldScreen
@@ -69,7 +70,7 @@ class VictoryScreenCharts(
         for (civEntry in sortedCivs) {
             if (civEntry.civ != selectedCiv) civButtonsTable.add()
             else civButtonsTable.add(markerIcon).size(24f).right()
-            val button = VictoryScreenCivGroup(civEntry, viewingCiv)
+            val button = VictoryScreenCivGroup(civEntry, viewingCiv, DefeatedPlayerStyle.REGULAR)
             button.touchable = Touchable.enabled
             civButtonsTable.add(button).row()
             button.onClick {

core/src/com/unciv/ui/screens/victoryscreen/VictoryScreenCivGroup.kt

@@ -20,15 +20,38 @@ internal class VictoryScreenCivGroup(
     civ: Civilization,
     separator: String,
     additionalInfo: String,
-    currentPlayer: Civilization
+    currentPlayer: Civilization,
+    defeatedPlayerStyle: DefeatedPlayerStyle
 ) : Table() {
     // Note this Table has no skin - works as long as no element tries to get its skin from the parent
 
-    constructor(civEntry: VictoryScreen.CivWithStat, currentPlayer: Civilization)
+    internal enum class DefeatedPlayerStyle {
-            : this(civEntry.civ, ": ", civEntry.value.toString(), currentPlayer)
+        REGULAR,
-    constructor(civ: Civilization, additionalInfo: String, currentPlayer: Civilization)
+        GREYED_OUT,
-            // That tr() is only needed to support additionalInfo containing {} because tr() doesn't support nested ones.
+    }
-            : this(civ, "\n", additionalInfo.tr(), currentPlayer)
+
+    constructor(
+        civEntry: VictoryScreen.CivWithStat,
+        currentPlayer: Civilization,
+        defeatedPlayerStyle: DefeatedPlayerStyle = DefeatedPlayerStyle.GREYED_OUT
+    )
+            : this(
+        civEntry.civ,
+        ": ",
+        // Don't show a `0` for defeated civs.
+        if (civEntry.civ.isDefeated()) "" else civEntry.value.toString(),
+        currentPlayer,
+        defeatedPlayerStyle
+    )
+
+    constructor(
+        civ: Civilization,
+        additionalInfo: String,
+        currentPlayer: Civilization,
+        defeatedPlayerStyle: DefeatedPlayerStyle = DefeatedPlayerStyle.GREYED_OUT
+    )
+    // That tr() is only needed to support additionalInfo containing {} because tr() doesn't support nested ones.
+            : this(civ, "\n", additionalInfo.tr(), currentPlayer, defeatedPlayerStyle)
 
     init {
         var labelText = if (additionalInfo.isEmpty()) civ.civName
@@ -37,12 +60,14 @@ internal class VictoryScreenCivGroup(
         val backgroundColor: Color
 
         when {
-            civ.isDefeated() -> {
+            civ.isDefeated() && defeatedPlayerStyle == DefeatedPlayerStyle.GREYED_OUT -> {
                 add(ImageGetter.getImage("OtherIcons/DisbandUnit")).size(30f)
                 backgroundColor = Color.LIGHT_GRAY
                 labelColor = Color.BLACK
             }
-            currentPlayer == civ // || game.viewEntireMapForDebug
+            currentPlayer.isSpectator()
+                    || civ.isDefeated() && defeatedPlayerStyle == DefeatedPlayerStyle.REGULAR
+                    || currentPlayer == civ // || game.viewEntireMapForDebug
                     || currentPlayer.knows(civ)
                     || currentPlayer.isDefeated()
                     || currentPlayer.victoryManager.hasWon() -> {