Whenever an unit get promoted, there is always 1 hitpoint missing
Details
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 1405 Build 2213: Vulcan Build Jenkins Build 2212: arc lint + arc unit
Event Timeline
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.
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.
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.
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? |
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.
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.
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).
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.
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). |
updated to fit a22 changes.
actually this also fixes the extra healthpoint for the hellenic civic center
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.
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.
You did a grey mix of unrelated things.
That rounding in Promotion.js is not "not needed", it's just false.
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.
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) ^^.
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)
Thanks for the fast and pleasant feedback, @fatherbushido.
I have not looked into the other patch so please commit the two ;)