#1717 closed enhancement (fixed)
[PATCH] Tooltips should show the batch training cost when the hotkey is held down
Reported by: | Jonathan Waller | Owned by: | zoot |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Alpha 12 |
Component: | UI & Simulation | Keywords: | simple gui tooltip batch |
Cc: | Patch: |
Description
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.
Change History (23)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
Keywords: | review added |
---|---|
Owner: | set to |
Status: | new → assigned |
Summary: | Tooltips should show the batch training cost when the hotkey is held down → [PATCH] Tooltips should show the batch training cost when the hotkey is held down |
comment:3 by , 11 years ago
(Actually, there is a bug in that commit causing building costs to show as "NaN". I'll try to fix that tomorrow.)
comment:4 by , 11 years ago
This update fixes the "NaN" bug above: https://github.com/zootzoot/0ad/compare/5aa215edc52a1a28a6b9e45d84c81222f3f6e33c...cost-tooltip-update
comment:5 by , 11 years ago
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.
comment:6 by , 11 years ago
Where would I place the factored out function that is callable by both ProductionQueue.js and the GUI code?
follow-up: 8 comment:7 by , 11 years ago
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.
follow-up: 10 comment:8 by , 11 years ago
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.
comment:9 by , 11 years ago
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.
follow-up: 11 comment:10 by , 11 years ago
Component: | Core engine → UI & Simulation |
---|
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).
comment:11 by , 11 years ago
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?
comment:12 by , 11 years ago
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.)
comment:13 by , 11 years ago
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.
comment:14 by , 11 years ago
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.
comment:15 by , 11 years ago
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?
comment:16 by , 11 years ago
Just use Math.ceil(). Apart from that the patch is looking good, do you mind to post a diff?
comment:17 by , 11 years ago
Added Math.ceil() call. Here is the diff:
https://github.com/zootzoot/0ad/compare/master...cost-tooltip-update.diff
comment:19 by , 11 years ago
Keywords: | review removed |
---|---|
Milestone: | Backlog → Alpha 12 |
Thanks for the patch. I changed some things to look nicer and fixed a bug (not all callers set entity (getEntityCostComponentsTooltipString)).
comment:20 by , 11 years ago
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.
follow-up: 22 comment:21 by , 11 years ago
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?
comment:22 by , 11 years ago
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.
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.