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

Reply via email to