[Freeciv-Dev] [bug #22050] Recursive transport problems
Update of bug #22050 (project freeciv): Status: Ready For Test = Fixed Open/Closed:Open = Closed ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #20, bug #22050 (project freeciv): 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) ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Update of bug #22050 (project freeciv): Status: In Progress = Ready For Test ___ Follow-up 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) ___ Additional Item Attachment: File name: trunk_S2_5_final_could_unit_load.patch Size:1 KB File name: S2_4_recursive_transports_fix.patch Size:5 KB ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Update of bug #22050 (project freeciv): Status: Need Info = In Progress ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Update of bug #22050 (project freeciv): Depends on: = bugs #22187 ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Update of bug #22050 (project freeciv): Depends on: = bugs #22317 ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Update of bug #22050 (project freeciv): Status: In Progress = Need Info ___ Follow-up 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. ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #12, bug #22050 (project freeciv): 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? ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #13, bug #22050 (project freeciv): 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. ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #14, bug #22050 (project freeciv): 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? ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #15, bug #22050 (project freeciv): 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. ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #10, bug #22050 (project freeciv): 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? ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #8, bug #22050 (project freeciv):
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.
*** (Consequences: bug #22190, bug #22189)
** 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?
/* Transporter must be native to the tile it is on (or it itself is
* transported). */
if (!can_unit_exist_at_tile(ptrans, unit_tile(ptrans))
!unit_transported(ptrans)) {
return FALSE;
}
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.
___
Reply to this item at:
http://gna.org/bugs/?22050
___
Message sent via/by Gna!
http://gna.org/
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Update of bug #22050 (project freeciv): Status: Fixed = In Progress Open/Closed: Closed = Open Planned Release: 2.5.0,2.6.0 = 2.4.3,2.5.0,2.6.0 ___ Follow-up 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. ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message posté via/par Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Update of bug #22050 (project freeciv): Status: Ready For Test = Fixed Open/Closed:Open = Closed ___ Follow-up Comment #7: Committed to trunk (r25047 http://svn.gna.org/viewcvs/freeciv?view=revisionrevision=25047) and S2_5 (r25048 http://svn.gna.org/viewcvs/freeciv?view=revisionrevision=25048). ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #6, bug #22050 (project freeciv): 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) ___ Additional Item Attachment: File name: recursive_transport3.patch Size:6 KB ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #5, bug #22050 (project freeciv): New version of the patch: could_unit_load() also prevents recursive transport depth doesn't increase over GAME_TRANSPORT_MAX_RECURSIVE. (file #20873) ___ Additional Item Attachment: File name: recursive_transport2.patch Size:4 KB ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Update of bug #22050 (project freeciv): Status: Need Info = Ready For Test Assigned to:None = pepeto Planned Release: = 2.5.0,2.6.0 ___ Follow-up 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) ___ Additional Item Attachment: File name: recursive_transport.patch Size:4 KB ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #3, bug #22050 (project freeciv): 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. ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
URL:
http://gna.org/bugs/?22050
Summary: Recursive transport problems
Project: Freeciv
Submitted by: pepeto
Submitted on: Sun 18 May 2014 05:46:41 PM CEST
Category: general
Severity: 3 - Normal
Priority: 5 - Normal
Status: Need Info
Assigned to: None
Originator Email:
Open/Closed: Open
Release:
Discussion Lock: Any
Operating System: Any
Planned Release:
___
Details:
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:
/* Only top-level transporters may be loaded or loaded into. */
if (unit_transported(pcargo)
|| unit_transported(ptrans)) {
return FALSE;
}
/* Check iff this is a valid transport. */
if (unit_transported(pcargo)
!unit_transport_check(pcargo, ptrans)) {
return FALSE;
}
*
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):
SANITY_CHECK(unit_list_search(pcargos, plevel) != NULL);
This test duplicates the test just above (+ unit iteration). It will just do
the tests^2.
Questions:
0 May really only top-level transporters be loaded or loaded into?
0 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?
0 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.
___
Reply to this item at:
http://gna.org/bugs/?22050
___
Message sent via/by Gna!
http://gna.org/
___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #1, bug #22050 (project freeciv): (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. ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
[Freeciv-Dev] [bug #22050] Recursive transport problems
Follow-up Comment #2, bug #22050 (project freeciv): 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). ___ Reply to this item at: http://gna.org/bugs/?22050 ___ Message sent via/by Gna! http://gna.org/ ___ Freeciv-dev mailing list Freeciv-dev@gna.org https://mail.gna.org/listinfo/freeciv-dev
