[PATCH] Buy/Sell Resources

[stuart]

• Resources can be traded for other resources at the Market from the Barter tab.
• (This is mostly an interface task, since it largely involves pressing buttons to juggle the player's resource values, without need for special events.)
• XML.Entity.Actions.Barter

[anonymous]

Milestone Diplomacy Demo deleted

[fcxSanya]

I attached patch and 6 icons for simple form of bartering. For market (or any other building with BarterMarket class) will be displayed additional row of buttons on "Training" panel: buy/sell food/wood/stone for metal. Currently price is fixed (125 metal to buy 100 units of any resource, 75 metal for selling 100 units of any resource).

If we will apply this patch, it will be good if someone more skilled in image editing will redraw these 6 icons. Or maybe we already have good trading icons somewhere?

[fcxSanya]

New patch attached - now dynamic prices. Now each resource (food/wood/stone) have "true price" - price of 100 resource in metal. There is again constant difference between buy/sell price, currently I set it to +-25%, so you will buy resource for 125% of it's "true price" and sell for 75%. Each time when you buy or sell resource it's price correspondingly rise/fall. If price is not standard it slowly move to standard value.

All numeric constants (true prices, constant difference, per deal additional difference, price restore timer interval and restore value) can be adjusted at the top of binaries/data/mods/public/simulation/components/PlayerBarter.js file.

[fcxSanya]

Attached slightly updated version of patch. There is no logic changes. It just should be compatible with current revision (r10257), also I removed formatting changes in other part of code (there was few in previous version), because they increase risk of merging necessity with new revisions, also changed comment in PlayerBarter.js to more clear one.

[fcxSanya]

New version of patch: now prices are global and so price changes affect all players. Also barter buttons moved to the left panel (where garrison/formations placed for garrison holders/units respectively).

[fcxSanya]

In this version prices also displayed over buttons (not only in tooltip as before). Only food.png, wood.png, stone.png icons used, instead of old buy_*.png, sell_*.png

[fcxSanya]

Now this ticket and trac log in general full of my high-artistic images :D

Attached new version of patch. Now there is 'true bartering' system: each resource can be exchanged to any other directly.

[fcxSanya]

Removed unused variable (thanks to quantumstate :) ), reuploaded patch with same name.

[fcxSanya]

New version, fixed bug: bartering was allowed as soon as foundation of market was placed, now I added check in GuiInterface so bartering is not available for foundation.

[historic_bruno]

Some comments from reviewing the patch (not play tested yet):

Why is the component named GlobalBarter? If there's only one barter component, it should be called Barter :)

unit_commands.js:121, (g_barterBuy + 1)%4 would be the same since 5%4 = 1.

unit_commands.js:342, Maybe selling increments (100/500) should be defined as constants somewhere?

unit_commands.js:342,351,352, The modulus operators seem unnecessary since i < 4 and j < 2.

GuiInterface.js:255, Could use cmpIdentity.HasClass() in place of indexOf().

GuiInterface.js:257-259: For consistency it should be: ret.barterMarket = { "prices": ... }; instead of defining a separate boolean value (JS treats an undefined property like "false" anyway).

I think interfaces/GlobalBarter.js shouldn't be included in the patch.

GlobalBarter.js:2 typo "relarive" > relative

GlobalBarter.js:51: you don't need to define the resources array in amountsToSubtract, since TrySubtractResources will only check defined values.

GlobalBarter.js:32,40,88, it seems a bit messy to keep defining the same constant arrays, so they should either be a constant defined once, or maybe use something like this when possible: for (var in this.priceDifferences), which will get the resource names defined in the array (not the values).

I don't quite understand the reason for the ProgressTimeout.

[fcxSanya]

Replying to historic_bruno:

Thanks for review, Ben :)

Why is the component named GlobalBarter? If there's only one barter component, it should be called Barter :)

There was also PlayerBarter component in previous version of patch and it is possible that each market can have it's own MarketBarter component, it is why it called GlobalBarter. But since it looks like we don't going to change barter design again, I renamed it to Barter now.

unit_commands.js:121, (g_barterBuy + 1)%4 would be the same since 5%4 = 1.

Fixed.

unit_commands.js:342, Maybe selling increments (100/500) should be defined as constants somewhere?

I defined constants at top of file now.

unit_commands.js:342,351,352, The modulus operators seem unnecessary since i < 4 and j < 2.

Another thing from old version of patch, which I don't noticed. Removed now.

GuiInterface.js:255, Could use cmpIdentity.HasClass() in place of indexOf().

Changed.

GuiInterface.js:257-259: For consistency it should be: ret.barterMarket = { "prices": ... }; instead of defining a separate boolean value (JS treats an undefined property like "false" anyway).

Changed.

I think interfaces/GlobalBarter.js shouldn't be included in the patch.

It should, there is one line, which is don't displayed by Trac web viewer by some reason.

