Opened 10 years ago
Last modified 5 years ago
#2357 new enhancement
[PATCH] Notify user when workers are out-of-work
Reported by: | sanderd17 | Owned by: | Josh |
---|---|---|---|
Priority: | Should Have | Milestone: | Backlog |
Component: | UI – In-game | Keywords: | patch |
Cc: | Patch: |
Description
Idle workers can be found through the button right next to the minimap, but it would be nice if the button could show if there are idle units without clicking on it.
The blinking shouldn't be too obtrusive, as it will happen often in late game.
Attachments (20)
Change History (63)
comment:1 by , 10 years ago
Owner: | set to |
---|
by , 10 years ago
Attachment: | idle_worker_blink.patch added |
---|
comment:2 by , 10 years ago
I attached a patch which make the button blink slowly if there are some idle workers.
TODO:
- make it work when a save game is loaded (calculate the idle counter for all players)
Another thing which I don't like is how the idle counter comes from the UnitAI class (getIdleCount(player)) to session.js. Can anyone give me a hint how this can be made without using such a dirty hack (getNumberOfIdleUnits in GuiInterface.js:1712)?
One option would be to call FindIdleUnits() all the time but I think this would be bad for performance and that's why I tried to find another way.
comment:3 by , 10 years ago
Keywords: | patch review hint added |
---|---|
Summary: | Make the "idle workers" button blink or change colour → [PATCH] Make the "idle workers" button blink or change colour |
comment:4 by , 10 years ago
Milestone: | Backlog → Alpha 16 |
---|
comment:5 by , 10 years ago
Keywords: | review removed |
---|
comment:6 by , 10 years ago
I got some feedback from leper and sanderd17 in the IRC and will improve the patch tomorrow.
comment:7 by , 10 years ago
Keywords: | review added; hint removed |
---|
comment:8 by , 10 years ago
Now it is working. The patch also contains some improvments of the color fade functions (easier to work with).
PS: It's not compatible to savegames which were created without this patch.
by , 10 years ago
Attachment: | idle_worker_blink_v2.patch added |
---|
Made the code better. Thx to leper and sanderd17 for hints.
follow-up: 10 comment:9 by , 10 years ago
Could you also display the number of idle units in the tooltip when blinking the button? This wasn't asked for, but it looks like a simple addition, as you already keep track of the number of idle units anyway.
For the code:
- Session.js line 502 doesnt need to query the player ID. The first argument given to any GuiInterface method is the player id anyway. So that's just a lost call.
- UnitAI shouldn't have to worry about counters too much. It's nice if it can notify the cmpPlayer when the state of a unit changed. But cmpPlayer should handle ownership changes on its own. This would make the code a lot more simple, as it only needs to work in the idle case (probably both regular and formation idle), and only ever needs to query the own player component. Having a function to query the cmpPlayer of a different player in UnitAI isn't very clear IMO. And querying the own player can already be done with a single command.
Thanks for working on this.
by , 10 years ago
Attachment: | idle_worker_blink_v3.patch added |
---|
Moved ownershipChanged to player; added label.
follow-up: 11 comment:10 by , 10 years ago
Replying to sanderd17:
Could you also display the number of idle units in the tooltip when blinking the button? This wasn't asked for, but it looks like a simple addition, as you already keep track of the number of idle units anyway.
I added the label.
For the code:
- Session.js line 502 doesnt need to query the player ID. The first argument given to any GuiInterface method is the player id anyway. So that's just a lost call.
Forgot to remove this, after I noticed that its called this way.
- UnitAI shouldn't have to worry about counters too much. It's nice if it can notify the cmpPlayer when the state of a unit changed. But cmpPlayer should handle ownership changes on its own. This would make the code a lot more simple, as it only needs to work in the idle case (probably both regular and formation idle), and only ever needs to query the own player component. Having a function to query the cmpPlayer of a different player in UnitAI isn't very clear IMO. And querying the own player can already be done with a single command.
That's a good point. I changed that. I'm not sure if it must be added to "formation idle" because the normal idle is called when a formation is stopped. Or isn't it?
comment:11 by , 10 years ago
Replying to boeseRaupe:
That's a good point. I changed that. I'm not sure if it must be added to "formation idle" because the normal idle is called when a formation is stopped. Or isn't it?
Not anymore. In one of my recent commits, I added a real idle state for formation members. As a preparation for attacking formations.
comment:12 by , 10 years ago
I think it works like it is because "UnitIdleChanged" is also called, when you move Units in formation. During my tests, I did not found any case where it was not working. If you can show me one, I'll try to fix it.
by , 10 years ago
Attachment: | idle_worker_blink_v4.patch added |
---|
Fixed problem with smooth reset. (opacity)
comment:13 by , 10 years ago
Reading the code again, wouldn't it be better to just include the idle units count in the GetSimulationState() code? The simulation state is also nicely cached in the Session.js.
Also, try not to hardcode classes. Define them in some template (either a boolean flag in the UnitAI, or a list of classes in cmpPlayer).
by , 10 years ago
Attachment: | idle_worker_blink_v5.patch added |
---|
Moved value to GetSimulationState() var.
comment:14 by , 10 years ago
I tried adding this to the player.js Schema, but it is not working. (template can not be parsed)
"<element name='IdleWorkerButtonClasses'>" +
"<zeroOrMore>" +
"<element name='class'><text/></element>" +
"</zeroOrMore>" +
"</element>"
What are I'm doing wrong ?
comment:15 by , 10 years ago
I don't know what you're doing wrong, but it's impossible in any case to get the a list of elements with the same name in the js templates. The most common method is to just have a whitespace separated single string, then you only need to execute the split() command on the string to transform it to an array.
There are a lot of examples that use it, the most simple example is probable in the garrisonholder, where a class list is used to see if a unit can garrison, and one is used to see if a unit can make the building fire more arrows.
comment:16 by , 10 years ago
Hmm, the problem is that the Schema for Player.js looks different than the other onces:
<a:component type='system'/><empty/>
What does that mean and where is the actual definition ?
comment:17 by , 10 years ago
The empty part means it's empty, So you'll have to delete that, as it isn't empty any more. If deleting the empty part alone isn't enough, also delete the other thing, I don't know what it does there, as player is no system component.
EDIT: Reading the logs, leper agrees. The component is no system component, so the entire schema needs to be replaced with your new schema.
by , 10 years ago
Attachment: | idle_worker_blink_v6.patch added |
---|
Now works with class definition in player.xml file.
comment:18 by , 10 years ago
Thx for the information. I missed the <Player> Tag in player.xml and tried to add the new nodes as childs of <Entity>...
follow-up: 20 comment:19 by , 10 years ago
In IsUsedForIdleWorkerButton of UnitAI, you return arrays when the query failed. I think you should default to false instead (as the other return values are true or false).
Also, why do you need two functions IncreaseIdleWorkerButtonCount and DecreaseIdleWorkerButtonCount that are only used once, and are only 3 lines long. It's better to include it directly in the UnitIdleChanged function IMO.
For the rest, it looks fine (didn't test it again though).
comment:20 by , 10 years ago
Replying to sanderd17:
In IsUsedForIdleWorkerButton of UnitAI, you return arrays when the query failed. I think you should default to false instead (as the other return values are true or false).
Also, why do you need two functions IncreaseIdleWorkerButtonCount and DecreaseIdleWorkerButtonCount that are only used once, and are only 3 lines long. It's better to include it directly in the UnitIdleChanged function IMO.
Both stuff which I used in a earlier version and forgot to change.
comment:21 by , 10 years ago
The current version has (at least) one bug left:
How to reproduce:
- load attached save game
- move group 1 a bit down
- group 2 will change its position
- delete group 2
--> Counter will show 8 again but should show 7.
I think the problem is that, after group 2 has moved, it's idle state is "false". I think it should be true or shouldn't it.
A added some warnings to check that and also set the Timer for the idle state from 1s to 5s.
I have no idea how to fix that because I did not found where group 2 is told to move a bit...Any hints how to fix it?
by , 10 years ago
Attachment: | idle_worker_blink_v7.patch added |
---|
Problem with formation update; with some warnings...
comment:22 by , 10 years ago
...
Formation.JS is calling RemoveMembers(), if the unit the ownership of the unit changes. Then the state is set to INDIVIDUAL.IDLE by UnitAI.JS. I fixed the bug by not switching to INDIVIDUAL.IDLE, if the unit is already dead.
by , 10 years ago
Attachment: | idle_worker_blink_v9.patch added |
---|
added: select all idle workers with right click.
comment:23 by , 10 years ago
Instead of not switching states when the unit is dead, it would be nicer if the value of this.isIdle is't changed when switching between idle states. Therefore, the leave method should know what the next state will be. The patch I attached contains some hints on how to do that.
by , 10 years ago
Attachment: | unitai_leave_msg.diff added |
---|
comment:24 by , 10 years ago
It can not switch from Indiv. idle to Formation idle (because formation must always walk in between) or can it ?
follow-up: 26 comment:25 by , 10 years ago
It could be possible by sheer coincidence, if the formation member is already on the right position when forming the formation. But as we're on a µm precise, the chance it happens is really small. I don't know exactly if it first switches to a different state, or can directly go to the idle state.
It would still be nice to include that possibility.
EDIT: Also, requiring the nextstate array to be of length 2 is strange. It will be better to just get the last item, and check for "IDLE" there.
by , 10 years ago
Attachment: | idle_worker_blink_v10.patch added |
---|
Used example from sanderd17 to not change isIdle in case Formation.idle <--> Individual.idle
comment:26 by , 10 years ago
Replying to sanderd17:
It could be possible by sheer coincidence, if the formation member is already on the right position when forming the formation. But as we're on a µm precise, the chance it happens is really small. I don't know exactly if it first switches to a different state, or can directly go to the idle state.
It would still be nice to include that possibility.
EDIT: Also, requiring the nextstate array to be of length 2 is strange. It will be better to just get the last item, and check for "IDLE" there.
Ok, I changed that.
by , 10 years ago
Attachment: | idle_worker_blink_v11.patch added |
---|
Made it compatible to the newest version of minimap.
by , 10 years ago
Attachment: | idleWorker.png added |
---|
put into: binaries\data\mods\public\art\textures\ui\session\icons
by , 10 years ago
Attachment: | idleWorkerActive.png added |
---|
binaries\data\mods\public\art\textures\ui\session\icons
comment:27 by , 10 years ago
I made it compatible to the newest version of minimap.
Because it would look very strange, if just a part of the image would blink (the part, which was approximated using 3 the rectangles), I created a 32x32 Icon with the sleeping worker.
Also see my comment in #823.
by , 10 years ago
Attachment: | idle_worker_blink_v12.patch added |
---|
Compatible with rounded idle worker button; depends on #2421
by , 10 years ago
Attachment: | minimap-idle-colorMask.png added |
---|
copy to "binaries\data\mods\public\art\textures\ui\session"
comment:28 by , 10 years ago
I attached a new version of the patch which is compatible with the recent changes (rounded button image).
Depends on #2421!
How to test the patch:
comment:29 by , 10 years ago
Priority: | Nice to Have → Should Have |
---|
comment:30 by , 10 years ago
Summary: | [PATCH] Make the "idle workers" button blink or change colour → [PATCH] Notify user when workers are out-of-work |
---|
follow-up: 33 comment:31 by , 10 years ago
I don't really like how this look. Maybe it should be whiter?
follow-up: 34 comment:32 by , 10 years ago
Instead of blinking, it would be much simpler (and in my opinion nicer) to just disable the idle worker button when there are no idle workers.
comment:33 by , 10 years ago
Replying to wraitii:
I don't really like how this look. Maybe it should be whiter?
The color can be easily changed in the colorFade_IdleWorker() function. (binaries/data/mods/public/gui/common/colorFades.js)
comment:34 by , 10 years ago
Replying to Josh:
Instead of blinking, it would be much simpler (and in my opinion nicer) to just disable the idle worker button when there are no idle workers.
But then the user doesn't get a visual notification when idle workers are there. (That's my opinion ;))
comment:35 by , 10 years ago
Keywords: | simple review removed |
---|
Maybe something like: disabled if idle = 0, enabled if idle > 0, flashing if idle > 10.
In regards to the patch, putting the idle classes in the player template is a little counter-intuitive. I'd prefer to just have fast response times when requesting idle workers from any class. To help visualize what I'm talking about, I'd like to keep an automatically maintained array in the player component looking something like:
uneval(idle); // Prints ['citizen-soldier':0, 'worker':2, 'trade':0, ... ]
comment:36 by , 10 years ago
Milestone: | Alpha 16 → Alpha 17 |
---|
comment:37 by , 10 years ago
Owner: | changed from | to
---|
boeseRaupe: Do you still have time for this? As I haven't seen you around for awhile, I'll try to finish this.
comment:38 by , 10 years ago
Hello Josh, do you think you'll be able to finish this today? We're supposed to go into feature freeze during the meeting.
comment:40 by , 9 years ago
Milestone: | Alpha 18 → Alpha 19 |
---|
comment:42 by , 9 years ago
Milestone: | Alpha 19 → Backlog |
---|
I think I have a partial fix somewhere, but I'm not working on it at the moment.
comment:43 by , 5 years ago
Component: | UI & Simulation → In-game UI |
---|
Move tickets to In-game UI
as UI & Simulation
got some sub components.
Effect is working but patch need some improvments...