Page MenuHomeWildfire Games

Add missing round when calculating trader gain, removed in "fast-actions cheat using modifiers manager"/rP22964
ClosedPublic

Authored by Stan on Sep 25 2019, 1:16 PM.

Details

Summary

rP22964 nuked a round causing the recent bug where traders have huge values displayed.

Test Plan

Test that everything is back to normal.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/315/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/830/display/redirect

elexis added a subscriber: elexis.Sep 25 2019, 1:47 PM

(The one committing should verify that the case of 0 gain is handled correctly)

elexis retitled this revision from Add missing round removed in rP22964 to Add missing round removed in fast-actions cheat using modifiers manager rP22964.Sep 25 2019, 1:48 PM
Stan added a comment.Sep 25 2019, 2:21 PM
In D2327#97264, @elexis wrote:

(The one committing should verify that the case of 0 gain is handled correctly)

Indeed however a level 0 means the markets are too close, and if we set the gain to 1 that would possibly mean that given two very close markets one could generate a huge income quickly, not unlike dancing.
In that regard using Math.Ceil() would be bad, and Math.Floor() wouldn't change things much, so only reverting back to Math.Round() sounds sane.

wraitii retitled this revision from Add missing round removed in fast-actions cheat using modifiers manager rP22964 to Add missing round when calculating trader gain, removed in "fast-actions cheat using modifiers manager"/rP22964.Sep 29 2019, 8:35 AM
wraitii updated the Trac tickets for this revision.
elexis updated the Trac tickets for this revision.Sep 29 2019, 8:41 AM

Loot too

wraitii accepted this revision.Sep 29 2019, 9:08 AM

When trade is 0, this means it's impossible to set up trade between the two markets, without notification, which seems to be compatible with what existed before.

Thanks for the patch.

This revision is now accepted and ready to land.Sep 29 2019, 9:08 AM
This revision was automatically updated to reflect the committed changes.

! In D2327#97705, @elexis wrote:
Loot too

I definitely saw a missing-round number in the summary screen too in one svn match (refs #5605).
I see Loot.js has a Math.floor already, so it were only looted trade carts that showed non-rounded numbers in the summary screen?
Looter.js doesn't seem to have rounding

kush_hero_nastasen_1.json has

{ "value": "Looter/Resource/food", "multiply": 1.5 },

If ApplyValueModificationsToEntity doesn't round and Looter.prototype.Collect doesn't round, it sounds like it might still occur (whichever commit it was)? anyway

Stan added a comment.Sep 29 2019, 3:59 PM

Yeah was a trade cart.

In D2327#97828, @Stan wrote:

Yeah was a trade cart.

How do you know?

Stan added a comment.Sep 29 2019, 4:08 PM

Cause I tested it and that was the bug reported ?

Just to be clear, I'm talking about Loot, not trader gain.
There are more than one way to obtain Loot.
So unless you tested that all loot in the summary screen of that replay was attributed to trade carts, then you don't know.

Other than that I also spoke of the game I saw, not the game bb had uploaded, which I just uploaded at https://trac.wildfiregames.com/attachment/ticket/5605/commands.zip
But there wasnt a Kushite hero so I guess but dont actually know that it were trade carts in that replay too.

Now I've tried a replay and attacked soldiers who gathered, but didn't reproduce.
(Until I've seen the round in the code, I will have to suspect that it might be missing, regardless which commit.)

So doing one replay, I find a unit motion bug.

Doing another replay, I found summary screen still shows decimal numbers.

So I really wonder how you can say yes it was a trade cart, and yes it was tested and fixed when it's not, nor was it considered in the code review.
I mentioned it in the ticket and in the revision proposal before the commit, and after the commit.