Opened 18 years ago

Closed 12 years ago

Last modified 8 years ago

#30 closed enhancement (fixed)

[PATCH] Trade

Reported by: Stuart Walpole Owned by: fcxSanya
Priority: Must Have Milestone: Alpha 9
Component: UI & Simulation Keywords:
Cc: Patch:

Description (last modified by Jan Wassenberg)

Attachments (7)

trading_2011_10_22.diff (25.9 KB ) - added by fcxSanya 12 years ago.
trading_2012_02_06.diff (32.7 KB ) - added by fcxSanya 12 years ago.
action-setup-trade-route.png (2.9 KB ) - added by fcxSanya 12 years ago.
trading_2012_02_24.diff (35.0 KB ) - added by fcxSanya 12 years ago.
trading_2012_03_06.diff (36.0 KB ) - added by fcxSanya 12 years ago.
trading_2012_03_08.diff (35.5 KB ) - added by fcxSanya 12 years ago.
trading_2012_03_08_v2.diff (35.5 KB ) - added by fcxSanya 12 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 by Jan Wassenberg, 16 years ago

Description: modified (diff)

comment:2 by (none), 13 years ago

Milestone: Diplomacy Demo

Milestone Diplomacy Demo deleted

comment:3 by Andrew, 13 years ago

Milestone: Backlog

comment:4 by fcxSanya, 12 years ago

Component: Game engineSimulation
Owner: set to fcxSanya
Status: newassigned
Summary: Entity - Event - TradeTrade

by fcxSanya, 12 years ago

Attachment: trading_2011_10_22.diff added

comment:5 by fcxSanya, 12 years ago

I still work on this, attached patch just to show/fix current state.

comment:6 by fcxSanya, 12 years ago

