bugFreeciv - Bugs: bug #22050, Recursive transport problems

 
 
Show feedback again

bug #22050: Recursive transport problems

Submitted by:  pepeto <pepeto>
Submitted on:  Sun 18 May 2014 03:46:41 PM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: pepeto <pepeto>Open/Closed: Closed
Release: Operating System: Any
Planned Release: 2.4.3,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)

Sat 19 Jul 2014 12:12:17 PM UTC, comment #20:

Here's my revised release note notes:

  • S2_5/trunk: net effect of all changes under this ticket is as comment #8, except that the invariant which is enforced is as I described the original code to be, not the weaker one.
    • (This and max depth are probably best described as new restrictions in S2_5, though, given that S2_4 enforcement was so weak.)
  • S2_4 net effect:
    • Remove broken attempts to enforce max load depth and which units can transport which; anything that the ruleset appears to permit
      • No change to behaviour at unit load time
      • Deep/"invalid" transport stacks used to cause sanity check complaints, now don't
    • (no change to ability to load onto loaded transport -- you still can't -- so don't need to backport bug #22190 / bug #22189)
Jacob Nevins <jtn>
Project Administrator
Sat 19 Jul 2014 09:06:11 AM UTC, SVN revision 25617:

Recursive transport:

  • remove incorrect and unrelated tests from could_unit_load() ;
  • drop any rule which would limit recursive transporting ;
  • remove test duplicate in sanity check module.

Reported by Jacob Nevins (jtn@gna)

See gna bug #22050

(Browse SVN revision 25617)

pepeto <pepeto>
Project MemberIn charge of this item.
Sat 19 Jul 2014 09:06:03 AM UTC, SVN revision 25616:

Recursive transport:

  • remove unrelated test from could_unit_load() ;
  • re-establish the rule a transporter cannot carry a cargo unit which can

transport it.

Reported by Jacob Nevins (jtn@gna)

See gna bug #22050

(Browse SVN revision 25616)

pepeto <pepeto>
Project MemberIn charge of this item.
Sat 19 Jul 2014 09:05:59 AM UTC, SVN revision 25615:

Recursive transport:

  • remove unrelated test from could_unit_load() ;
  • re-establish the rule a transporter cannot carry a cargo unit which can

transport it.

Reported by Jacob Nevins (jtn@gna)

See gna bug #22050

(Browse SVN revision 25615)

pepeto <pepeto>
Project MemberIn charge of this item.
Sun 13 Jul 2014 05:24:11 PM UTC, comment #16:

Patches attached:

  • for S2_4: remove all load/unload restrictions + a part of the improvements done in S2_5 and trunk ;
  • for S2_5/trunk: remove the test which not related to could_unit_load() + re-establish the rule that cargo unit must not to be able to load its transport (in unit_transport_check()).

PS: I will be away for a while, if someone wants to commit it before I return, he would be welcome.

(file #21406, file #21407)

pepeto <pepeto>
Project MemberIn charge of this item.
Thu 19 Jun 2014 06:26:01 PM UTC, comment #15:

I was thinking the same. Probably it shouldn't add other rules that are clearly defined in the ruleset. And we should remove the recursive transport depth limit too.

pepeto <pepeto>
Project MemberIn charge of this item.
Thu 19 Jun 2014 06:09:09 PM UTC, comment #14:

Ah, yes, I think I'm guilty of writing some of the transport checks that can't work with unit types. We should probably remember this to fix it in the long term, but for the short term, perhaps just permit arbitrary nesting, and hope that ruleset authors don't create silly rulesets?

Emmet Hikory <persia>
Project Member
Thu 19 Jun 2014 05:50:45 PM UTC, comment #13:

> Is the reason it can't be solved in pathfinding because we
> don't have the unit_type in pathfinding, so can't check if a
> given transport is contained in our type, or is it something
> else?


That's a part of the problem. But also, we would need to know also the type of all cargos (recursive) and also the cargo depth of the unit.

> Why doesn't it work for server move handling?


I think it could be handled, but it would require lot of work.

But there is also the AI problems... Lot of places in the code, we are just looking up for a transport of a class, not a complex unit+cargo stack.

pepeto <pepeto>
Project MemberIn charge of this item.
Thu 19 Jun 2014 05:40:39 PM UTC, comment #12:

Is the reason it can't be solved in pathfinding because we don't have the unit_type in pathfinding, so can't check if a given transport is contained in our type, or is it something else? Why doesn't it work for server move handling?

Emmet Hikory <persia>
Project Member
Thu 19 Jun 2014 05:29:02 PM UTC, comment #11:

> I will rewrite unit_transport_check() to prevent cargo to be
> able to load on of its upper-level transport.


Actually, the code which restrict recursive transport cannot be handled correctly by pathfinding, neither by server move handling etc... I dunno what is the best to do.

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 16 Jun 2014 10:23:59 PM UTC, comment #10:

>> Before this fix, had the check been effective, it would have
>> completely prevented Helicopters and Carriers from ever being
>> on each other, but Helicopters could have carried Dinghies.
>> It's as if a complex system of unit classes had been set up to
>> exclude this nesting.
>> (As it is, the 'forbidden' nesting will be allowed at
>> UNIT_LOAD time, but will cause sanity-check grumbling later.)
>
> If I understood correctly, it was allowing (even in
> sanity-check) Helicopters loaded onto Carriers and Carriers
> onto Helicopters.


I checked again, I think I misunderstood this piece of code. Thank you for having pointed it to me. I will rewrite unit_transport_check() to prevent cargo to be able to load on of its upper-level transport.

>> I think at least some of these fixes should go to S2_4.
>
> I will try to propose a patch for it then.


After reflexion, I think the whole patch should be ported to S2_4. Anyone objects?

pepeto <pepeto>
Project MemberIn charge of this item.
Sat 14 Jun 2014 11:09:09 PM UTC, comment #9:

> Re comment #1, what is this check (which is unchanged) for?
> If this ever fails, that's an illegal state, surely regardless
> of cargo, surely? -- the transport has somehow ended up on a
> tile it can't exist on.


I didn't read it as a mistake. However, you are right, this is not a test related to whether the unit could load. It's only sanity checking. I will probably make a patch to move this test in more appropriated place.

> Before this fix, had the check been effective, it would have
> completely prevented Helicopters and Carriers from ever being
> on each other, but Helicopters could have carried Dinghies.
> It's as if a complex system of unit classes had been set up to
> exclude this nesting.
> (As it is, the 'forbidden' nesting will be allowed at
> UNIT_LOAD time, but will cause sanity-check grumbling later.)


If I understood correctly, it was allowing (even in sanity-check) Helicopters loaded onto Carriers and Carriers onto Helicopters.

> If we want to deal sensibly with transport cycles I think we
> should probably do it at ruleset load time (I don't think there
> are any checks on this currently), or just leave it up to the
> ruleset author not to do silly things (I don't think it breaks
> the game engine).


I would say ruleset author shouldn't make silly things... I think that if he wants the rules you describe, there is not reason to disallow it.

> I think at least some of these fixes should go to S2_4.


I will try to propose a patch for it then.

pepeto <pepeto>
Project MemberIn charge of this item.
Sat 14 Jun 2014 04:33:11 PM UTC, comment #8:

Trying to write release notes for this patch, here's my restatement of its effects for my own benefit:

  • Bug fixes:
    • There was no load-time enforcement of max transport depth.
    • There was no load-time enforcement of unit type rules, in rulesets with cycles in the can_unit_transport() relation (which doesn't include any of our supplied ones).
      • These would be allowed, but cause sanity check failures later.
    • Interpretation of max depth was inconsistent. If you had set up T(T(T(T(T(C))))) you would get grumbled at by some sanity checks (but not others). Now this is consistently allowed (but T(T(T(T(T(T(C)))))) is not).
  • Rule changes:
    • You can now load onto a transport that is already inside another transport.
    • In rulesets with cycles in the transport relation, the enforced invariant has been weakened.
      • Previously: No unit may contain a unit which could transport it. ("contain" = directly or indirectly)
      • Now: No unit may contain a unit of its own type.
  • Performance improvements.

Re comment #1, what is this check (which is unchanged) for?

If this ever fails, that's an illegal state, surely regardless of cargo, surely? -- the transport has somehow ended up on a tile it can't exist on.


More on the change in invariant that unit_transport_check() was supposed to enforce:

Consider a (silly) ruleset with:

  • Sea class: Carrier and Dinghy unit types
    • Carrier has cargo class Heli
  • Heli class: Helicopter unit type
    • Helicopter has cargo class Sea (in a cargo net, let's say)

In this ruleset, there's a potential cycle Carrier->Helicopter->Carrier.

Before this fix, had the check been effective, it would have completely prevented Helicopters and Carriers from ever being on each other, but Helicopters could have carried Dinghies. It's as if a complex system of unit classes had been set up to exclude this nesting.
(As it is, the 'forbidden' nesting will be allowed at UNIT_LOAD time, but will cause sanity-check grumbling later.)

After this fix, you can have Carrier-carrying-Helicopter and Helicopter-carrying-Carrier(!), but you're not then allowed to set up Carrier->Helicopter->Carrier or similar. This is a weaker check.

I don't think this is a great loss, however. If we want to deal sensibly with transport cycles I think we should probably do it at ruleset load time (I don't think there are any checks on this currently), or just leave it up to the ruleset author not to do silly things (I don't think it breaks the game engine).


> I don't plan to commit to stable S2_4, as it makes rule
> changes.

I think at least some of these fixes should go to S2_4.

I think the only controversial rule change is the ability to load onto nested transporters. I think the other rule change is OK because the old rule was poorly enforced anyway.

Jacob Nevins <jtn>
Project Administrator
Wed 04 Jun 2014 10:03:02 PM UTC, comment #7:

Committed to trunk (r25047) and S2_5 (r25048).

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 02 Jun 2014 09:03:05 AM UTC, comment #6:

The tests didn't pass. My patch doesn't fix all problems.

Attaching new version:

  • unit_transport_check() also check cargo of the cargo unit (because some transported units also could be incompatible with one of the transport) ;
  • Check GAME_TRANSPORT_MAX_RECURSIVE taking in account the cargo depth of the cargo unit.

(file #20907)

pepeto <pepeto>
Project MemberIn charge of this item.
Fri 30 May 2014 11:02:42 PM UTC, comment #5:

New version of the patch: could_unit_load() also prevents recursive transport depth doesn't increase over GAME_TRANSPORT_MAX_RECURSIVE.

(file #20873)

pepeto <pepeto>
Project MemberIn charge of this item.
Wed 28 May 2014 09:00:39 AM UTC, comment #4:

Going to apply my patch:

  • Changes in unit_transport_check():

* Take in account the transporter argument, don't assume the cargo is already loaded in it.
* Only disallow cargo that are on the same type of one of the upper-level transporter.

  • Fix could_unit_load():

* Only top-level transported can be loaded into another transporter. But units can be loaded into a transporter already transported.
* Really check if the cargo is a valid for the transporter.

  • Sanitycheck: remove test duplicates.

I don't plan to commit to stable S2_4, as it makes rule changes.

(file #20824)

pepeto <pepeto>
Project MemberIn charge of this item.
Mon 19 May 2014 04:03:04 PM UTC, comment #3:

On question 3, i would agree that the volume taken up by a transported unit shouldn't change if it has something else inside it. This is not true for the case of something being transported outside, but those cases are so few we can probably ignore them. Weight is another issue, but we might safely ignore it. Also, space taken up by a deployed unit is much more than needed when it is packed for transport - but the multiplier would presumable vary between different unit types.

Regarding unit sizes, that level of realism might be more than we need. We already have, for example, the "Big Land" category. Then again, there are already so many fudge factors in unit composition that the ruleset author might end up picking random numbers out of the air. For instance, depending upon whom you talk to [and what era], an aircraft squadron might be made of 6 - 50 aircraft. Now which is bigger, six bombers or 50 fighters?

By comparison, Civilization IV only made the distinction that the Caravel could carry smaller units like Spies but not mainline units. Otherwise all units were considered equal. "Small Land", anyone?

RPGs i've played usually either separately list volume and weight for items or give a single arbitrary 'size' figure which indicates all of: bulkiness, volume, & weight. This is probably what persia had in mind.

David Lowe <doctorjlowe>
Sun 18 May 2014 05:35:09 PM UTC, comment #2:

On questions, I have some opinions, as follows:

1a) For interface simplicity, it makes sense to me that only top-level transporters can be used as a load target, especially as the user has no way to indicate into which transporter a unit should be loaded. Using the external transport also makes narrative sense: it might be hard to load a tank only a boat already placed in a helicopter cargo net.

1b) There's no interface rationale to restrict unloading to the outermost transport, but it gains the same narrative support as 1a above. That said, this should be enforced
at the client interaction level, as otherwise things get very odd (e.g. alliance ends, with loaded tank in boat in helicopter cargo net: should the tank not be bounced?).

2) While the intent (mentioned in the comment in unit_transport_check()) makes sense, I think this is actively annoying. For example consider the possiblility of a missle-carrying mech.inf. being loaded into a cargo plane, which then wants to land on a carrier: should this only be permitted if there are no missiles currently loaded? A more sensible check should probably be just to ensure that none of the containing transports are of the unit type of the unit to be transported.

3) I think this is correct, to support the same scenario of the missile-carrying Mech. Inf. in a cargo plane on a carrier: the carrier can carry the same number of planes, whether they have units loaded, and the plane doesn't care if the Mech. Inf. happens to have missiles loaded. It is up to ruleset authors to ensure that the capacities of various transport are narratively sensible. That said, I'm not sure that all units are the same size: it might be interesting to declare, for example, that the aforementioned plane can take 1 Mech. Inf. or 4 Paratroopers (although this depends on the narrative for the units: if 1 "Paratrooper Unit" actually represents a group of 15-20 effectives, whereas the Mech. Inf. is 6 effectives who can all fit inside the vehicle, they may require similar space), but this is probably a separate feature (units would have a size parameter, which would indicate how many cargo slots they occupied).

Emmet Hikory <persia>
Project Member
Sun 18 May 2014 03:51:57 PM UTC, comment #1:

(Lost the second point of my analysis about could_unit_load())

  • can_unit_exist_at_tile(ptrans) wouldn't be effective neither for the same reason.
pepeto <pepeto>
Project MemberIn charge of this item.
Sun 18 May 2014 03:46:41 PM UTC, original submission:

When I was investigating for bug #21899, I noticed some other related bugs or strange behaviors. In patch #4694, I said I couldn't understand unit_transport_check(). After looking to patch #2480 about recursive transport, I now know why.

After a quick test, I noticed I was enabled to unload a "land transport" from a "sea transport" (on land of course). I got an enigmatic server message about tiles nativity I couldn't reproduce afterward.

But before making a fix, I have some questions I cannot answer alone.


Analysis:
In could_unit_load() ("common/unit.c"):

  • The call to unit_transport_check() will never be effective:

In unit_transport_check() ("common/unit.c"):

  • The argument ptrans is totally ignored. I don't think it is intentional.
  • I don't like the usage of game_unit_by_number() just to drop the const flag.
  • It useless to use a unit_list as long as units are already chained by pcargo->transporter->transporter->etc.

In check_units() ("server/sanitycheck.c"):
This test duplicates the test just above (+ unit iteration). It will just do the tests^2.


Questions:

  1. May really only top-level transporters be loaded or loaded into?
  2. unit_transport_check() assumes that a unit cannot be loaded to its direct transport if it can be transported by any indirect transport. It is not what I would expect. What do you think?
  3. A full transport loaded into another counts only for 1 occupancy. It is not what I would expect neither. But I would like your opinions.
pepeto <pepeto>
Project MemberIn charge of this item.

 

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

Attach File(s):
   
   
Comment:
   

 

Digest:
   bug dependencies.

Digest:
   bug dependencies.

 

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

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 19 Jul 2014 09:06:40 AM UTCpepetoStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sun 13 Jul 2014 05:24:11 PM UTCpepetoAttached File-=>Added trunk_S2_5_final_could_unit_load.patch, #21406
      Attached File-=>Added S2_4_recursive_transports_fix.patch, #21407
      StatusIn Progress=>Ready For Test
    Sat 12 Jul 2014 03:01:34 PM UTCpepetoDependencies-=>Depends on bugs #22317
    Sat 12 Jul 2014 03:01:01 PM UTCpepetoDependencies-=>Depends on bugs #22187
    Sat 12 Jul 2014 02:53:47 PM UTCpepetoStatusNeed Info=>In Progress
    Thu 19 Jun 2014 05:29:02 PM UTCpepetoStatusIn Progress=>Need Info
    Sat 14 Jun 2014 11:09:09 PM UTCpepetoStatusFixed=>In Progress
      Open/ClosedClosed=>Open
      Planned Release2.5.0,2.6.0=>2.4.3,2.5.0,2.6.0
    Wed 04 Jun 2014 10:03:02 PM UTCpepetoOpen/ClosedOpen=>Closed
    Wed 04 Jun 2014 10:03:01 PM UTCpepetoStatusReady For Test=>Fixed
    Mon 02 Jun 2014 09:03:05 AM UTCpepetoAttached File-=>Added recursive_transport3.patch, #20907
    Fri 30 May 2014 11:02:42 PM UTCpepetoAttached File-=>Added recursive_transport2.patch, #20873
    Wed 28 May 2014 09:00:39 AM UTCpepetoAttached File-=>Added recursive_transport.patch, #20824
      StatusNeed Info=>Ready For Test
      Assigned toNone=>pepeto
      Planned Release=>2.5.0,2.6.0
    Sun 18 May 2014 03:47:29 PM UTCpepetoDependencies-=>bugs #20722 is dependent
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup