Page MenuHomeWildfire Games

Fix the promotion healthpoint missing issue
ClosedPublic

Authored by Grugnas on May 3 2017, 12:55 PM.

Details

Summary

Whenever an unit get promoted, there is always 1 hitpoint missing

Test Plan

Fixed by ceiling the result of the division cmpPromotedUnitHealth.GetMaxHitpoints() * healthFraction instead of round.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1443
Build 2281: Vulcan BuildJenkins
Build 2280: arc lint + arc unit

Event Timeline

Grugnas created this revision.May 3 2017, 12:55 PM
elexis added a comment.May 3 2017, 1:47 PM

Not always 1HP missing, at least in the tooltips.
But to see it in the tooltips, go age 3 ("gift from the gods" cheat), then promoting a briton spearman for example.

If we use the developer overlay (alt+d) and show the entity state, we can observe the following:

hitpoints: 190
maxHitpoints: 190.08

So it seems more related to the neverending issue of having floats for health in the simulation inthe first place, i.e. #3818

Also misses {"nick": "Grugnas", "name": "Giuseppe Tranchese"} in case we wanted to commit this, but it looks like it doesn't really solve the above tickets.

Grugnas added a comment.EditedMay 3 2017, 2:28 PM

Acually i am not convinced anymore about adressing the issue to the promoted hitpoints,

EDIT: promoting an unit to rank 3 and then phase up, the unit wont have ant missing hitpoints.

Vulcan added a subscriber: Vulcan.May 3 2017, 2:54 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/970/ for more details.

fatherbushido requested changes to this revision.May 3 2017, 4:24 PM
fatherbushido added a subscriber: fatherbushido.
fatherbushido added inline comments.
binaries/data/mods/public/simulation/components/Promotion.js
61

Note for myself: that "satlin" function is used in many places.

62

Is the Round Ceil Floor really needed?
Moreover I wonder if we can't directly call that Health function without using that ratio.
Notice also that while at it, do the same modification in Transform.
Ultimately, that function should replace the present part of the code. (See the related ticket).

This revision now requires changes to proceed.May 3 2017, 4:24 PM
Grugnas updated this revision to Diff 1616.EditedMay 3 2017, 10:22 PM
Grugnas edited edge metadata.

By Truncating the surplus decimals whenever the GetHitpoints function is called(truncate from the 3rd decimal), the simulation properly loads the hitpoints whenever a multiplication occurs and the GUI display the currect value for any promoted unit and Hellenic civic Centers.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/979/ for more details.

fatherbushido requested changes to this revision.May 4 2017, 11:42 AM

There is some work to do here. Look at the Health code (and the SetHitpoints function), look also at the Transform helper (for the health part).

binaries/data/mods/public/simulation/components/Health.js
79

no

84

no

This revision now requires changes to proceed.May 4 2017, 11:42 AM
Grugnas updated this revision to Diff 1643.May 4 2017, 1:14 PM
Grugnas edited edge metadata.

Update: Simply flooring the maxhitpoints calculus result, buildings and promoted units max hitpoints has integer value in the simulation and in the gui. The most noticeable side effect is that rank 3 units in City Phase get 190 max hitpoints instead of 190.08.
I merged the change with some changes proposed by fatherbushido in a patch attached to the ticket #3818.

Owners added subscribers: Restricted Owners Package, Restricted Owners Package.May 4 2017, 1:14 PM
Grugnas updated this revision to Diff 1644.May 4 2017, 1:18 PM

remove tooltips.js file from the diff

fatherbushido requested changes to this revision.May 4 2017, 3:58 PM

You are losing yourself.
I just said:
Some of the work done in Promotion is done in Health.SetHitPoints so that needs to be studied, checked, ...
The same thing is done in Transform, so you can in the same time make it coherent (even if ultimately Promotion should use that Transform helper, there is a ticket and a patch for that).

This revision now requires changes to proceed.May 4 2017, 3:58 PM
elexis added a comment.May 4 2017, 4:01 PM

toFixed returns a string, not a number. Don't know why cost are rounded to two digits after the comma (if it were to be used, convert it to a number according to http://trac.wildfiregames.com/wiki/Coding_Conventions with a + before the string)

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1001/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1002/ for more details.

not yet. I'm sorry for the time you spent waiting for an update, but actually I am kinda busy with my studies and got no time to finish.

Grugnas planned changes to this revision.May 26 2017, 9:35 AM
fatherbushido resigned from this revision.Aug 2 2017, 9:53 PM
fatherbushido requested changes to this revision.EditedAug 18 2017, 12:22 PM