About binary files needed for patch: I used the same binaries/data/mods/public/art/textures/ui/session/resources/*.png files as I attached to #23 (*_selected.png and *_inactive.png to be more precise). binaries/data/mods/public/art/textures/cursors/action-setup-trade-route.png is copy of binaries/data/mods/public/art/textures/cursors/action-gather-treasure.png

Another note: related discussion of forum: http://www.wildfiregames.com/forum/index.php?showtopic=15258 .

comment:7 by Kieran P, 12 years ago

Milestone: BacklogAlpha 8
Priority: Should HaveMust Have
Summary: Trade[PATCH] Trade
Type: taskenhancement

comment:8 by historic_bruno, 12 years ago

Milestone: Alpha 8Alpha 9

by fcxSanya, 12 years ago

Attachment: trading_2012_02_06.diff added

comment:9 by fcxSanya, 12 years ago

Keywords: review added

Finally I have something which I hope ready to be reviewed.

Some notes about tooltips, like a log what I tried to do and why finally did what I did, may be tedious and unnecessary to read even for patch reviewers:

As I said/asked/discussed previously in IRC/forum I discovered that I need to modify the engine code to have a 'game world' tooltip, since existing tooltips was bound to gui elements so it was impossible to place such tooltip on a gui page and manipulate it. On the other hand we have a 'text' control, which can be placed on a page and accessed from a code, but it lacks tooltip's autopositioning/autosizing features. So I was need to either modify tooltips or text controls. Firstly I found that tooltips have some shared state tracking/show delay logic to allow features like one, described in the comment in GUITooltip.cpp:

<...> If the mouse re-enters an object within a short time, the new
tooltip is displayed immediately. (This lets you run the mouse across a
series of buttons, without waiting ages for the text to pop up every time.)

Since such functionality is not needed for the 'game world' tooltip and even can disturb somehow as I thought, I supposed to investigate that maybe it is easily to add an additional property to the 'text' control like 'bool tooltiplike' and if it is 'tooltiplike' do autopositioning/autosizing, but then I found out that text this code is spread across CTooltip code (CToolip.cpp) and heavily different from code in the 'text' control (CText.cpp), also I found out that indeed CTooltip contains text positioning and tooltip drawing code and GUITooltip is tooltips manager which keeps track on tooltip state and show/hide concrete tooltips (CTooltip instances). So at this point I decided that I need another instance of CTooltip which will be available from gui scripts and will not be managed by GUITooltip. I conjectured that there is two alternatives:

  • more simple - expose CTooltip to gui scripts as other gui elements are exposed - via AddObjectType("tooltip", &CTooltip::ConstructObject); in CGUI::Initialize();
  • more sophisticated - add single instance of CTooltip to CGUI class (which represents a gui page) and expose it via some accessor function;

I started from second approach:

  • added instance of CTooltip to the CGUI:
    // Tooltip for game world objects (for buildings info etc.)
    CTooltip* m_GameWorldTooltip;
    
  • exposed it in the CGUI:
    IGUIObject* CGUI::GetGameWorldTooltip() const
    {
    	return m_GameWorldTooltip;
    }
    
  • exposed it in the CGUIManager (which is manager for gui pages):
    IGUIObject* CGUIManager::GetGameWorldTooltip() const
    {
    	if (m_CurrentGUI)
    		return m_CurrentGUI->GetGameWorldTooltip();
    	else
    		return top()->GetGameWorldTooltip();
    }
    
  • exposed it in the ScriptGlue.cpp:
    JSBool GetGameWorldTooltip(JSContext* cx, uintN argc, jsval* vp)
    {
    	JSU_REQUIRE_NO_PARAMS();
    
    	try
    	{
    		IGUIObject* gameWorldTooltip = g_GUI->GetGameWorldTooltip();
    		JS_SET_RVAL(cx, vp, OBJECT_TO_JSVAL(gameWorldTooltip->GetJSObject()));
    
    		return JS_TRUE;
    	}
    	catch (PSERROR_Scripting&)
    	{
    		return JS_FALSE;
    	}
    }
    
    JS_FUNC("getGameWorldTooltip", GetGameWorldTooltip, 0)
    

And this part worked, but then I discovered that in CGUI there is too many manipulations with collection of other gui elements and I need to figure out how to add my tooltip to it or to do something different to achieve special event handling for it. At this point I looked on first alternative of exposing CTooltip to gui scripts and found it fairly simple:

  • as I wrote above: add AddObjectType("tooltip", &CTooltip::ConstructObject); into CGUI::Initialize();
  • insert tooltip on a gui page in scripts: <object name="informationTooltip" type="tooltip" style="informationTooltip" />
  • and implement the mouse motion handling for it:
     void CTooltip::HandleMessage(SGUIMessage &Message)
     {
    +	if (Message.type == GUIM_MOUSE_MOTION)
    +		if (GUI<CPos>::SetSetting(this, "_mousepos", GetMousePos()) != PSRETURN_OK)
    +			debug_warn(L"Failed to set tooltip mouse position");
     	IGUITextOwner::HandleMessage(Message);
     }
    

So I stopped on this approach. It has defect that it is technically possible to add multiple 'game world' tooltips on the same gui page and they will probably just overlap, but it is obliviously enough that we should not do this.

by fcxSanya, 12 years ago

comment:10 by Jonathan Waller, 12 years ago

I have been looking at the javascript code and have a few comments. I haven't gone through the GUI stuff properly yet, but I am pretty busy so it might be a few days before i have a chance.

I work on the assumption that any code outside of the components system should be treated as hostile so validation checks should be performed.

In Identity.js classes no longer need to be defined

UnitAI.prototype.CanTrade = function(target) - Here target is an unused parameter, this is misleading, either the code should check if the target can be traded with or remove the parameter. I think the former would be better.

UnitAI.prototype.PerformTradeAndMoveToNextMarket() line 2454 comment should read "Market wasn't reached, give up"

Trader.js

CalculateGain should probably check that the Markets have positions, in case it is passed bad parameters (it is called from GUIInterface which doesn't perform a validity check either).

In SetTargetMarket you put comments before the else if's I think it would be clearer to move them inside the curly braces.

SetTargetMarket should verify that the target is actually a building which can perform trade.

comment:11 by Jonathan Waller, 12 years ago

I have taken a look through the rest of the code. There was only one thing I noticed.

The SetPreferredGoods function doesn't validate the input so random junk could be sent from the network code which would cause undefined behavior. It should test against food, wood...

by fcxSanya, 12 years ago

Attachment: trading_2012_02_24.diff added

in reply to:  11 comment:12 by fcxSanya, 12 years ago

Replying to comments 10 and 11 by quantumstate:

Thanks again for the review :) I updated code according to your notes and added the gain into the tooltip as we discussed on IRC.

Last edited 12 years ago by fcxSanya (previous) (diff)

comment:13 by historic_bruno, 12 years ago

Did some play testing with your latest patch, it seems to work very well. There were only a few things I found worth mentioning:

  • There's a graphical glitch in the tooltips: they seem to appear in the previous position before being moved. It should move first (while hidden) then appear. Otherwise they look good, very clear and not distracting to gameplay.
  • I like how the trader stops after his current destination, when the next market on the route is destroyed. Would it be possible to also reset the state of the trade tooltip, or is there a good reason not to?

Code comments:

  • I noticed you added a "SeaMarket" class to the dock templates, I'm not a big fan of classes like that, wouldn't it work better if docks had "Market" and "Dock" classes, and you tested for both as needed? :)
  • UnitAI.CanTrade() should check the target market's owner is diplomatically friendly with the trader. And it should be checked again before using UnitAI.PerformTrade() in case diplomacy changes or something unexpected happens (the GUI will check the initial diplomacy but we also like to check in the simulation).
Last edited 12 years ago by historic_bruno (previous) (diff)

in reply to:  13 ; comment:14 by fcxSanya, 12 years ago

Replying to historic_bruno:

Thanks for the review :)

  • There's a graphical glitch in the tooltips: they seem to appear in the previous position before being moved. It should move first (while hidden) then appear.

I started to type answer, then decided to ask Philip on IRC, he is not answered yet, so for now I will paste what I typed there (see the "edit" below):

<...>the tooltip is displayed/hided in the gui scripts and position changed in CTooltip.cpp HandleMessage on GUIM_MOUSE_MOTION event; as far as I understand this happens because this event passed from CGUI only to not hidden controls: GUI<SGUIMessage>::RecurseObject(GUIRR_HIDDEN | GUIRR_GHOST, m_BaseObject, &IGUIObject::HandleMessage, msg); I want to pass this event also to my tooltip independently of it's visibility in less ugly way; I think less ugly solution is to add some property to CTooltip like "independent" (as opposed to tooltips managed by GUITooltip) and additional restriction to RecurseObject like GUIR_INDEPENDENT_TOOLTIP and add an additional call to RecurseObject with this restriction beside the original one

<Edit>

Philip's answer:

<Philip`>: fcxSanya: Maybe the tooltip can grab the current mouse position from somewhere when it's first enabled, rather than it using the MOTION event for that?

<fcxSanya>: Philip`, in gui scripts do you mean?

<Philip`>: fcxSanya: In CTooltip

<fcxSanya>: hm, I will look

</Edit>

Also I noticed that GUIM_MOUSE_MOTION passed to all tooltips (including these, which managed by the GUITooltip), so I need to add a "independent" property anyway and check it in the CTooltip's HandleMessage function.

I stuck on the first item, so did not implemented other changes yet, but will answer what I think:

  • I like how the trader stops after his current destination, when the next market on the route is destroyed. Would it be possible to also reset the state of the trade tooltip, or is there a good reason not to?

Sounds like it would be good to do, I will look at this.

  • I noticed you added a "SeaMarket" class to the dock templates, I'm not a big fan of classes like that, wouldn't it work better if docks had "Market" and "Dock" classes, and you tested for both as needed? :)

My idea is that they have different meanings: "Market" is where land units can trade and "SeaMarket" is correspondingly for naval units. Currently you can see that the dock has both classes so both land and naval units can trade there (i.e. land trader can trade between market and dock or between two docks), using templates we can allow this behaviour only to specified civs. I don't know whether this feature makes any sense, so I can cut it and leave only one 'Market' class as you propose if we don't need it.

  • UnitAI.CanTrade() should check the target market's owner is diplomatically friendly with the trader. And it should be checked again before using UnitAI.PerformTrade() in case diplomacy changes or something unexpected happens (the GUI will check the initial diplomacy but we also like to check in the simulation).

I will do.

Last edited 12 years ago by fcxSanya (previous) (diff)

by fcxSanya, 12 years ago

Attachment: trading_2012_03_06.diff added

in reply to:  14 comment:15 by fcxSanya, 12 years ago

Replying to fcxSanya:

<Philip`>: fcxSanya: Maybe the tooltip can grab the current mouse position from somewhere when it's first enabled, rather than it using the MOTION event for that?

Also I noticed that GUIM_MOUSE_MOTION passed to all tooltips (including these, which managed by the GUITooltip), so I need to add a "independent" property anyway and check it in the CTooltip's HandleMessage function.

I added the "independent" property, removed GUIM_MOUSE_MOTION processing and changed CTooltip::SetupText() to use GetMousePos() rather than "_mousepos" setting. Now I don't understand why we need "_mousepos" for regular tooltips, they looks working fine with GetMousePos() too, but I kept the old logic for them.

  • I like how the trader stops after his current destination, when the next market on the route is destroyed. Would it be possible to also reset the state of the trade tooltip, or is there a good reason not to?

Sounds like it would be good to do, I will look at this.

Now trader should reset his markets when it can't trade between his markets.

  • I noticed you added a "SeaMarket" class to the dock templates, I'm not a big fan of classes like that, wouldn't it work better if docks had "Market" and "Dock" classes, and you tested for both as needed? :)

My idea is that they have different meanings: "Market" is where land units can trade and "SeaMarket" is correspondingly for naval units. Currently you can see that the dock has both classes so both land and naval units can trade there (i.e. land trader can trade between market and dock or between two docks), using templates we can allow this behaviour only to specified civs. I don't know whether this feature makes any sense, so I can cut it and leave only one 'Market' class as you propose if we don't need it.

For now I kept it as is.

  • UnitAI.CanTrade() should check the target market's owner is diplomatically friendly with the trader. And it should be checked again before using UnitAI.PerformTrade() in case diplomacy changes or something unexpected happens (the GUI will check the initial diplomacy but we also like to check in the simulation).

I will do.

I created Trader.CanTrade function and now it called in either UnitAI.CanTrade and GuiInterface.GetTradingDetails. UnitAI.PerformTradeAndMoveToNextMarket checks ownership before calling UnitAI.PerformTrade.

Edit: looks like binaries/data/mods/public/art/textures/cursors/action-setup-trade-route.txt duplicated it's content (this happens after reverting and applying patches again), I will fix this later.

Last edited 12 years ago by fcxSanya (previous) (diff)

comment:16 by historic_bruno, 12 years ago

Could you replace the UnitAI diplomacy check in PerformTradeAndMoveToNextMarket with this.CanTrade? That way if we decide we want e.g. gaia markets to allow trade, it's a simple fix.

by fcxSanya, 12 years ago

Attachment: trading_2012_03_08.diff added

in reply to:  16 comment:17 by fcxSanya, 12 years ago

Replying to historic_bruno:

Could you replace the UnitAI diplomacy check in PerformTradeAndMoveToNextMarket with this.CanTrade? That way if we decide we want e.g. gaia markets to allow trade, it's a simple fix.

Done :)

by fcxSanya, 12 years ago

Attachment: trading_2012_03_08_v2.diff added

comment:18 by fcxSanya, 12 years ago

Latest attached version of the patch (trading_2012_03_08_v2.diff​) contains javascript code-style changes, pointed out by Ben on IRC:

06:03 < historicbruno> fcxSanya: in Trader.js line 144, you do a comparison with null, it's probably best to just do: return (this.firstMarket && this.secondMarket);

06:04 < historicbruno> I added something about this to the Coding Conventions recently: http://trac.wildfiregames.com/wiki/Coding_Conventions#JavaScript

06:06 < fcxSanya> historicbruno, what about brackets, it is conventional to use them in such cases?

06:07 < historicbruno> also in GuiInterface lines 718 and 722, similar thing

06:09 < historicbruno> fcxSanya: I've seen it both ways, I guess for a simple && you could leave off the parentheses

06:13 < fcxSanya> historicbruno, I will fix this, then check coding conversions vs my patch and then commit

06:14 < historicbruno> I think we rarely use "null" in scripts

06:14 < fcxSanya> historicbruno, better to use "undefined"?

06:16 < historicbruno> maybe not in this case

06:16 < fcxSanya> e.g. in Trader.Init I want to declare that it have 'firstMarket' so I'm writing 'this.firstMarket = null'

06:16 < fcxSanya> 'this.firstMarket = undefined;' probably looks a bit weird :)

06:20 < historicbruno> fcxSanya: also, I think lines 700 and 710 of GuiInterface will be true even if data.target is undefined (which seems possible if someone calls it and forgets to set that property)

06:23 < historicbruno> === is probably better than == in that case, because of the weird type conversion that == does

06:35 < fcxSanya> "<historicbruno> fcxSanya: also, I think lines 700 and 710 of GuiInterface will be true even if data.target is undefined (which seems possible if someone calls it and forgets to set that property)" this function don't make any sense if data.target is undefined :) and cmpEntityTrader.CanTrade(data.target) should return false in this case (due to "if (!cmpTargetIdentity)" check)

06:36 < fcxSanya> but I will change == to === just for the clarity sake

I kept "null" as is for now.

comment:19 by fcxSanya, 12 years ago

Resolution: fixed
Status: assignedclosed

In 11279:

Trade. Closes #30.

comment:20 by sanderd17, 8 years ago

Keywords: review removed
Note: See TracTickets for help on using tickets.