Page MenuHomeWildfire Games

Fixed round typo in selection_details.js
AbandonedPublic

Authored by nwtour on Mar 9 2021, 10:58 AM.

Details

Reviewers
Silier
Summary

Strange numbers in selection_details.js


Default values should be rounded (like in gui/common/tooltips.js)

Test Plan

.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

nwtour requested review of this revision.Mar 9 2021, 10:58 AM
nwtour created this revision.
Silier requested changes to this revision.Mar 9 2021, 11:31 AM
Silier added a subscriber: Silier.

please keep in mind value in simulation can be 0.4 in this case it would show 0, what is wrong, therefore in simulation we rather use ceil to not have entities with non 0 hp existing and running around.
it looks like issue with float number storage, because it should not happen to have 500.1 max hp when in template it is 500 (example numbers) and I don't know about bonus which would add 0.1 hp

This revision now requires changes to proceed.Mar 9 2021, 11:31 AM
nwtour added a comment.Mar 9 2021, 4:40 PM
In D3657#159739, @Angen wrote:

please keep in mind value in simulation can be 0.4 in this case it would show 0, what is wrong,
we rather use ceil

I didn't make it up myself. The adjacent file (binaries/data/mods/public/gui/common/tooltips.js) uses:
Math.round(entState.hitpoints)

it looks like issue with float number storage, because it should not happen to have 500.1 max hp when in template it is 500 (example numbers) and I don't know about bonus which would add 0.1 hp

As I understand it, some technologies give a buff. I don’t know why they are used from the beginning of the game, but this is a standard game function
(e.g. in binaries/data/mods/public/simulation/data/technologies/civbonuses/greek_structures.json { "value": "Health/Max", "multiply": 1.1 })
function GetTechModifiedProperty_numeric from binaries/data/mods/public/globalscripts/Technologies.js show as how float appears that is not able to become int
multiply *= modification.multiply where modification.multiply = 0.7
multiply *= modification.multiply where modification.multiply = 0.7
multiply *= modification.multiply where modification.multiply = 0.75
originalValue * multiply + add where originalValue = 1 multiply = 0.36749999999999994 add = 0
multiply *= modification.multiply where modification.multiply = 1.1
multiply *= modification.multiply where modification.multiply = 1.1
originalValue * multiply + add where originalValue = 36 multiply = 1.2100000000000002 add = 0
multiply *= modification.multiply where modification.multiply = 0.7
multiply *= modification.multiply where modification.multiply = 0.75
originalValue * multiply + add where originalValue = 1 multiply = 0.5249999999999999 add = 0

Silier added a comment.Mar 9 2021, 4:59 PM

I know you did not made rounding up :)
I am saying that if we use it in this case, where ingame hp are shown, it will result in entities walking/functioning with displayed 0 hp :) what is no.
it is long standing issue which needs to be fixed systematically in a way, there will not be needs for more diffs like this.
In the past it was multiple times suggested that some global function for displaying or rounding numbers in gui is needed.
If you _really_ want to use round here, I suggest two approaches: 1. round only if value is greater than 1, else use ceil.

  1. round only Max values and clamp current ceiled values with them.
nwtour updated this revision to Diff 16390.Mar 9 2021, 8:48 PM

another way

nwtour added a comment.Mar 9 2021, 8:59 PM
In D3657#159744, @Angen wrote:

If you _really_ want to use round here,

I completely agree with your arguments about the fact that the human brain perceives 0 as UNDEFINED and it is better to round it up

I updated the patch to make it clear how I see it
at the start of the match, the parameters should be natural
Math.ceil should realy work in those moments when something happened in the match.

One parameter in different windows must be the same

Round values are desirable

This patch solves this issues.

Silier requested changes to this revision.Mar 9 2021, 8:59 PM

i cant agree with this change.
also this statement is incorrect

GetTechModifiedProperty run on start of match and params should be natural numbers

the type of the value or parameter it needs / should be is defined in the template schema.

there are several values which need to stay floats (gather rates for example) and should not be forced to be integers.
some attack values are supposed to be also floats

This revision now requires changes to proceed.Mar 9 2021, 8:59 PM
Silier added a comment.Mar 9 2021, 9:05 PM

One parameter in different windows must be the same

variables are the same, displaying is different, that means how they are displayed needs to be unified, hence proposed global function which would returned correctly rounded values to be displayed in gui

nwtour added a comment.EditedMar 9 2021, 9:19 PM
In D3657#159799, @Angen wrote:

hence proposed global function which would returned correctly rounded values to be displayed in gui

My competence in the project and in English is not enough for this process. I can offer moral support ;-)

the type of the value or parameter it needs / should be is defined in the template schema

If the idea is clear (do not start up unjustified EPSILON from modifications at the init stage) and it has prospects, I can start studying the issue...

Silier added a comment.EditedMar 10 2021, 7:59 AM

every entity is defined by components such as Attack, Resources, UnitAI and so on.
every component defines schema for xml, telling what kind of values it can define, for example if walk speed can be decimal number, non negative integer,...
any such a number can be modified by technologies and auras and that's where currently modified function takes place.
so rounding values in this funtion will make from initial decimal values, which ought to stay decimal, integers.

For in game health, the damage calculation is decimal due to fact how armour is calculated so health is decimal number. In fact it have been integer but have been changed to be decimal (some years ago) probably due to change which have been made to the armour as it works currently. But stayed to display as integer because it looks nicer and not too important to see that unit has 500.032 health, just 500 is enough.

now, when displaying in gui (your initial differential), coder usually knows what the number should looks like (how many decimal places to display)
so now instead calling round, ceil, floor, toFixed functions, there would be generic function roundForGui which would take value as parameter and minimal number of decimal places to display. function would take care of not displaying 0 but 0.2, 0.3 if requested decimal places would be 0.

In D3657#159801, @Angen wrote:

so now instead calling round, ceil, floor, toFixed functions

Ok. It will solve issue. I'll then leave a way to reproduce the bug:

cp binaries/data/mods/public/simulation/data/technologies/civbonuses/greek_structures.json binaries/data/mods/public/simulation/data/technologies/civbonuses/greek_structures2.json
(Additional multiple : "1.1" make MaxHitPoints long float)

please read discussion here at first :)
https://code.wildfiregames.com/D2053

nwtour abandoned this revision.Dec 16 2021, 9:34 PM