Will you upload the proper patch?
Imo the first step is just to nuke the round which has no reason to be here. (health is decimal in the sim. And it's not the role of Promotion to do the job of Health).
It will fix the reported issue.

binaries/data/mods/public/simulation/components/Promotion.js
62

As I said, just do that. (Removing the Round).

Grugnas updated this revision to Diff 3161.EditedAug 18 2017, 1:12 PM
Grugnas edited edge metadata.

updated to fit a22 changes.
actually this also fixes the extra healthpoint for the hellenic civic center

fatherbushido added a comment.EditedAug 18 2017, 1:26 PM

Well I still don't follow. It's a one liner isn't it?
L62 of Promotion.js (as in your current diff)

fatherbushido requested changes to this revision.Aug 18 2017, 1:26 PM
This revision now requires changes to proceed.Aug 18 2017, 1:26 PM

Well I still don't follow. It's a one liner isn't it?
L62 of Promotion.js (as in your current diff)

it is 1 line only (???). Anyway since that line uses OnValueModification from Health.js which is used by hellenic buildings hp bonus and promotion and returns an unmanageable value ( f.e. for the civice center 3300.0000005 showing 3301 in the gui), the rounding in Promotion.js is not needed because the round is done on the lane 403 of Health.js.

Yes, that one line in Promotion.js probably fixes the issue in the title.
Yes, rounding issues are messed up everywhere and the proposed patch is probably as incomplete as the other patches where people have worked on that issue.

fatherbushido added a comment.EditedAug 18 2017, 2:13 PM

Following comment:

There are finally not so much rounding issues (the main/deepest one imo is the relative template todo).
But above all, the goal of the diff is not adress that (by adding more mess) but to fix that real (simulation) bug.

In D409#31655, @Grugnas wrote:

it is 1 line only (???). Anyway since that line uses OnValueModification from Health.js which is used by hellenic buildings hp bonus and promotion and returns an unmanageable value ( f.e. for the civice center 3300.0000005 showing 3301 in the gui), the rounding in Promotion.js is not needed because the round is done on the lane 403 of Health.js.

You did a grey mix of unrelated things.
That rounding in Promotion.js is not "not needed", it's just false.

Grugnas updated this revision to Diff 3162.Aug 18 2017, 2:30 PM
Grugnas edited edge metadata.

The rounding on promotedUnitHitpoints variable has been removed because units promoted while in phase 3 get some missing hp compared to the max hp.
This simply fix the bug described by the title.

Grugnas updated this revision to Diff 3163.Aug 18 2017, 2:37 PM
fatherbushido accepted this revision.Aug 18 2017, 4:05 PM

Thx for the patch (see D798 for the bug catch).
Ideally that should be handle in Health component (see leper's MoveFrom idea in P46) which would avoid such inconsistencies (it's the role of Health to know what to do with health, not the role of Promotion).

This revision is now accepted and ready to land.Aug 18 2017, 4:05 PM
FeXoR accepted this revision.EditedAug 18 2017, 5:09 PM
FeXoR added a subscriber: FeXoR.

This patch has been reviewed and accepted and I also agree with it.

After reading through Promotion.js and Health.js (and parts of Heal.js) this seams like the way to go.

@fatherbushido 's comment about the helth change actually should be part pf Helth.js might be valid.
On the other hand Promotion deals with 2 templates (the one promoted from and the one promoted to) and Helth usually only deals with one template.

So who should commit it?

Sidekicks (not really related to this ticket, but since it came up...):
There might be other issues with rounding but this specific issue is fixed by this patch.

IMO internal health data should always be floats and only the human readable string for the GUI should be truncated/rounded/whatever (roundup/ceil is IMO the way to go here BTW).

Personal request: @fatherbushido please write me in a PM in the forum how we communicate the health upgrade of units when advancing through the phases to the player. This is IMO an important thing. At least I like transparency much more than "herrschaftswissen" (knowledge for the sake of action or control) ^^.

In D409#31757, @FeXoR wrote:

So who should commit it?

As you want. I would have done it tommorow, but it s fine if you did it too. Don t forget to commit D798 just before.

Personal request: @fatherbushido please write me in a PM in the forum how we communicate the health upgrade of units when advancing through the phases to the player. This is IMO an important thing. At least I like transparency much more than "herrschaftswissen" (knowledge for the sake of action or control) ^^.

It s in the tooltip of the phase technology (you can also see that one in the structure tree)

FeXoR added a comment.Aug 19 2017, 4:48 AM

Thanks for the fast and pleasant feedback, @fatherbushido.
I have not looked into the other patch so please commit the two ;)

This revision was automatically updated to reflect the committed changes.
In D409#31797, @FeXoR wrote:

Thanks for the fast and pleasant feedback, @fatherbushido.
I have not looked into the other patch so please commit the two ;)

You're welcome ;-)

ps/trunk/binaries/data/mods/public/simulation/components/Promotion.js
61 ↗(On Diff #3177)

This satlin function is useless as Health.SetHitPoints already do such capping.
The change is not worth.
P46 is imo the good approach for all those things.