[find_path] from WLP mainlined

Discussion of Lua and LuaWML support, development, and ideas.

Moderators: Forum Moderators, Developers

Post Reply
User avatar
Elvish_Hunter
Forum Moderator
Posts: 1397
Joined: September 4th, 2009, 2:39 pm
Location: Lintanir Forest...

[find_path] from WLP mainlined

Post by Elvish_Hunter » September 28th, 2011, 7:28 pm

Ceres wrote: 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)
Current maintainer of these add-ons:
1.14: The Sojournings of Grog, A Rough Life, The White Troll (co-author), Wesnoth Lua Pack
1.12: Children of Dragons

User avatar
Sapient
Inactive Developer
Posts: 4453
Joined: November 26th, 2005, 7:41 am
Contact:

Re: Ceres' Lua Problems

Post by Sapient » September 29th, 2011, 12:35 pm

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 :mrgreen:

Thank you for fulfilling the specification!
http://www.wesnoth.org/wiki/User:Sapient... "Looks like your skills saved us again. Uh, well at least, they saved Soarin's apple pie."

Anonymissimus
Inactive Developer
Posts: 2460
Joined: August 15th, 2008, 8:46 pm
Location: Germany

Re: [find_path] from WLP mainlined

Post by Anonymissimus » September 29th, 2011, 5:04 pm

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.
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

User avatar
Sapient
Inactive Developer
Posts: 4453
Joined: November 26th, 2005, 7:41 am
Contact:

Re: [find_path] from WLP mainlined

Post by Sapient » September 29th, 2011, 5:28 pm

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.
http://www.wesnoth.org/wiki/User:Sapient... "Looks like your skills saved us again. Uh, well at least, they saved Soarin's apple pie."

Anonymissimus
Inactive Developer
Posts: 2460
Joined: August 15th, 2008, 8:46 pm
Location: Germany

Re: [find_path] from WLP mainlined

Post by Anonymissimus » September 29th, 2011, 5:56 pm

Sapient wrote: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.
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

User avatar
Sapient
Inactive Developer
Posts: 4453
Joined: November 26th, 2005, 7:41 am
Contact:

Re: [find_path] from WLP mainlined

Post by Sapient » September 29th, 2011, 6:43 pm

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.
http://www.wesnoth.org/wiki/User:Sapient... "Looks like your skills saved us again. Uh, well at least, they saved Soarin's apple pie."

User avatar
Elvish_Hunter
Forum Moderator
Posts: 1397
Joined: September 4th, 2009, 2:39 pm
Location: Lintanir Forest...

Re: [find_path] from WLP mainlined

Post by Elvish_Hunter » September 29th, 2011, 6:46 pm

Sapient wrote: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. :)
Anonymissimus wrote: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?
Current maintainer of these add-ons:
1.14: The Sojournings of Grog, A Rough Life, The White Troll (co-author), Wesnoth Lua Pack
1.12: Children of Dragons

Anonymissimus
Inactive Developer
Posts: 2460
Joined: August 15th, 2008, 8:46 pm
Location: Germany

Re: [find_path] from WLP mainlined

Post by Anonymissimus » September 29th, 2011, 8:24 pm

Elvish_Hunter wrote: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).
silene wrote: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.
Elvish_Hunter wrote:Also, is better having allow_multiple_turns with yes or no as default value?
I vote for no.
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

AI
Developer
Posts: 2394
Joined: January 31st, 2008, 8:38 pm

Re: [find_path] from WLP mainlined

Post by AI » September 29th, 2011, 11:04 pm

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
Inactive Developer
Posts: 2460
Joined: August 15th, 2008, 8:46 pm
Location: Germany

Re: [find_path] from WLP mainlined

Post by Anonymissimus » September 29th, 2011, 11:22 pm

AI wrote: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.
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

AI
Developer
Posts: 2394
Joined: January 31st, 2008, 8:38 pm

Re: [find_path] from WLP mainlined

Post by AI » September 30th, 2011, 12:02 am

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.

User avatar
Elvish_Hunter
Forum Moderator
Posts: 1397
Joined: September 4th, 2009, 2:39 pm
Location: Lintanir Forest...

Re: [find_path] from WLP mainlined

Post by Elvish_Hunter » September 30th, 2011, 3:11 pm

Anonymissimus wrote: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. :)
Anonymissimus wrote: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... :P
AI wrote:Keys that are not present are mostly equivalent to no, so that should be your default default.
Anonymissimus wrote: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?
Current maintainer of these add-ons:
1.14: The Sojournings of Grog, A Rough Life, The White Troll (co-author), Wesnoth Lua Pack
1.12: Children of Dragons

Anonymissimus
Inactive Developer
Posts: 2460
Joined: August 15th, 2008, 8:46 pm
Location: Germany

Re: [find_path] from WLP mainlined

Post by Anonymissimus » September 30th, 2011, 6:35 pm

AI wrote: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. ;)
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

AI
Developer
Posts: 2394
Joined: January 31st, 2008, 8:38 pm

Re: [find_path] from WLP mainlined

Post by AI » October 18th, 2011, 6:19 pm

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.

Post Reply