#23 closed enhancement (fixed)
[PATCH] Buy/Sell Resources
Reported by: | Stuart Walpole | Owned by: | |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 8 |
Component: | Core engine | Keywords: | |
Cc: | fcxSanya | Patch: |
Description (last modified by )
- 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
Attachments (27)
Change History (51)
comment:1 by , 16 years ago
Description: | modified (diff) |
---|
comment:2 by , 14 years ago
Milestone: | Diplomacy Demo |
---|
comment:3 by , 14 years ago
Milestone: | → Backlog |
---|
by , 13 years ago
Attachment: | barter_2011_08_27.diff added |
---|
by , 13 years ago
Attachment: | buy_food.png added |
---|
by , 13 years ago
Attachment: | buy_stone.png added |
---|
by , 13 years ago
Attachment: | buy_wood.png added |
---|
by , 13 years ago
Attachment: | sell_food.png added |
---|
by , 13 years ago
Attachment: | sell_stone.png added |
---|
by , 13 years ago
Attachment: | sell_wood.png added |
---|
comment:4 by , 13 years ago
Cc: | added |
---|---|
Keywords: | review added |
Summary: | Entity - Barter → [PATCH] Entity - Barter |
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?
comment:5 by , 13 years ago
Milestone: | Backlog → Alpha 8 |
---|---|
Priority: | Nice to Have → Should Have |
comment:6 by , 13 years ago
Summary: | [PATCH] Entity - Barter → [PATCH] Buy/Sell Resources |
---|---|
Type: | task → enhancement |
by , 13 years ago
Attachment: | barter_2011_08_30.diff added |
---|
comment:7 by , 13 years ago
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.
by , 13 years ago
Attachment: | barter_2011_09_12_v2.diff added |
---|
comment:8 by , 13 years ago
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.
by , 13 years ago
Attachment: | barter_2011_09_14.diff added |
---|
comment:9 by , 13 years ago
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).
by , 13 years ago
Attachment: | barter_2011_09_14_v2.diff added |
---|
by , 13 years ago
by , 13 years ago
by , 13 years ago
comment:10 by , 13 years ago
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
by , 13 years ago
Attachment: | food_inactive.png added |
---|
by , 13 years ago
Attachment: | food_selected.png added |
---|
by , 13 years ago
Attachment: | metal_inactive.png added |
---|
by , 13 years ago
Attachment: | metal_selected.png added |
---|
by , 13 years ago
Attachment: | stone_inactive.png added |
---|
by , 13 years ago
Attachment: | stone_selected.png added |
---|
by , 13 years ago
Attachment: | wood_inactive.png added |
---|
by , 13 years ago
Attachment: | wood_selected.png added |
---|
comment:11 by , 13 years ago
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.
by , 13 years ago
Attachment: | barter_2011_09_20_v2.diff added |
---|
comment:12 by , 13 years ago
Removed unused variable (thanks to quantumstate :) ), reuploaded patch with same name.
by , 13 years ago
Attachment: | barter_2011_09_22.diff added |
---|
comment:13 by , 13 years ago
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.
follow-up: 15 comment:14 by , 13 years ago
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.
by , 13 years ago
Attachment: | barter_2011_10_02.diff added |
---|
comment:15 by , 13 years ago
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
, sinceTrySubtractResources
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?
by , 13 years ago
Attachment: | barter_2011_10_17.diff added |
---|
comment:17 by , 12 years ago
Priority: | Should Have → Must Have |
---|
follow-up: 19 comment:18 by , 12 years ago
(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_COMPONENT
s 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 callssetupUnitBarterPanel
and returns. Can you just callsetupUnitBarterPanel
directly? - "
var selectedResourceIndex = [g_barterSell, g_barterBuy][j];
" looks a bit odd. You're looping overBARTER_ACTIONS.length
, seemingly to keep it flexible and to avoid hard-coding the value 2, but then that line implicitly assumesj < 2
. I think it might be clearer if it simply looped with "for (var j = 0; j < 2; j++)
" to avoid giving the impression thatj
could potentially be larger. - "
button.onpress = (function(e){ return function() { selectBarterResourceToSell(e); } })(i);
" - whye
? (I'd tend to call the argumenti
, 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 havevar
.
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 ofSetTimeout
, then it'll automatically repeat at the correct rate, and callCancelTimer
if!needRestore
to stop it.
by , 12 years ago
Attachment: | barter_2011_11_23.diff added |
---|
comment:19 by , 12 years ago
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 callssetupUnitBarterPanel
and returns. Can you just callsetupUnitBarterPanel
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);
" - whye
? (I'd tend to call the argumenti
, 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.
comment:20 by , 12 years ago
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 :-)
comment:21 by , 12 years ago
@ 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.
comment:22 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:23 by , 8 years ago
Keywords: | review removed |
---|
Milestone Diplomacy Demo deleted