patchFreeciv - Patches: patch #3721, Getting rid of move_type...

 
 
Show feedback again

patch #3721: Getting rid of move_type dependency on unitselect dialog

Submitted by:  Marko Lindqvist <cazfi>
Submitted on:  Tue 19 Feb 2013 07:27:44 AM UTC  
 
Category: clientPriority: 5 - Normal
Status: Ready For TestPrivacy: Public
Assigned to: Jacob Nevins <jtn>Open/Closed: Open
Planned Release: 2.5.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

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

 

Tue 23 Apr 2013 06:19:52 PM UTC, comment #5:

Oh, you were talking about server/client sync. Yes, even though client is single-threaded, and in that sense does a lot of things "atomically", user action can happen between any two packets from server.
As for transport and cargo being in different tiles, that can certainly happen - we've just been fixing entire class of bugs when client does (even without user interaction) illegal things while that's the case.

Marko Lindqvist <cazfi>
Project Administrator
Tue 23 Apr 2013 12:08:58 PM UTC, comment #4:

Not being thread safe isn't just about whether the client is multithreaded. The entire computational system (client+server) has at least two threads, and if I read unit_move() correctly, send_unit_info_to_onlookers() is called a few times before the transported units are moved, so that there is a window during which the client sees the transporting unit moved and the transported units not yet moved, because the client thread (the single thread of the client process) isn't waiting for the server to finish with unit_move_handling() to do the next action. In practice, the situation described above would require either extremely variable network latency or a vast disparity in computational power between the client and the server, but this definitely conceivable with a fast gaming PC running the client and a heavily-loaded public internet server running on aging donated hardware in the lowest QoS band of some oddly-connected data centre.

Note that in the situation above, although the units are on different tiles, they still believe themselves to be transported, so by querying transport status, the information collected can be expected to also be reliable once the server has finished processing the move (and since the information that the transport is no longer occupied is sent before the information that the unit has moved, the opposite case (thinking a unit is in a transport when it has in fact moved out) will never occur with the current code).

Emmet Hikory <persia>
Project Member
Mon 22 Apr 2013 01:36:39 PM UTC, comment #3:

While making things thread-safe in case threads are used in the future does have it merits, I don't see value in partial solution that just has smaller window for particular error, but is still not at all thread-safe. That doesn't make it one bit easier to implement it thread-safely in the future.

Marko Lindqvist <cazfi>
Project Administrator
Sun 21 Apr 2013 09:01:05 PM UTC, comment #2:

I don't think it is safe to use !can_exist_at_tile() as the filter if we're looking to exclude transported units: that implies an assertion that at any time the client is sufficiently idle to take a new command, no untransported unit cannot exist on it's tile, which wouldn't be thread-safe if units are drowning, or possibly in certain transport situations (I haven't reviewed the client code sufficiently to confirm this: it may be that the client already group-moves units the server permits to move, or similar). Additionally, units in nested transport may be selected if they are native, even if they cannot be used (e.g. Cruise Missile in Mech. Inf. in Transport). If the intent is to find units that are both transported and unable to disembark, we should be using something like:

ptrans = unit_transport_get(punit);
if (ptrans != NULL
&& !(can_unit_unload(punit, ptrans)
&& can_unit_exist_at_tile(punit, utile))) {
return FALSE;
}

Using tile_city(utile) != NULL in both conditions causes any units in cities to appear twice. Perhaps these conditions could depend instead on the local terrain+infrastructure, so:

LAND: (!is_ocean(utile) && is_native_tile(unit_type(punit), utile)))
SEA: (is_ocean(utile) && is_native_tile(unit_type(punit), utile)))

This still causes some duplication (some units may be native to both, either inherently, or due to roads/bases/etc.), but doesn't automatically select all sea units in harbour when selecting LAND. On the other hand, it will make non-native (e.g. BuildAnywhere) units unselectable, which may be confusing.

The above said, I don't use this selector other than for tile range, so these opinions may be ill-informed.

Emmet Hikory <persia>
Project Member
Fri 15 Mar 2013 03:39:19 PM UTC, comment #1:

Giving to jtn for him to decide if this fits to his select dialog plans.

Marko Lindqvist <cazfi>
Project Administrator
Tue 19 Feb 2013 07:27:44 AM UTC, original submission:

Unit select dialog uses unit move_type, which we've tried to retire from non-ai code almost a decade already (since gen-movement development begun).

Attached patch replaces individual checks for move types with generic can_unit_exist_at_tile() check.

This changes behavior in many ways. It's probably matter of taste if these behavioral changes are for better or worse:

- "Both" selection type removed entirely. In most rulesets "Both" units mean air units, in some also amphibious units. So you no longer be able select just that selection of units.
- "Land" and "Sea" selection methods required both tile to be of the selected type, and the unit to have that move type. Terrain is still limited the same way, but units are subject to above mentioned can_unit_exist_at_tile() -> 1) Also "Both" units belong to selection 2) tile must really be native to unit, e.g., wheeled land units are not in selection in roadless tiles, alien ruleset native terrain limitations apply
- "World" used to select all units. Now units are subject to can_unit_exist_at_tile() so transported units are not part of the selection

If this is not sufficient, we may use some other property of units than move_type to make the decision what selection groups they belong to. Unit class is one possibility, but suffers from that separate classes like "Trireme" and "Sea" could then not be combined. Maybe we need to introduce new ruleset field for ruleset author to classify units, either to hardcoded categories or dynamic ones.

If we accept this patch to TRUNK (I'd go with it at least as temporary solution), should it go also to S2_4?

Marko Lindqvist <cazfi>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #17256:  UnitselectNativity.patch added by cazfi (4kB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by persia (Posted a comment)
  • -unavailable- added by cazfi (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 2 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 15 Mar 2013 03:39:19 PM UTCcazfiAssigned toNone=>jtn
    Tue 19 Feb 2013 07:27:44 AM UTCcazfiAttached File-=>Added UnitselectNativity.patch, #17256
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup