Stop relying on the internal structure of the current Trader component.
ClosedPublic
Actions

Authored by • leper on Feb 24 2017, 5:01 PM.

Details

Reviewers

mimo
fatherbushido

Commits

rP19243: Stop relying on the internal structure of the current Trader component.

Summary

Get the required information by returning it in the proper places.
Also clean up some pointless modulo operations.

This is pulled out of the Silk Road code (#4314), this part is independent of everything else and related to some patch you merged/finished quite some time ago.

The remaining WIP code there goes quite some of the way for actually implementing trade routes (#1207 and #3872), but ignores the whole GUI issue. It also does not (yet?) consider routes/waypoints at all.

Test Plan

Make sure it does the same, that is open up the trade demo map and trade a bit. (Should work fine, but I haven't actually tested this diff apart from porting it over and having tested that quite some time ago.

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

• leper created this revision.Feb 24 2017, 5:01 PM

shookees added a subscriber: shookees.Feb 24 2017, 5:11 PM

shookees added inline comments.

binaries/data/mods/public/simulation/components/Trader.js
218 ↗	(On Diff #595)	why does PerformTrade return nextMarket?

• leper added inline comments.Feb 24 2017, 6:05 PM

binaries/data/mods/public/simulation/components/Trader.js
218 ↗	(On Diff #595)	Because we need to return it in order to get it in UnitAI's `PerformTradeAndMoveToNextMarket`. Sure we could add a getter, but that would just result in strange edge cases when called at some points in time.

Build is green

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

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

Harbormaster completed remote builds in B584: Diff 595.Feb 24 2017, 8:22 PM

• leper added a reviewer: mimo.Feb 24 2017, 8:58 PM

Looks good to me, and works fine.

binaries/data/mods/public/simulation/components/UnitAI.js
5362 ↗	(On Diff #595)	as PerformTrade can return INVALID_ENTITY, we should test on it here (the test on !amount is not enough as line 212 of Trader.js does not make it null).

shookees added inline comments.Feb 24 2017, 9:51 PM

binaries/data/mods/public/simulation/components/Trader.js
218 ↗	(On Diff #595)	I understand that it might look more convenient by passing through that variable, but code readability wise that's a plain wtf

Check if we don't have a valid next market.

• leper marked an inline comment as done.Feb 24 2017, 10:04 PM

• leper added inline comments.

binaries/data/mods/public/simulation/components/Trader.js
218 ↗	(On Diff #595)	I don't really share that sentiment.
binaries/data/mods/public/simulation/components/UnitAI.js
5362 ↗	(On Diff #595)	Good catch!

mimo accepted this revision.Feb 24 2017, 10:42 PM

mimo added inline comments.

binaries/data/mods/public/simulation/components/Trader.js
218 ↗	(On Diff #595)	I'm also fine with this.

This revision is now accepted and ready to land.Feb 24 2017, 10:42 PM

Closed by commit rP19243: Stop relying on the internal structure of the current Trader component. (authored by • leper). · Explain WhyFeb 24 2017, 10:59 PM

This revision was automatically updated to reflect the committed changes.

• leper marked an inline comment as done.

elexis awarded a token.Feb 24 2017, 11:04 PM

Build is green

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

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

Harbormaster completed remote builds in B589: Diff 601.Feb 24 2017, 11:39 PM