bugFreeciv - Bugs: bug #20842, unit still in focus after put in...

 
 
Show feedback again

bug #20842: unit still in focus after put in sentry mode in batch

Submitted by:  Didier Cassirame <didc>
Submitted on:  Fri 24 May 2013 01:42:08 PM UTC  
 
Category: client-gtk-2.0Severity: 2 - Minor
Priority: 5 - NormalStatus: Fixed
Assigned to: pepeto <pepeto>Open/Closed: Closed
Release: 2.3.4Operating System: None
Planned Release: 2.3.5, 2.4.1, 2.5.0, 2.6.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 13 Jan 2014 09:46:31 PM UTC, SVN revision 24117:

When focusing a unit from the urgent queue, discard units which have already
new orders.

Reported by Didier Cassirame

See gna bug #20842

(Browse SVN revision 24117)

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 13 Jan 2014 09:46:23 PM UTC, SVN revision 24116:

When focusing a unit from the urgent queue, discard units which have already
new orders.

Reported by Didier Cassirame

See gna bug #20842

(Browse SVN revision 24116)

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 13 Jan 2014 09:46:17 PM UTC, SVN revision 24115:

Remove obsolete comment.

See gna bug #20842

(Browse SVN revision 24115)

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 13 Jan 2014 09:46:16 PM UTC, SVN revision 24114:

When focusing a unit from the urgent queue, discard units which have already
new orders.

Reported by Didier Cassirame

See gna bug #20842

(Browse SVN revision 24114)

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 13 Jan 2014 09:46:08 PM UTC, SVN revision 24113:

Remove obsolete comment.

See gna bug #20842

(Browse SVN revision 24113)

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 13 Jan 2014 09:46:06 PM UTC, SVN revision 24112:

When focusing a unit from the urgent queue, discard units which have already
new orders.

Reported by Didier Cassirame

See gna bug #20842

(Browse SVN revision 24112)

pepeto <pepeto>
Project MemberIn charge of this item.
Wed 08 Jan 2014 10:01:09 AM UTC, comment #10:

> Anyway, I started testing your patch on 2.4-beta-2, and it
> seems to work without problem.


I encountered some problems to reproduce with version >= 2.4, I didn't find the reason. But this bug is still reproducible when an enemy unit moves near your unit stack (you also can simulate it with adding a unit near your owns with editor). So I am attaching the version for these branches.

> I think I am still missing something with ACTIVITY_IDLE. Is the
> comment in "packand.c" you mentioned still up to date? I doubt
> about it, because for normal actions such as you described,
> units should be put into the urgent focus queue.


I have tried to investigate. As far as I saw, since revision 8870 for PR#10695, the client doesn't set ACTIVITY_IDLE directly anymore. I propose to remove this obsolete comment (patch attached).

(file #19707, file #19708)

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 10 Jun 2013 04:17:42 AM UTC, comment #9:

I think you are correct. Your patch looks much better than mine: it does the test at a better location, avoiding code duplication, and handle the order list as well. Very good!

As for why I thought only sentry activity orders were concerned, I misunderstood the purpose of the code which I pointed to as a justification for the bug only happening for sentry order. That code is the part which inserts units in the urgent list. It does not relate to the category of orders affected by the bug after a unit is woken up. Oddly though, I never experienced it in any other situation than the sentry order when playing the game.

Anyway, I started testing your patch on 2.4-beta-2, and it seems to work without problem. I'll continue testing and keep you posted if anything comes up.

Didier Cassirame <didc>
Sat 08 Jun 2013 09:24:56 PM UTC, comment #8:

Your patch fix only when you apply "sentry" to units. But if you do "fortify", the bug still exists.

I have first thought to add your line in request_new_unit_activityXXX(), but it may be still broken if order lists have been requested. This would make the code hard to maintain.

I have opted for another way : modifying how units are selected from the urgent_focus_queue. Could you test my patch on S2_3?

I think I am still missing something with ACTIVITY_IDLE. Is the comment in "packand.c" you mentioned still up to date? I doubt about it, because for normal actions such as you described, units should be put into the urgent focus queue.

(file #18059)

pepeto <pepeto>
Project MemberIn charge of this item.
Sat 01 Jun 2013 03:36:41 PM UTC, comment #7:

The modification seems effective on the latest beta (I see the same behaviour change).

Didier Cassirame <didc>
Fri 31 May 2013 10:16:31 AM UTC, comment #6:

As far as I understand, the `urgent_focus_queue` is only updated in the case of a unit which was woken up by a server message out of sentry state to idle state (look at the test conditions around the use of `urgent_unit_focus` in `packhand.c`). My patch should be enough to handle the issue in the 2.3.4 version. I will checkout trunk to see how it may be relevant there.

Didier Cassirame <didc>
Fri 31 May 2013 08:17:23 AM UTC, comment #5:

Good analysis. This patch looks much better. Before preparing commit, could you affirm that only sentry order is affected by this bug?

pepeto <pepeto>
Project MemberIn charge of this item.
Fri 31 May 2013 07:36:36 AM UTC, comment #4:

One last detail which I forgot to add: I think you could not reproduce the bug simply because it's triggered by the pace at which the server will process the packets it receives; for a relatively low number of units, the server will reply quickly enough to let the client updates the units status before it needs to select the next unit to focus on.

My computer being fairly old, I can see the issue happening very early. Perhaps if you raise the number of units in my example by a significant amount (a hundred or so might do it), you would experience it as well.

If you believe that such scenario is unlikely to happen, or you still cannot reproduce it, just disregard and close the bug report.

Thanks.

Didier Cassirame <didc>
Fri 31 May 2013 12:07:06 AM UTC, comment #3:

Your helpful answer, and further testing, led me to investigate again the issue which did not actually disappear after my first patch. As you rightfully indicated, the client should not expect the server to accept requests unilaterally, and thus should not update the state of an unit before receiving the server update. There seems to be one exception to that rule (see comments of function `handle_unit_packet_common` in client/packhand.c), but it doesn't concern the issue at hand.

Basically, whenever a group of units is being awaken, the server will send a batch of packets to indicate that change to the client. In certain situations, the client will then put these units in the `client/control.c:urgent_focus_queue` list, so that these units get the player attention. However, when these units are put in sentry mode, they are not removed from that queue. Later, when the client looks for a unit to be put in focus, it will check `urgent_focus_queue` (see the `client/control.c:advance_unit_focus` function). My original patch was not far off it seems, and the new patch I hereby propose simply removes any unit put in sentry mode from the urgent queue.

I provide also a minimal map to test the issue on the client before and after the patch. Let me restate the steps to reproduce the bug:

- load up the map,
- click on the sole unit stack there,
- click on the 'ready all' button of the select dialog,
- press 'shift-v' to select the whole unit stack,
- press 's' to put them all in sentry again

The above steps consistently produce the issue on my computer. Also, a small log of the packets exchanged between the client and the server is provided, with light annotations (and a few more debugging messages which I added to my own build) of the packet stream. It shows the packets which made units to be added to the urgent list (see packets '10' to '20', which correspond to the idle activity change notification from the server after pressing 'shift-v'), and the selection of the next focused unit (after packet '29').

(file #18034, file #18035)

Didier Cassirame <didc>
Sat 25 May 2013 12:56:01 AM UTC, comment #2:

Could you explain in which situations the server would refuse a request of activity change to sentry? I think this could help me better understand the issue I am experiencing (which may not be an issue but just normal behaviour).

Regarding the bug reproduction, my explanations were perhaps not detailed enough or misleading in some way; let me know if certain steps seem ambiguous or inconsistent. The server seems to return an answer: once being put in sentry client side, each unit in the group appears as being in sentry mode, except for the one which ends up being focused on (I suppose that it also was in sentry, but got immediately woken up because of focus). Somehow, even though the order is accepted, the units are still iterated over in the current focus group.

Btw, I am using stock version 2.3.4 compiled from source on debian squeeze. The packaged version is not installed anymore, but did expose the same issue.

Didier Cassirame <didc>
Fri 24 May 2013 05:56:06 PM UTC, comment #1:

Units should lose focus when the server changes their activity. I don't think it's a good solution to assume the server will accept the client request.

PS: I failed to reproduce this bug.

pepeto <pepeto>
Project MemberIn charge of this item.
Fri 24 May 2013 01:42:08 PM UTC, original submission:

When selecting a group of units (using Ctrl-V for instance), and putting them in sentry, the whole unit set is indeed put in sentry (one can see that their state as been updated, except for the one in focus), but focus still goes through all of them in turn.

This is not happening every time, but it occurs under certain conditions which remain to be clearly identified.

Step to reproduce:

- start a new game
- select all the units on the start tile (using V shortcut or the menu)
- put them in sentry mode once (shortcut S)
- select all the units again
- put them in sentry mode

Now you have to cycle through each unit even though they are supposed to be in sentry.

This problem happens for the gtk2 client, but might also be there in other clients.

You'll find a patch attached which solves the issue (potentially for any client).

I am not sure if the solution offered is the most appropriate one: the issue does not happen for other orders (I just tried with irrigate) though it seemingly uses the same algorithm to update the units state. I must be missing something somewhere.

Didier Cassirame <didc>

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #18034:  0001-fix-focus-unit-list-when-unit-put-in-sentry.patch.gz added by didc (522B - application/octet-stream - new patch and map to reproduce the issue.)
file #18035:  test_case.tgz added by didc (10kB - application/octet-stream - new patch and map to reproduce the issue.)
file #18002:  fix-focus-unit-list-when-unit-put-in-sentry.tgz added by didc (583B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by cazfi (Updated the item)
  • -unavailable- added by pepeto (Posted a comment)
  • -unavailable- added by didc (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 13 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 13 Jan 2014 09:46:55 PM UTCpepetoStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Wed 08 Jan 2014 10:01:09 AM UTCpepetoAttached File-=>Added 0001-Remove-units-which-aren-t-IDLE-from-the-urgent-focus.patch, #19707
      Attached File-=>Added 0002-Removed-obsolete-comment-r8870.patch, #19708
    Wed 09 Oct 2013 09:54:27 PM UTCcazfiPlanned Release2.3.5, 2.4.0, 2.5.0, 2.6.0=>2.3.5, 2.4.1, 2.5.0, 2.6.0
    Sat 08 Jun 2013 09:24:56 PM UTCpepetoAttached File-=>Added S2_3_urgent_focus_queue.patch, #18059
    Mon 03 Jun 2013 12:59:04 PM UTCpepetoStatusIn Progress=>Ready For Test
      Planned Release=>2.3.5, 2.4.0, 2.5.0, 2.6.0
    Fri 31 May 2013 08:17:23 AM UTCpepetoStatusNone=>In Progress
      Assigned toNone=>pepeto
    Fri 31 May 2013 12:07:06 AM UTCdidcAttached File-=>Added 0001-fix-focus-unit-list-when-unit-put-in-sentry.patch.gz, #18034
      Attached File-=>Added test_case.tgz, #18035
    Fri 24 May 2013 01:42:08 PM UTCdidcAttached File-=>Added fix-focus-unit-list-when-unit-put-in-sentry.tgz, #18002
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup