patchFreeciv - Patches: patch #3804, Enabling restricted air transport

 
 
Show feedback again

patch #3804: Enabling restricted air transport

Submitted by:  None
Submitted on:  Wed 27 Mar 2013 08:20:27 AM UTC  
 
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Originator Email: -unavailable-
Open/Closed: ClosedPlanned 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.

 

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

Sat 27 Apr 2013 06:43:03 AM UTC, SVN revision 22770:

Made it possible to unload from or load to unreachable units only if they are
in a city or native base. Similar to targeting attacks against unreachable
units, some cargo units may be marked to ignore these restrictions.

Patch by Emmet Hikory

See patch #3804

(Browse SVN revision 22770)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 25 Apr 2013 08:54:41 AM UTC, comment #26:

Thanks for catching that. Updated patch attached.

(file #17829)

Emmet Hikory <persia>
Project Member
Thu 25 Apr 2013 07:19:12 AM UTC, comment #25:

Embargs/disembargs bitvectors are not documented in ruleset comments.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 19 Apr 2013 08:54:02 AM UTC, comment #24:

In the end I split wipe_unit() refactoring to ticket of its own as it occurred to me that it's bugfix required to S2_4 too: bug #20744

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 19 Apr 2013 04:51:40 AM UTC, comment #23:

Updated functional patches to use try_to_save_unit() instead of unit_can_be_saved(). Once applied, this ticket should be closed, without the unit patch.

(file #17765, file #17766)

Emmet Hikory <persia>
Project Member
Tue 16 Apr 2013 10:36:09 AM UTC, comment #22:

wipe_unit() refafactoring patch:
- unit_can_be_saved() function name is misleading as it does more than returns boolean. Maybe something like "try_to_save_unit()" to indicate that unit might be saved in that function already.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 12 Apr 2013 08:04:16 AM UTC, comment #21:

Plan to commit first two patches in a couple of days.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 10 Apr 2013 12:41:35 AM UTC, comment #20:

Update to the experimental ruleset patch to take into account changes in README.ruleset_experimental after revision 22705.

(file #17733)

Emmet Hikory <persia>
Project Member
Sun 07 Apr 2013 08:40:30 AM UTC, comment #19:

I've rebased this on revision 22685, and split into three patches to make it easier to read each and test separately. First is refactor-wipe-unit.patch, which does just that: using unit_list structures to avoid drowning count issues or confusion from other potential sources of loose units on a tile. Second is enable-air-transport.patch, which includes all the code changes necessary for ruleset authors to have restricted transports of various types. Third is experimental-air-transport.patch, which just adds units and commentary to the experimental ruleset: these units may not be properly balanced, graphics may not be available, etc., so the third in the series may be better delayed in application (or skipped entirely if thought unsuitable for use with experimental for some reason).

(file #17695, file #17696, file #17697)

Emmet Hikory <persia>
Project Member
Sat 30 Mar 2013 05:26:10 PM UTC, comment #18:

About threading: see recent discussion on freeciv-dev mailing list. Freeciv is essentially single-threaded at the moment, but we sho0uld be preparing to the time it's not.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 30 Mar 2013 04:58:14 PM UTC, comment #17:

Thanks for the clarification. If the implementation model is acceptable, that's enough for me, and I'll keep rebasing it against trunk locally until it seems safe to apply. If anyone has trouble testing the patch currently attached due to trunk changes, please report here, and I'll post a snapshot of patch changes at that time. Similarly, if problems are discovered because of the patch, please post, and I'll address issues.

The only behavioural change for rulesets without embarks/disembarks vectors that I intended to introduce was a small optimisation in the drowning logic: specifically that for UTYF_GAMELOSS and UTYF_UNDISBANDABLE units, if they were unable to find transport (or, if UTYF_UNDISBANDABLE, teleport) in the first attempt, they are simply killed, rather than running them through the find-transport logic a second time (which I would expect to have the same negative result).

If there is a behavioural difference with the entire patch, I'd like details, and will test against a much-reduced wipe_unit() refactoring, as if the attempt failed the first time and succeeds just because we tried again, there's probably something else wrong.

Thinking about it, the current drowning logic (before this patch) iterates over all untransported units on the tile: in the event that the server is running many threads, it could be possible for two transports to be removed on the same tile in separate threads and their units to be intermixed when being saved from drowning (I haven't looked enough at the main server processing loops to know if this is even possible with the current architecture). In the refactored code, this isn't possible, as the set of units being saved/destroyed is limited to the cargo of the transport currently being destroyed, which could potentially cause differing results in saving units based on timing of transport wipes.

Emmet Hikory <persia>
Project Member
Sat 30 Mar 2013 04:28:41 PM UTC, comment #16:

I'd say that if S2_4 wipe_unit() is not proven to be broken, we don't want to touch it any more.

I just am not committing this patch immediately after 36h inspection period, but wait closer to S2_5 branching date (02-May).

btw. Should wipe_unit() produce exactly same results for rulesets that do not have load/unload restriction before and after this patch? Asking for autogame testing - is it bug in patch if final savegames differ from what resulted before the patch.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat 30 Mar 2013 10:05:50 AM UTC, comment #15:

Moving the units to a separate patch seems perfectly reasonable, and I'm fairly unconcerned if that ever lands: the units and vector definitions are only set for testing, and may not actually work well in play.

Shall I also separate out the refactoring of wipe_unit into something that can be applied to both S2_4 and TRUNK, and then create a patch to enable air that depends upon that? Or alternately, is the plan to wait until S2_4 release prior to applying this to TRUNK? I don't mind either way, but if the refactoring looks useful sooner than air transport, I'll separate it right away, whereas if it's only interesting in this context, I'll wait until 2.4 release, and rebase the patch on the wipe_unit() implementation existing at that point.

Emmet Hikory <persia>
Project Member
Sat 30 Mar 2013 09:08:17 AM UTC, comment #14:

Reading the patch (not yet testing) looks good.

Note to committer and anyone using this patch: network capstr needs bump.
It's ok that this is not included in patch as it changes very often just causing patches including it not to apply cleanly.

- Could you separate experimental ruleset change to another patch (ticket). While I consider the code change to 2.5, I don't want to add new units to supplied ruleset for it in 2.5 timeframe (our major releases have been pending gfx required for new features often enough)
- Another issue completely external to this patch itself: We've had plenty of problems with wipe_unit() and friends lately. Keeping it identical between S2_4 and TRUNK would give more testing to S2_4 implementation nearing stable release, so I rather postpone committing this a bit.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 29 Mar 2013 09:09:00 AM UTC, comment #13:

This is a patch with the bitvector implementation, which has passed all the same tests as the original implementation, as well as tests for units trying to implicitly unload through movement. This would not pass a test for units trying to implicitly load by movement, with the result that with the current ruleset patch, the first Air unit on a tile with a Stratotanker will auto-load (but may always be unloaded, if the player wishes to select another unit to load). The client also sometimes considers some movements permitted that are blocked by the server (resulting in the MR_CANNOT_DISEMBARK message, rather than a direct visual clue that the movement cannot be performed).

(file #17587)

Emmet Hikory <persia>
Project Member
Fri 29 Mar 2013 07:25:08 AM UTC, comment #12:

The obvious is what needed stating. Thanks for the pointers: even worse, I had not even defined these bitvectors in packets.def, so that regardless of the fact that I wasn't sending them, they could not be sent.

Emmet Hikory <persia>
Project Member
Fri 29 Mar 2013 07:01:25 AM UTC, comment #11:

Risking stating the obvious: start by checking (reading code, adding logging) that disembarks bitvector is not all-FALSE in client side, i.e., that you send them from server to client ( ruleset.c:send_ruleset_units() ) and client fills them in once packet received ( packhand.c:handle_ruleset_unit() )

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Fri 29 Mar 2013 06:41:43 AM UTC, comment #10:

I've encountered a strangeness during the implementation: a difference in behaviour of unit.c:can_unit_unload() when called from the client and the server. I am using BV_ISSET(unit_type(pcargo)->disembarks, uclass_index(unit_class(ptrans))) to determine if pcargo is permitted to be unloaded from ptrans. When called from unittools.c:wipe_unit(), this works perfectly, so when the transporter is disbanded or destroyed, it is possible to determine if the unit is helpless or merely imperiled. Strangely, when called from any of the gtk3 client accessors (city popup menu, application menu, keystroke (for either transporter's UnloadAll, or cargo's Unload)), the same test invariably returns false. I find the same behaviour for the gtk2 client, wasn't able to test with the Qt client (load/unload does not yet appear to be implemented), and haven't built any of the others.

Is there any obvious client/server context consideration I need to be making, or is this also mysterious to others, so that I am best served by debugging the client behaviour step-by-step?

Emmet Hikory <persia>
Project Member
Thu 28 Mar 2013 01:55:57 PM UTC, comment #9:

No apologies needed (if not from my part). I just wanted to make sure everyone, including lurkers, has the same understanding that alien ruleset was used as an example only.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 28 Mar 2013 01:48:20 PM UTC, comment #8:

Great. Thanks for the confirmation.

As for the examples from the Alien ruleset: my apologies if I implied such changes should be made. I selected it as an example only because it has two different semantics for unreachable units as well as a non-surface road, so was an excellent source of potential corner cases without needing long discussion of definitions.

Emmet Hikory <persia>
Project Member
Thu 28 Mar 2013 01:37:20 PM UTC, comment #7:

I think that's enough for now. No point in using more resources to this when we don't know if anybody is ever going to use it. We can rework it if users express huge demand for improved version.

As for Burrow Tubes example, in addition to being quite absurd idea (but that's ok in Alien ruleset), I consider tubes a bit of a hackish thing anyway. Road separated from ground/sea level is not really supported. Like bombers up in the air in classic rules, burrowing unit in a tube can prevent enemy units from entering tile etc.

For the record, I don't plan to add antigravity or burrowing transports that would use these new rules to alien ruleset. If alien ruleset is going to be part of official distribution in freeciv-2.6, it should now be improved for usability, playability, and quality reasons; it should be stabilizing - no new features for the sake of a new feature.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 28 Mar 2013 12:47:17 PM UTC, comment #6:

Heh, if you're happy with a less complex patch even if it's not known to be correct for any imagined use cases, that makes it relatively easy. I'll implement with two bitvectors (embark, disembark) that work like targets, and in wipe_unit just kill/teleport anything that can't disembark if the transporter is wiped (trying to find nomenclature that doesn't imply semantic assignment for inability to disembark). For my immediate purposes, this is entirely sufficient, although it does impose constraints on ruleset authors with multiple semantics in place for UCF_UNREACHABLE. I presume that in those cases, in the event that defintion of some road, base, or unit would break the local semantics within the ruleset, the ruleset author would have the option of not using the feature in every case (or even at all) to preserve world continuity (or potentially extend the feature as part of a separate patch covering just those corner cases).

Emmet Hikory <persia>
Project Member
Thu 28 Mar 2013 08:18:47 AM UTC, comment #5:

> Unfortunately, without the certain knowledge that the model is
> of vertical separation with only intervening air, this isn't
> enough.


It certainly does not allow just anything, but I think it has good feature enabling / complexity ratio. You need good use-case examples to convince me that any added complexity is worth it. We can leave some restrictions for ruleset authors to work around.

> Consider the case of a Burrowing unit with disembarks including
> Antigravity disembarking over a Burrow Tube: should this unit
> survive because it is disembarking on a native road,


That's just one case of our tile nativity status being just boolean - one tile cannot be differently native than the other. One could also question should Antiburrow missiles really be able to hit burrowing units in tubes, and other units not.
As a ruleset author I had to make the decision of whether to add burrow tubes to ruleset or not, knowing that having them sets some restrictions to other ruleset features.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 28 Mar 2013 07:37:59 AM UTC, comment #4:

Yes, more abstraction makes sense, and certainly expands the range of what ruleset authors would be able to do. is_flying_unit() and is_flying_unit_class() are clearly not in the spirit of gen_move. dai_choose_attacker_air() would need to return to the prior state, but that entire AI module needs work for better abstraction of unit types. Units could have bv_unit_class embarks, disembarks which would be checked in could_unit_load() and can_unit_unload() if ptrans was UCF_UNREACHABLE. wipe_unit() would allow units that can disembark on the location to try to board other transport (assuming they may) if they cannot exist at the tile, and kill/teleport other units.

Unfortunately, without the certain knowledge that the model is of vertical separation with only intervening air, this isn't enough. The Alien ruleset (and potential extensions thereto) provides a nice example for discussion purposes. Consider the case of a Burrowing unit with disembarks including Antigravity disembarking over a Burrow Tube: should this unit survive because it is disembarking on a native road, or die because there is an Ocean or Boiling Ocean between the transporter and the road? Alternately, consider the case of a Native unit with disembarks including Burrowing and embarks including Antigravity disembarking from a Burrow Tube and boarding the Antigravity unit (given !can_exist_at_tile(), this can only happen if the Burrowing transporter is processed by wipe_unit()).

So, to complete an abstract implementation, there needs to be a way to know if any other restrictions apply to embark/disembark situations (especially in cases where a unit is being rescued after loss of transport). If all bases are assumed to be at ground level (and so REACHABLE), and RF_UNREACHABLE is defined as an additional consideration (potentially to be used for artifacts like Burrow Tubes), it may be possible to answer the preceeding questions, but it may cause other issues (an Amphibious unit would no longer be capable of disembarking from Burrowing units in a Burrow Tube). Given an even more complex scenario, this may be considerably more complicated. Further, depending on how the ability to embark/disembark is conceived/described in the helptext, this may be imagined to be a once per turn operation: performing two different such operations in a single turn may be incompatible with the intent of a ruleset author.

More potential cases for consideration and/or suggestions for implementation details to cover corner cases are very welcome.

Emmet Hikory <persia>
Project Member
Wed 27 Mar 2013 11:55:11 PM UTC, comment #3:

At this point I were most worried about level of abstraction. I don't want a feature that is handling specifically flying units, but units that for any reason cannot unload or load cargo outside cities or bases. That affects the function naming at least - "Unreachable" flag was named like that instead of "Flying" for the very reason it might be used for some other unreachability reason than unit flying. Also I'm been thinking if we can go by allowing just one such a rule in a ruleset. If yes, we could have just pair of flags of which one defines units that usually cannot unload, and the other the special cargo units. But like we can have multiple reasons unit cannot be attacked, each with its own (not shared) exceptions, this should too. Alien ruleset has both antigravity and burrowing units as unreachable. The units that can attack flying units are not the same units that can attack burrowing units. Similarly units that can paradrop from antigravity unit should not be automatically able to dig their way out from transport burrowing through earth.
As the requirement are so similar to what unreachable/attacking has, I think implementation should be similar too. First of all I think unreachable in respect to attacks, unreachable in respect to unloading & unreachable in respect to loading will so often be the same units that they should depend on same "Unreachable" flag instead of forcing maintenance of two distinct flags. It would still be possible to have unit unreachable in just one sense by explicitly listing all the units able to attack it/unload from it. To the ruleset format I would just add two unit class vectors, similar to "targets", for unit types. One would list classes that the unit can unload from, and the other classes that the unit can load to. Code to handle the vectors would be almost copy+paste from "targets" handling.

Does this desing make sense to you?

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 27 Mar 2013 01:56:55 PM UTC, comment #2:

For unloading, I think it's important to only be able to air-drop units capable of such activities. For the case of trying to model a BMD-3 or M551 Sheridan, one would grant UTYF_PARATROOPERS and set paratroopers_range to zero or a small value (and maybe set paratroopers_mr_sub to something high for the Sheridan, as it can't fire during landing). That said, it seems inappropriate to try to airdrop any tank, as most true Main Battle Tanks (land equivalent of a Battleship: seemed like a good idea, but less useful as time passes) would be too heavy to airdrop. Rulesets that wished to assume all units from some tech point to have parachutes would just add the flag.

Checking the alien ruleset (at least current SVN trunk), no units would qualify for is_flying_unit. Only the two Missiles have fuel, and they aren't UCF_UNREACHABLE. On the other hand, because of this, no antigravity unit would be restricted (which may or may not be OK, depending on your conception of flight patterns for these units). That said, I well understand that this is more a coincidence than an argument for non-conflict: which of UCF_* or UTYF_* do you think would be a better choice to set a "high-altitude" or "continuous flight" (precise nomenclature to be determined)? I lean towards UCF_*, but I chose to have lots of different classes for my ruleset for other reasons anyway: I don't know whether this would be the correct choice for other folk.

Also, aside from the definition of is_flying_unit(), are there also issues with the implementation of the cannot-load and cannot-unload aspects of the patch?

Anonymous
Wed 27 Mar 2013 01:22:46 PM UTC, comment #1:

The idea that one should't be able to unload/load airborne units just anywhere is one I'd like to have. I'm more concerned about loading side; parachutes do exist but tanks rarely jump in to airplanes flying over them.

At the same time implementation cannot be anything like this patch - to decide that unit is airborne from some properties that by coincidence happen to select just correct units in default ruleset. In fact this may make Burrowing units from alien ruleset to count as airborne.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 27 Mar 2013 08:20:27 AM UTC, original submission:

Please consider applying the attached patch to enable restricted air transport: specifically that units carried by Aerial units cannot safely disembark unless either at an airport or themselves able to fly. The
patch includes two sample units I used for testing: I am unconcerned if
the actual units end up in trunk, rather only about the C code.

I am unsure how this will work with the progressing gen_move changes, which I presume will eventually obsolete is_sailing_unit() and is_ground_unit(), and so would appreciate comments from anyone familiar with the gen_move plan regarding alternate suggestions for this implementation in the event it would conflict with gen_move (perhaps a new UTYF_* or UCF_* value, or some other indicator).

Anonymous

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #17829:  enable-air-transport+docs.patch added by persia (23kB - application/octet-stream)
file #17765:  refactor-wipe-unit+rename.patch added by persia (9kB - application/octet-stream)
file #17766:  enable-air-transport+rename.patch added by persia (16kB - application/octet-stream)
file #17733:  experimental-air-transport+post22705.patch added by persia (5kB - application/octet-stream)
file #17695:  refactor-wipe-unit.patch added by persia (9kB - application/octet-stream)
file #17696:  experimental-air-transport.patch added by persia (5kB - application/octet-stream)
file #17697:  enable-air-transport.patch added by persia (16kB - application/octet-stream)
file #17587:  air-transport-vectorimplementation.patch added by persia (27kB - application/octet-stream)
file #17558:  air-transport.patch added by None (13kB - application/octet-stream)

 

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 (Posted a comment)
  • -unavailable- added by None (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 14 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 27 Apr 2013 06:43:15 AM UTCcazfiStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Thu 25 Apr 2013 08:54:41 AM UTCpersiaAttached File-=>Added enable-air-transport+docs.patch, #17829
    Fri 19 Apr 2013 04:51:40 AM UTCpersiaAttached File-=>Added refactor-wipe-unit+rename.patch, #17765
      Attached File-=>Added enable-air-transport+rename.patch, #17766
    Fri 12 Apr 2013 08:04:16 AM UTCcazfiPlanned Release=>2.5.0
    Wed 10 Apr 2013 12:41:35 AM UTCpersiaAttached File-=>Added experimental-air-transport+post22705.patch, #17733
    Sun 07 Apr 2013 08:40:30 AM UTCpersiaAttached File-=>Added refactor-wipe-unit.patch, #17695
      Attached File-=>Added experimental-air-transport.patch, #17696
      Attached File-=>Added enable-air-transport.patch, #17697
    Sat 30 Mar 2013 09:08:17 AM UTCcazfiStatusNone=>Ready For Test
      Assigned toNone=>cazfi
    Fri 29 Mar 2013 09:09:00 AM UTCpersiaAttached File-=>Added air-transport-vectorimplementation.patch, #17587
    Wed 27 Mar 2013 08:20:27 AM UTCNoneAttached File-=>Added air-transport.patch, #17558
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup