bugFreeciv - Bugs: bug #16383, Option to forbid RiverNative units...

 
 
Show feedback again

bug #16383: Option to forbid RiverNative units moving diagonally / cross-continent

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Sun 08 Aug 2010 09:08:21 AM UTC  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Emmet Hikory <persia>Open/Closed: Closed
Release: 2.3.0Operating System: Any
Planned Release: 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)

Tue 29 Apr 2014 04:02:04 PM UTC, SVN revision 24826:

Test nativity of moves

While the nativity of the tiles for a move is tested, the nativity
of the actual move is not tested. As a result, in the event that nativity
is provided by a road, the move_mode is not enforced for the provision of
nativity, possibly causing odd results, such as unit classes native to
rivers being able to cross continents.

Requested by Jacob Nevins
Based on earlier work by Marko Lindqvist

See bug #16383

(Browse SVN revision 24826)

Emmet Hikory <persia>
Project MemberIn charge of this item.
Sun 27 Apr 2014 03:00:18 AM UTC, comment #27:

This has seemed stable as part of autotests, so now planning to commit in a couple days, unless someone has concerns about the implementation.

Emmet Hikory <persia>
Project MemberIn charge of this item.
Wed 02 Apr 2014 10:18:59 AM UTC, comment #26:

Rebased over r24743 (as patch #3829 landed).

(file #20454)

Emmet Hikory <persia>
Project MemberIn charge of this item.
Mon 31 Mar 2014 04:42:50 PM UTC, comment #25:

The application of patch #4609 encourages an entirely different approach to this issue, as reflected in the attached patch (applies over r24741). By separating the nativity check logic from everything else, it properly handles cases where nativity continuity is not provided by the same road used for movement bonus (as suggested in comment #24). I presume that all bases that provide nativity automatically interface with all possible roads for the purpose of nativity: changing this should be done in future, rather than here (as there are lots of places that need changing, and much of the underlying work exists in other open patches).

The test for transport capacity is not included in the function in large part because of bug #21871: in the event this is applied first, that should be extended to use potentiality for transport tests for this new use in pathfinding, and if that is applied first, this should use potential rather than current capacity in the pathfinding test.

The test for being a city also isn't included because we do it so many other places anyway that it ends up being a duplicate call in both places.

In the event that patch #3829 lands, is_native_move() should probably be updated
to use the integrators cache. Similarly, if patch #3901 lands this needs to be adjusted to apply is_native_move() to the new inline test functions, rather than
single_move_cost.

(file #20450)

Emmet Hikory <persia>
Project MemberIn charge of this item.
Sat 15 Jun 2013 06:55:26 AM UTC, comment #24:

> it may be interesting to consider special handling for the case
> where two cardinally adjacent tiles are both native because of
> different roads


Also: nativity might be provided by one road (such as alien ruleset "Tunnel") and move bonus by another road (any road inside tunnel)

Marko Lindqvist <cazfi>
Project Administrator
Mon 06 May 2013 11:06:16 AM UTC, comment #23:

Grr. I need a better way to test pathfinding: things just seeming to work don't seem to be enough. The attached patch actually clears the roads for the virtual tile in tile_move_cost_ptrs(), thereby testing the correct condition to determine if a road is required. I'm not certain this is sufficient: it may be interesting to consider special handling for the case where two cardinally adjacent tiles are both native because of different roads (or one has a base): for !UCF_TERRAIN_SPEED units (e.g. Triremes), this makes no difference, but for other units, there may be wide variability in the move cost associated with such a move when it should perhaps always cost SINGLE_MOVE or MOVE_COST_IGTER.

(file #17902)

Emmet Hikory <persia>
Project MemberIn charge of this item.
Mon 29 Apr 2013 09:23:33 AM UTC, comment #22:

While working on patch #3886, I realised this also needs adjustments for pathfinding, so that AI Triremes don't consider the river-path through the continent the best possible route only to become confused when unable to actually make a given move. While updating this patch to address that (by adding much of the same logic into tile_move_cost_ptrs()), I understood why there are two calls to tile_virtual_destroy(): my apologies for the extraneous question.

Attached is a (somewhat ugly) patch including minor adjustments to CardinalRiverMoveRestrictions-2.patch to better support pathfinding as well as the string change suggested in my last comment. Given the duplication of logic, it might be nice to define can_class_exist_at_tile() (to be used by can_exist_at_tile()), and possibly wrap the vtile create/clear roads/test nativity/destroy logic in another helper function: neither is addressed in my patch for clearer comparison to the prior version (although git notations seem not to have helped here).

(file #17854)

Emmet Hikory <persia>
Project MemberIn charge of this item.
Mon 29 Apr 2013 12:30:27 AM UTC, comment #21:

"%s cannot move road diagonally" doesn't parse for me (seems to imply the road is being moved, rather than the unit). How about "%s cannot move diagonally on available roads." It would be nice to know which road was cardinal or relaxed+disconnected for this notice, but that gets unpleasant to read in the presence of several cardinal or relaxed+disconnected roads the unit might have used.

For reduction of text, could the tile_virtual_destroy() call be moved outside the if (!can_exist_at_tile()) conditional, or does it need to have been destroyed prior to the road iteration? In terms of program flow, it makes no difference aside from call ordering, but if we ever wanted to change things, it means more lines to edit to change the call.

Emmet Hikory <persia>
Project MemberIn charge of this item.
Sun 28 Apr 2013 11:34:19 PM UTC, comment #20:

- Really prevent the move if no suitable road to travel found. It was falling to default MR_OK after checking the roads
- Player notify text about the reason move failed

As this might need heavy adjustments later for special cases like attacks, and this is more like new feature than extension to road/river type rework of 2.5, I'm not targeting this to 2.6 only.

(file #17848)

Marko Lindqvist <cazfi>
Project Administrator
Sun 28 Apr 2013 10:34:42 PM UTC, comment #19:

Untested patch

(file #17847)

Marko Lindqvist <cazfi>
Project Administrator
Tue 05 Feb 2013 11:49:39 AM UTC, comment #18:

This should be doable now, but needs to wait that rivers are handled as road types. There's no point to implement it to current river implementation getting obsolete soon.

Marko Lindqvist <cazfi>
Project Administrator
Tue 05 Feb 2013 11:33:54 AM UTC, comment #17:

I think this should be determined from recently added "move_mode" road type property. This is also linked to bug #20472

Marko Lindqvist <cazfi>
Project Administrator
Tue 19 Jun 2012 01:29:43 AM UTC, comment #16:

About patch #3333 issue: Your comments (comment #7) seem a bit confused. You mix "shore bombardment" and "can_attack_from_non_native()". Shore bombardment is about ability to attack against non-native tile. can_attack_from_non_native() is about ability to launch attack from non-native tile.
For the former "AttackNonNative" flag has been implemented some time ago already. Patch #3333 address the latter with "AttFromNonNative" flag.

Marko Lindqvist <cazfi>
Project Administrator
Tue 19 Jun 2012 12:22:01 AM UTC, comment #15:

In October, I wrote:

> I'll post my current patch set here shortly.

Cough!
Attached is my unpolished patch stack, applicable to trunk very shortly after S2_4 branched. I haven't tried rebasing it or running it since then (Nov 2011).

(I'm vaguely hoping that patch #3333 / bug #19828 might yield a solution to the attacks problem that stalled this work.)

(file #15847)

Jacob Nevins <jtn>
Project Administrator
Sun 30 Oct 2011 02:46:06 PM UTC, comment #14:

I fear this is unlikely to make 2.4.0 now :(

I've split out the controversial cities change from the general movement change, but the latter is not as bug-free as I claimed in comment #9, due to the ambiguities in RiverNative unit attacks mentioned in comment #7 -- a RiverNative unit does the wrong thing when directed to a city.

I'm unlikely to have time to resolve these issues before 2.4.0. I'll post my current patch set here shortly.

Jacob Nevins <jtn>
Project Administrator
Sat 17 Sep 2011 05:15:37 PM UTC, comment #13:

> An idea: How about creating a new range (for requirements) named
> InCity, OnCityTile or something like that for requirements that
> are limited to the city tile? Units that only can move on rivers
> would have a build requirement like this: "Special", "River",
> "InCity"

We have the CityTile universal, but there's no generalised notion of "build requirements" for it to hook into (it's just hardcoded tech/building/gov). In any case, that wouldn't have a bearing on whether a unit can exist at a tile.

Jacob Nevins <jtn>
Project Administrator
Sat 17 Sep 2011 05:12:49 PM UTC, comment #12:

Pre-existing bugs discovered in comment #9 raised separately as bug #18613 and bug #18675. (The latter was fixed as a side effect of this patch; I've pulled the relevant bit out.)

Jacob Nevins <jtn>
Project Administrator
Sun 11 Sep 2011 10:39:21 PM UTC, comment #11:

> Would you be OK with some way of specifying in the ruleset
> whether a given kind of non-native unit requires present or
> merely adjacent native terrain to a city? (I haven't even begun
> to think about what that might look like, but it should be
> possible.)

An idea: How about creating a new range (for requirements) named InCity, OnCityTile or something like that for requirements that are limited to the city tile? Units that only can move on rivers would have a build requirement like this: "Special", "River", "InCity"

Sveinung Kvilhaugsvik <sveinung>
Project Member
Tue 06 Sep 2011 02:00:24 PM UTC, comment #10:

Made a unit_type that was RiverNative and BuildAnywhere but was not yet enabled in the terrain ruleset. This resulted that the unit could be in the city and could move along rivers iff the city was on a river tile. This seems like an inconsistency but it could be a feature for making river-only units in a mod. I suggest keeping it.

Zakri Kneebone <i1abnrk>
Mon 05 Sep 2011 09:27:26 PM UTC, comment #9:

I'll look at splitting out the cities bit from the rest -- shouldn't be too hard, and it'll hopefully allow the (fully working) movement part to be committed. That is the most important part, which allows RiverNative units to be trivially abused in many cases; if we have a few remaining oddities that require lucky/careful city placement, that's not so bad.

> Current philosophy is to limit building of the units in cities
> without access to native tiles, [...]

I agree that criteria for building and hosting units are in principle different, the former being in can_city_build_unit_{direct,later}() and the latter in can_exist_at_tile().
However, I think the only current practical difference at the moment is that a city in the inland part of a "city channel" can't build sea units.
(Aside: I assumed this was an oversight and considered fixing it, but became alarmed the expense of is_city_channel_tile().)

> [...] but any unit can be stored to
> any city (if you have units that are capable of transporting
> ship to inland city, then your technology certainly allows
> also keeping ship in that inland city).

I don't think that's embodied in the current code. In can_exist_at_tile(), a unit type is considered to be able to exist in a city only if there's some adjacent native terrain (unless it's a BuildAnywhere unit). There's also a recently-added special case here to handle city channels.

Actually, now I come to test this, there are bugs:

  • (by inventing a trireme transporter) my transporter+trireme in a landlocked city cannot unload the trireme (which I think is correct), but put them next to the city and the boat can move into the city; the game then generates several assertion failures on every new turn.
  • If I landlock a city by transforming terrain, a sea unit caught in the city stays there, with the same result (assertion failures). Here update_unit_activity() only checks units on the tile, not in adjacent cities.

(These should be separate tickets, of course.)

Anyway, I think it's not such a stretch from this to adding additional restrictions.

> I think we should retain consistency here, so if such a thing
> is used for RiverNative units, rules for other non-native
> units should be similarly changed.

Strictly applied, this would make it impossible to build boats at all in the default ruleset!
I wouldn't advocate changing the behaviour for all non-native units.

It's true that I've introduced a hardcoded exception here to solve a particular problem, and I accept that the fact that there are already various hardcoded special cases for rivers isn't a strong argument for adding one more.
(BTW, are you considering folding rivers into the gen roads stuff?)

Would you be OK with some way of specifying in the ruleset whether a given kind of non-native unit requires present or merely adjacent native terrain to a city? (I haven't even begun to think about what that might look like, but it should be possible.)

> Of course, building RiverNative units in all/only correct
> places part should be fixed (if it really is broken).

It's not broken, it's just that it allows something that's arguably an exploit, so I wanted to change the behaviour.

> Actually, could you tell me how it works before this patch?

Similar to sea units: you can currently build a RiverNative unit in cities on or adjacent to a river tile (even if not cardinally adjacent).

Jacob Nevins <jtn>
Project Administrator
Sun 04 Sep 2011 06:43:49 PM UTC, comment #8:

> Cities must be on a river to build and host RiverNative units


This change in philosophy is probably the most far-fetching thing in this patch. I think we should retain consistency here, so if such a thing is used for RiverNative units, rules for other non-native units should be similarly changed.
Current philosophy is to limit building of the units in cities without access to native tiles, but any unit can be stored to any city (if you have units that are capable of transporting ship to inland city, then your technology certainly allows also keeping ship in that inland city).
IF we change that, what happens in respect to:
- Ship building in coastal cities (affects virtually all rulesets)
- civ1 style "channels" where ship can travel from coastal city to inland city (affects civ1 ruleset)
- Land units (esp. infantry) in ocean cities (affects alien ruleset)
- RiverNative ships building in normal coastal city (no river)

You may read between the lines that you have to do a bit more marketing work to sell that change to me. :-)

Of course, building RiverNative units in all/only correct places part should be fixed (if it really is broken). Actually, could you tell me how it works before this patch?

Marko Lindqvist <cazfi>
Project Administrator
Sun 04 Sep 2011 12:00:59 AM UTC, comment #7:

Attached my work in progress trunk patch for this.

It's testable. If anyone can help with the remaining pathfinding glitches, that would be great, as I'm not too familiar with pathfinding.

Patch is in two forms: all-in-one diff, and mbox containing split out steps (with pathfinding changes separate from the rest).

It does two main things:

  • Optionally, prevents non-cardinal movement of RiverNative units along rivers (point 1).
    • Moves between non-river to river tiles (transiting the mouth of a river) must be cardinal too; I vaguely wanted not to do that, but there were some awkward corner cases.
    • Moves from river to a tile with a transporter should be unrestricted, as for other dis/embarkation (but I haven't tried creating a trireme-transporter to test this).
    • I've created a new ruleset parameter "rivernative_move_mode" to control this, rather than using river_move_mode, so that modders can disable land unit river move bonuses entirely while retaining this new behaviour for boats.
    • The patch enables this for the experimental ruleset.
    • I've apparently-successfully taught the pathfinding about this restriction, and I've seen an AI successfully move a unit along a river.
  • Cities must be on a river to build and host RiverNative units (point 2).
    • This isn't optional, since I didn't think it that controversial. It could be made optional if required.
    • This required noticeable work to remove assumptions that cities are always safe havens for any kinds of unit. Beware bugs here.

Bugs/difficulties:

  • Pathfinding doesn't yet know about the city restriction; it thinks it can move into a city next to a river (and even do so diagonally). It can't, so "orders aborted because of failed move". I think I've seen an AI Trireme get stuck because of this.
  • There are pre-existing difficulties with RiverNative units attacking, which is hampering work on the PF, as many of the PF algorithms take possible attacks into account, so I need to know what should work:
    • It's not possible to have a sea RiverNative unit (such as experimental Triremes) which can do shore bombardments, even if NoLandAttacks isn't set. This is because such units have to be move_type "Both" (otherwise the game complains on loading), and can_attack_from_non_native() only allows UMT_SEA units.
    • Conversely, experimental Triremes can attack land units on river squares, even if NoLandAttacks is set. This is understandable, but not ideal.
    • Ignoring that, for the legitimate case of ship-to-ship bombardments on rivers, it would be nice if non-cardinal attacks could be disallowed, but I'm not sure it's practical.

(Wrt my original point 3: I wouldn't try to apply this patch to a stable branch now.)

(file #14026, file #14027)

Jacob Nevins <jtn>
Project Administrator
Sat 03 Sep 2011 11:49:42 PM UTC, comment #6:

> In experimental ruleset, I'd suggest to remove "TerrainSpeed"
> flag from Trireme, else they can move up to 9 tiles along
> rivers.

Fixed in bug #18590, thanks.

> I would like to be able to create a land class that can't cross
> rivers unless roaded (to recreate the importance of bridges for
> tanks in ww2). [etc]

Suggestion raised separately as patch #2951.

Jacob Nevins <jtn>
Project Administrator
Fri 24 Sep 2010 01:39:49 PM UTC, comment #5:

> It is a pity it was never added to official release.


It has been added to development version. It won't be in any 2.2.x bugfix release, but it will be in 2.3.0 feature release.

Marko Lindqvist <cazfi>
Project Administrator
Fri 24 Sep 2010 01:36:05 PM UTC, comment #4:

Yes, that patch is exactly what I was looking for, thanks.
It is a pity it was never added to official release.

-Deleted Account- <tirolalira>
Fri 24 Sep 2010 07:51:07 AM UTC, comment #3:

> 2- In civ3 you can't use railroads out of your national
> borders. I would love to see in freeciv an option to count
> railroads in enemy territory as roads. Already suggested by
> Taza here: http://freeciv.wikia.com/wiki/User:...


is this something like gna patch #1266?

Matthias Pfafferodt <syntron>
Project Member
Fri 24 Sep 2010 12:44:54 AM UTC, comment #2:

I'm testing rivernative for triremes in my personal ruleset and I think you pointed important issues with those questions.

Q1: I like the possibility to avoid diagonal moves for rivernative units. I agree linking it to river_move_mode would be a good option.

Q2: If rivernative diagonal moves are allowed, I see ok that any city near rivers can build them. I would link the requirement to be ON a river to river_move_mode, too.

In experimental ruleset, I'd suggest to remove "TerrainSpeed" flag from Trireme, else they can move up to 9 tiles along rivers.

If a rivernative unit is upgraded to a non rivernative, the game launch an error when other units try to attack it. I'd suggest to disband units out of native tiles, or to avoid triremes to be upgraded to non-triremes in the ruleset.

Apart of that, this rule seems working right.

Let me a couple of related personal wishes:

1- I'm testing also the "big land" units that can't move to some terrain unless roaded (using roadnative), and I would like to be able to create a land class that can't cross rivers unless roaded (to recreate the importance of bridges for tanks in ww2).
I think it would also be realistic to have sea units that can enter rivers except if roaded (to recreate big ships stoped by bridges).

2- In civ3 you can't use railroads out of your national borders.
I would love to see in freeciv an option to count railroads in enemy territory as roads. Already suggested by Taza here: http://freeciv.wikia.com/wiki/User:Taza/Rule_changes

-Deleted Account- <tirolalira>
Sun 08 Aug 2010 09:37:10 AM UTC, comment #1:

Q1: I would say yes. The move should be restricted to the river_move_mode. The move to a city out the river doesn't matter to me. Of course, from the graphics, I would say no, but then it would be very hard to know what city is reachable or not (e.g. handling the coastal cities).

Q2: Yes, of course.

Q3: The best would be to fix that for 2.2. However, if the work is too important or risky for some reason, we can leave it as a feature for that branch.

pepeto <pepeto>
Project Member
Sun 08 Aug 2010 09:08:21 AM UTC, original submission:

Observations on playing with "experimental" ruleset on trunk (but presumably affects implementation of RiverNative on S2_2 too):

Currently, RiverNative units (Triremes) can always move diagonally between river squares. This is unlike the effects of rivers on land units, which are governed by river_move_mode, which is usually set to require movement strictly along rivers (prohibiting diagonal movement).

As well as reducing movement costs, if two river sources are diagonally adjacent, this allows a Trireme to go from one side of a continent to another along rivers. This seems excessively powerful.

Question 1: should RiverNative be influenced by river_move_mode (or by a separate but similar enumeration in the ruleset), or should it stay as it is?

Secondly, it's possible for cities adjacent to (not on) river squares to produce RiverNative units. This implies that such units can move through such cities. This makes it possible in some circumstances for a city to "bridge" two rivers to allow cross-continent movement. Even if we fix diagonal movement, there will still be circumstances where this is possible.

Question 2: should cities have to be on a river square in order to produce/contain RiverNative units? (Combined with 1, I think this would plug the cross-continent hole.)

Question 3: if we are changing this, should we change it on S2_2 also? Are there any significant rulesets which use RiverNative yet which would be upset by such a change?

I had a look at the original ticket where this was implemented (RT #40396) and didn't find any rationale in this area.

Jacob Nevins <jtn>
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 #20454:  enforce-move-nativity+r24743.patch added by persia (8kB - application/octet-stream)
file #20450:  enforce-move-nativity.patch added by persia (7kB - application/octet-stream)
file #15847:  trunk-rivernative-cardinal-wip-bis.mbox added by jtn (31kB - application/mbox - trunk r20486)
file #14026:  trunk-rivernative-cardinal-wip.diff added by jtn (25kB - text/x-diff - trunk r20223: WIP)
file #14027:  trunk-rivernative-cardinal-wip.mbox added by jtn (27kB - application/octet-stream - trunk r20223: WIP)

 

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 sveinung (Posted a comment)
  • -unavailable- added by i1abnrk (Posted a comment)
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by syntron (Posted a comment)
  • -unavailable- added by tirolalira (Posted a comment)
  • -unavailable- added by pepeto (Posted a comment)
  • -unavailable- added by jtn (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 25 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 29 Apr 2014 04:13:59 PM UTCpersiaStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sun 27 Apr 2014 03:00:18 AM UTCpersiaCategoryNone=>general
      Assigned toNone=>persia
    Wed 02 Apr 2014 10:18:59 AM UTCpersiaAttached File-=>Added enforce-move-nativity+r24743.patch, #20454
    Mon 31 Mar 2014 04:42:50 PM UTCpersiaAttached File-=>Added enforce-move-nativity.patch, #20450
    Thu 30 May 2013 09:43:01 PM UTCcazfiAssigned tocazfi=>None
    Mon 06 May 2013 11:06:16 AM UTCpersiaAttached File-=>Added CardinalRiverMoveRestrictions-2+persia2.patch, #17902
    Mon 29 Apr 2013 09:23:33 AM UTCpersiaAttached File-=>Added CardinalRiverMoveRestrictions-2+persia.patch, #17854
    Sun 28 Apr 2013 11:34:19 PM UTCcazfiAttached File-=>Added CardinalRiverMoveRestrictions-2.patch, #17848
      Planned Release2.5.0=>2.6.0
    Sun 28 Apr 2013 10:34:42 PM UTCcazfiAttached File-=>Added CardinalRiverMoveRestrictions.patch, #17847
      StatusIn Progress=>Ready For Test
    Tue 05 Feb 2013 11:49:39 AM UTCcazfiStatusPostponed=>In Progress
      Assigned tojtn=>cazfi
    Tue 19 Jun 2012 12:22:01 AM UTCjtnAttached File-=>Added trunk-rivernative-cardinal-wip-bis.mbox, #15847
      StatusIn Progress=>Postponed
    Sun 30 Oct 2011 02:46:06 PM UTCjtnPlanned Release2.4.0=>2.5.0
    Sun 04 Sep 2011 12:00:59 AM UTCjtnAttached File-=>Added trunk-rivernative-cardinal-wip.diff, #14026
      Attached File-=>Added trunk-rivernative-cardinal-wip.mbox, #14027
      Priority1 - Later=>5 - Normal
      StatusNeed Info=>In Progress
      Assigned toNone=>jtn
      Release=>2.3.0
      Operating SystemNone=>Any
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup