[PATCH] Tooltips should show the batch training cost when the hotkey is held down

[Jonathan Waller]

It would be nice if when the hotkey for batch training is held down the tooltip would update to show the cost of training the batch of units. This would make the decreased training time more visible.

[zoot]

I have a basic implementation of this here: ​https://github.com/zootzoot/0ad/commit/ac9b379854310f923abdfcad45c141def9494479

The caveat is that it does not currently show the reduced training time for batches, because I wasn't certain what would be the best way to do that.

[zoot]

(Actually, there is a bug in that commit causing building costs to show as "NaN". I'll try to fix that tomorrow.)

[zoot]

This update fixes the "NaN" bug above: ​https://github.com/zootzoot/0ad/compare/5aa215edc52a1a28a6b9e45d84c81222f3f6e33c...cost-tooltip-update

[Jonathan Waller]

The total training time for the batch should be shown. ProductionQueue.js:188 is where the time is calculated. This should be factored out into a reusable function that is called by both bits of code.

[zoot]

Where would I place the factored out function that is callable by both ProductionQueue.js and the GUI code?

[leper]

I'd add it to ProductionQueue.js and add it to the values passed to the gui in GUIInterface.js.

That way we could even define the time factor in the xml files in the ProductionQueue tag.

[zoot]

Replying to leper:

I'd add it to ProductionQueue .js and add it to the values passed to the gui in GUIInterface.js. That way we could even define the time factor in the xml files in the ProductionQueue tag.

Can you be more specific? I've had a deep look and I can't immediately find a good spot to add it. The values passed to the GUI are defined at GuiInterface.js:205.

[Jonathan Waller]

I'm not sure that adding it to ProductionQueue.js is a neat solution, it means passing more current entity stuff around. public/globalscripts would be my suggestion (a comment in ProductionQueue and the tooltip code should mention that). Adding some factor to the templates would be equally easy with this approach, but that is beyond the scope of this ticket.

[leper]

Replying to zoot:

Can you be more specific? I've had a deep look and I can't immediately find a good spot to add it. The values passed to the GUI are defined at GuiInterface.js:205.

(The result of some irc discussion)

Add a GetBatchTimeFactor (or something like that, there is probably a better name for it) to ProductionQueue that for the moment returns 0.7 and use the function instead of the 0.7 at line 188. In GuiInterface add this value to ret.production and use that in the GUI.

The new ProductionQueue function could even be extended (though I'd say that this is out of the scope of this ticket, but it wouldn't harm to do it) to read that value from the template and/or be modifiable by technologies (e.g. ProductionQueue/BatchTime).

[zoot]

Replying to leper:

(The result of some irc discussion) Add a GetBatchTimeFactor (or something like that, there is probably a better name for it) to ProductionQueue that for the moment returns 0.7 and use the function instead of the 0.7 at line 188. In GuiInterface add this value to ret.production and use that in the GUI.

That's a lot like what I tried initially:

​https://github.com/zootzoot/0ad/commit/0c281e69d9531751d667e347ef51c074b909999f

My problem is that I don't see a good way to pass the ret.production.batchtimefactor value into setupUnitPanel() in gui/session/unit_commands.js. Just slapping on a new batchtimefactor parameter on setupUnitPanel's signature seems nasty.

Should I do that or is there a better way?

[leper]

You already have entState in setupUnitPanel right?

The cost (and some other informations) are retrieved by using GetEntityCostTooltip (utility_functions.js) which can be extended to get another parameter that can be retrieved by modifying/extending GetTrainingBatchStatus (input.js).

I'm not really sure how to display the time factor for buildings with different time modifiers though, but as that isn't present yet we can just ignore it.

And the commit you linked above changes the current time modifier. (You could still choose to add the count as a parameter to GetBatchTimeModifier and export that through GuiInterface instead of adding it as a value.)

[zoot]

Not having much luck :) Tried making a function in ProductionQueue.js and passing a reference to it to getEntityCostTooltip():

​https://github.com/zootzoot/0ad/compare/zootzoot:master...zootzoot:cost-tooltip-update-x1

But it gives me the error:

ERROR: JavaScript error: simulation/components/ProductionQueue.js line 403
TypeError: cmpPlayer is null
  (10)@simulation/components/ProductionQueue.js:403
  (1,150)@simulation/components/GuiInterface.js:210
  (1,"GetEntityState",150)@simulation/components/GuiInterface.js:1703

Will experiment some more tomorrow.

[leper]

Take a look at CanAttack() or GetTradingRouteGain() in GuiInterface.js and don't pass the function in ret.production (as the cmpPlayer will be invalidated after leaving GetTemplateData().

Or you could just pass the 0.7 in ret.production and duplicate the calculation formula on the gui side, but I'd prefer the first solution.

[zoot]

This basically works:

​https://github.com/zootzoot/0ad/compare/master...cost-tooltip-update-x2

The GUI code properly calls the function in ProductionQueue.js.

The caveat now is that the build time shown in the tooltip is often a rather nasty looking floating point number. Any advice on how to round or truncate it?

[leper]

Just use Math.ceil(). Apart from that the patch is looking good, do you mind to post a diff?

[zoot]

Added Math.ceil() call. Here is the diff:

​https://github.com/zootzoot/0ad/compare/master...cost-tooltip-update.diff

[leper]

In 12876:

Show cost in tooltips for batches. Closes #1717, patch by zoot.

[leper]

Thanks for the patch. I changed some things to look nicer and fixed a bug (not all callers set entity (getEntityCostComponentsTooltipString)).

[zoot]

Regarding r12877: Technically, the "0.7" value is an exponent, not a factor. Some might read it and mistakenly think there is multiplication going on, when it is really exponentiation that happens.

[leper]

I can't prove you wrong on this, but I'm pretty sure that you have seen the TODO something a bit more neutral would probably be best (though replacing it later is just a matter of running a script).

How about BatchTimeModifier?

[zoot]

Replying to leper:

I can't prove you wrong on this, but I'm pretty sure that you have seen the TODO something a bit more neutral would probably be best (though replacing it later is just a matter of running a script). How about BatchTimeModifier ?

I agree, BatchTimeModifier should work.

[leper]

In 12883:

Rename property. Refs #1717.