GlobalBarter.js:2 typo "relarive" > relative

Fixed.

GlobalBarter.js:51: you don't need to define the resources array in amountsToSubtract, since TrySubtractResources will only check defined values.

Changed.

GlobalBarter.js:32,40,88, it seems a bit messy to keep defining the same constant arrays, so they should either be a constant defined once, or maybe use something like this when possible: for (var in this.priceDifferences), which will get the resource names defined in the array (not the values).

Defined constant.

I don't quite understand the reason for the ProgressTimeout.

This is function, which is called by timer to slowly restore prices after deals. What is wrong with it?

[fcxSanya]

New version of patch - refactored changes in unit_commands.js.

[Philip]

(Haven't tried actually running the code, just read over the patch. Also I don't have any opinion on the gameplay design.)

Just some minor comments:

Simulation2.cpp

• The LOAD_SCRIPTED_COMPONENTs should be in alphabetical order, because that's very slightly less arbitrary than any other order.

session.xml

• Is it really necessary to specify z values for the button objects, instead of the default ordering?

input.js

• Inconsistent spacing (none after the {, but one before the }). (Probably get rid of the latter space.)

unit_commands.js

• It seems convoluted to call setupUnitPanel("Barter", ...) which then immediately calls setupUnitBarterPanel and returns. Can you just call setupUnitBarterPanel directly?
• "var selectedResourceIndex = [g_barterSell, g_barterBuy][j];" looks a bit odd. You're looping over BARTER_ACTIONS.length, seemingly to keep it flexible and to avoid hard-coding the value 2, but then that line implicitly assumes j < 2. I think it might be clearer if it simply looped with "for (var j = 0; j < 2; j++)" to avoid giving the impression that j could potentially be larger.
• "button.onpress = (function(e){ return function() { selectBarterResourceToSell(e); } })(i);" - why e? (I'd tend to call the argument i, to match the name of the variable that's bound to that argument, but I don't know if other people find that bizarre or confusing.)
• "amount = amountToBuy" - missing semicolon.
• "performDealButton = getGUIObjectByName("PerformDealButton");" - should have var.

Barter.js

• Should do some verification of the data (or at least have a TODO about it), to stop players cheating by e.g. hacking the GUI scripts to let them sell -10000 gold and thus end up with 10000 more gold that they started with. So amount must be non-negative, and the resource names must be recognised ones, and the player must have at least one building with BarterMarket.
• If the turn length is not a factor of 5000ms, the timer will not trigger at an average of every 5000ms. (E.g. if the turn length was 499ms, it would trigger after 5489ms and then every 5489ms after that). It would probably be nice to avoid that problem. Use cmpTimer.SetInterval instead of SetTimeout, then it'll automatically repeat at the correct rate, and call CancelTimer if !needRestore to stop it.

[fcxSanya]

Replying to Philip:

Thanks for the review.

Is it really necessary to specify z values for the button objects, instead of the default ordering?

I just copied some block with the same layout as I needed and renamed elements :)

It seems convoluted to call setupUnitPanel("Barter", ...) which then immediately calls setupUnitBarterPanel and returns. Can you just call setupUnitBarterPanel directly?

I considered that it is better for the consistency to always call setupUnitPanel which will call nested functions when needed. Also I supposed that there will be more shared logic between setupUnitPanel and setupUnitBarterPanel, but while I modified these functions the shared part reduced to the sole line usedPanels[guiName] = 1;

"button.onpress = (function(e){ return function() { selectBarterResourceToSell(e); } })(i);" - why e? (I'd tend to call the argument i, to match the name of the variable that's bound to that argument, but I don't know if other people find that bizarre or confusing.)

The same situation as with the 'z' values: there is 'e' used in the existing code:

// Button Function (need nested functions to get the closure right)
button.onpress = (function(e){ return function() { callback(e) } })(item);

So I used the same name.

All should be fixed in the current version of the patch.

[Philip]

performDealButton.onpress = (function(exchangeResourcesParameters){ return function() { exchangeResources(exchangeResourcesParameters) } })(exchangeResourcesParameters);

That one's not necessary, you can just do

performDealButton.onpress = function() { exchangeResources(exchangeResourcesParameters) };

(The nested-function trick is generally only needed when you're creating a function() closure inside a loop (because the closure stores a reference to each variable it uses, not a copy of the variable's current value, and the variable exists until the end of the function it's declared in; and the next iteration of the loop will change that variable's value, so all the closures will end up seeing the value from the final iteration), and this isn't one of those cases.)

In Barter.js: I think you need to set this.restoreTimer = undefined after CancelTimer, else the timer will never get set up a second time.

Otherwise it all looks good to me :-)

[k776]

@ fcxSanya Once you've applied Philip's final comments, and tested the system works through and through, please feel free to commit this. Thanks for your work on this.

[fcxSanya]

(In [10588]) Barter. Closes #23.