[find_path] from WLP mainlined

[Elvish_Hunter]

Sapient was right, I should've asked if someone already did this before. Obviously just checking the patches on Gna and the changelog wasn't enough. On that note, why isn't it yet mainlined then?

Well, now it is. I committed in trunk the version from the WLP, with a small modification.

(EDIT: split from Ceres' Lua Problems)

[Sapient]

20110928 21:25:08< CIA-7> elvish_hunter * r51329 /trunk/ (changelog data/lua/wml-tags.lua): Added [find_path]
20110928 21:25:32< Elvish_Hunter> I hope that Sapient will appreciate...

<--- impressed

Thank you for fulfilling the specification!

[Anonymissimus]

To be sure this doesn't get forgotten:

20110928 21:51:32< anonymissimus> some other thoughts; I thinks its better to rename some of the tags/key
20110928 21:51:50< anonymissimus> instead of [traveler] why not simply [filter] as usual ?
20110928 21:53:02< anonymissimus> also, I find ignore_something=yes|no quite confusing, variable or key names should never have a negation IMHO
20110928 21:53:49< anonymissimus> even if that's already the name of the key when passing to wesnoth.find_path and maybe the name of teh parameters in the C++ code as well
20110928 21:55:21< anonymissimus> check_visibility and similar would be better names imho (as I did for ingore_passability in a tag, before adding that key to several other tags)
20110928 21:56:00< anonymissimus> well, I'll point you to the log in case you don't read it

The ignore_something keys at least should be renamed, especially since I (re)named similar keys in other tags already, such as check_passability. Unless something is released already we can do that right away without any deprecation.

[Sapient]

While "filter" has the advantage of instant recognition that this is a standard unit filter, on the other hand I think "traveler" conveys more information about the purpose of the tag and also makes it more readable overall.

Also, "ignore" is not necessarily a negative. I understand the need to avoid double negatives, but that ain't not no negative, no sir!

These are mostly just statements of my opinion and no offense intended to those who think differently, of course. Typically the implementer gets to decide those sort of minor details, so while I do have my preferences I prefer to leave it up to Elvish.

[Anonymissimus]

Typically the implementer gets to decide those sort of minor details

I don't think so. Typically the implementer has to watch out for how other tags handle things and then keep in line with that. So in this case, I previously made some changes (introduced certain key names, which are already released -> carved in stone). While I'm introducing the side_filter support for many tags I see that many tags handle their side= parameter differently for instance, and that causes problems. Key naming falls in that category too. Now I could go ahead and rename the ignore_units and ignore_teleport parameter in wesnoth.find_path, but since I'm no colour->color guy I don't think this is a valid enough reason to accept the UMC breakage.

[Sapient]

Yes, obviously if someone submitted features named colour I'm sure that name would be rejected. I guess you have a point there. Still it is nice to give implementers a reasonable amount of flexibility where appropriate. It can be a motivating factor.

Anyway, while my preference hasn't changed since I first made the proposal (if anyone even cares), I'll leave the final syntax decisions to others.

[Elvish_Hunter]

Typically the implementer gets to decide those sort of minor details, so while I do have my preferences I prefer to leave it up to Elvish.

I don't have any preference, so it isn't a problem for me to rename some keys.

Now I could go ahead and rename the ignore_units and ignore_teleport parameter in wesnoth.find_path, but since I'm no colour->color guy I don't think this is a valid enough reason to accept the UMC breakage.

This is my (first?) proposal, let's see if it's OK:

Code: Select all

[traveler] -> [filter]
[destination] -> [filter_location]
ignore_visibility -> check_visibility
ignore_teleport -> check_teleport
ignore_units -> check_zoc

Also, is better having allow_multiple_turns with yes or no as default value?

[Anonymissimus]

This is my (first?) proposal, let's see if it's OK:

Code: Select all

[traveler] -> [filter]
[destination] -> [filter_location]
ignore_visibility -> check_visibility
ignore_teleport -> check_teleport
ignore_units -> check_zoc

As for the keys, fine with me.
As for the tags, I'm not sure, actually. filter_location is maybe too generic. For instance, wesnoth.find_path could get extended at some point to that there no longer needs to be a unit at the source location and then we have a problem with such a tag name. If we keep [destination] then maybe it's better to keep [traveler] too...

The cost returned by find_path has some sophisticated behavior I'd like to make sure you are aware of, just in case you get confused by it (as I was).

Note that the cost value takes into account wasted points at end of turns and zones of control. For instance, if, arrived on a given tile, a unit would have only one mp left yet the next tile has a cost of 2, then the actual cost for the next tile will be 3, since the unit will have to wait the next turn to proceed with the move.

Also, is better having allow_multiple_turns with yes or no as default value?

I vote for no.

[AI]

Keys that are not present are mostly equivalent to no, so that should be your default default.

(so if you need the default to be yes, that's when you negate the key)

[Anonymissimus]

Keys that are not present are mostly equivalent to no, so that should be your default default.

(so if you need the default to be yes, that's when you negate the key)

I stronly disagree.
I've seen confusion not only on my side due to negations in variable or parameter names (such as wml key names), so this is to be avoided.
As for the default, if the tag itsself isn't new, it should match the previous behavior of the tag so that no deprecation is needed. If the tag is completely new, the default should match the most useful behavior. If unsure what's the most useful behavior, we can agree on no.

[AI]

That was how stuff was done when WML tags were still written in C++. I believe silene ensured *everything* worked that way when he made WML values typed in storage.

[Elvish_Hunter]

For instance, wesnoth.find_path could get extended at some point to that there no longer needs to be a unit at the source location and then we have a problem with such a tag name. If we keep [destination] then maybe it's better to keep [traveler] too...

I agree.

The cost returned by find_path has some sophisticated behavior I'd like to make sure you are aware of, just in case you get confused by it (as I was).

For the way this tag works, it shouldn't be a problem. If it is, well, I'll find a bug report sooner or later...

Keys that are not present are mostly equivalent to no, so that should be your default default.

If the tag is completely new, the default should match the most useful behavior. If unsure what's the most useful behavior, we can agree on no.

My new proposal:

Code: Select all

ignore_visibility, default yes -> check_visibility, default no
ignore_teleport, default no -> check_teleport, default yes
ignore_units, default no -> check_zoc, default yes

The reason behind these defaults is: a UMC author may want, usually, to calculate a path following the standard rules (ZoC, teleport), so these shouldn't be ignored unless requested; on the other hand, said author may need to calculate the path on a shrouded map, so shroud may be ignored unless requested. Thoughts?

[Anonymissimus]

That was how stuff was done when WML tags were still written in C++. I believe silene ensured *everything* worked that way when he made WML values typed in storage.

When accessing these boolean parameters from the passed vconfig userdata (cfg.comething) in a lua-implemented tag they become nil, and nil is automatically casted (no idea whether it is called casting in lua) to false. Thus empty keys defaulting to no remove two lines of code needed otherwise, such as

local something = cfg.something
if something == nil then something = true end
if something then --do stuff
else --do other stuff
end

as opposed to just

if cfg.something then ...

I guess this is the reason and certainly not worth sacrificing the greater usefullness or backwards compatibility, and I'm sure silene would agree.

[AI]

The reason is not ease-of-programming, it's having a consistent interface.

[unit] [status] healable (default=yes) was renamed to unhealable (default=no) for pretty much this reason.