Enhanced looping tags for WML

[Celtic_Minstrel]

I've implemented two new looping tags for WML to complement the [while] tag, and am considering a third as well. The first two are intended to be roughly drop-in substitutions for the existing {FOREACH} and {REPEAT} macros

1. Start with the simplest: A [repeat] tag. This is intended to replace the {REPEAT} macro, and currently works like this:

Code: Select all

# Repeat something 10 times
[repeat]
  times=10
  [do]
    # ... ActionWML here
  [/do]
[/repeat]

2. Next, a for-range loop: [for]. This is actually intended to work as a drop-in replacement for the {FOREACH} macro, but could also be used for running over any arbitrary range. Some examples:

Code: Select all

# Will print out 0, 2, 4, 6, 8, 10
[for]
  start,stop,step=0,10,2
  variable=q
  [do]
    [lua]
      code="wesnoth.message($q)"
    [/lua]
  [/do]
[/for]
# Will print out all the values in my_array, in reverse order, ie 12, 4, 21, 17, 10
# (Note that this specific example gets a bit more complicated if it's not numbers)
[set_variables]
  [value]
    value=10
  [/value]
  [value]
    value=17
  [/value]
  [value]
    value=21
  [/value]
  [value]
    value=4
  [/value]
  [value]
    value=12
  [/value]
[/set_variables]
[for]
  array=my_array
  reverse=yes
  variable=n
  [do]
    [lua]
      code="wesnoth.message($my_array[$n].value)"
    [/lua]
  [/do]
[/for]

(Don't be confused by the Lua there - that just means it's going to print to the chat area instead of popping up a message box. Obviously you could do anything else you wanted there.)

3. Last but not least, a [foreach] loop. You might notice that [for] is intended as a drop-in substitute for {FOREACH}, yet I also intend to create a separate [foreach]. That's because the {FOREACH} macro is a thin wrapper around the [while] tag and is really "for i from 0 to length" rather than "for x in array", ie it's not a real foreach loop. Furthermore, the [foreach] tag would make it impossible to add or remove entries in the array from within the loop, which is something that can already be done with the {FOREACH} macro. Example usage:

Code: Select all

[foreach]
  variable=my_array
  item_var=thing
  index_var=i
  [do]
    # ... ActionWML
  [/do]
[/foreach]

4. Finally, the implementation. The implementation of [for] and [repeat] can be viewed on GitHub, and this is the current [foreach] implementation (based on one by Elvish_Hunter):

Code: Select all

function wml_actions.foreach( cfg )
	local array_name = cfg.variable or helper.wml_error "[foreach] missing required variable= attribute"
	local array = helper.get_variable_array( array_name )
	if #array == 0 then return end -- empty and scalars unwanted
	local item_name = cfg.item_var or "this_item"
	local this_item = start_var_scope( item_name ) -- if this_item is already set
	local i_name = cfg.index_var or "i"
	local i = start_var_scope( i_name ) -- if i is already set
	
	for index, value in ipairs( array ) do
		wesnoth.set_variable( item_name, value )
		-- set index variable
		wesnoth.set_variable( i_name, index-1 ) -- here -1, because of WML array
		-- perform actions
		for do_child in helper.child_range( cfg, "do" ) do
			handle_event_commands(do_child, "loop")
		end
		-- set back the content, in case the author made some modifications
		if not cfg.readonly then
			array[index] = wesnoth.get_variable( item_name )
		end
	end
	
	-- house cleaning
	wesnoth.set_variable( item_name )
	wesnoth.set_variable( i )
	
	-- restore the array
	helper.set_variable_array( array_name, array )
end

---------

Overall I'd like feedback on these tags. Some people have noted that it would be possible to omit the [do] tags, putting ActionWML directly within [for], [foreach], or [repeat]; this is easily done, and the only reason I chose not to was for consistency with [while] (where the [do] tags are required). I could remove the need for [do] tags if most people would prefer that. Other feedback is also appreciated!

[Pentarctagon]

You forgot the url for the [url] tag for where to view this on github.

[tekelili]

Please, add key "variable=" to repeat

Code: Select all

[repeat]
  times=10
  variable=q
  [do]
    # ... ActionWML here
  [/do]
[/repeat]

I never use current REPEAT macro because it can not be nested (and also because I find dirty not be closed by NEXT). I strongly suggest [repeat] tag can be nested.

[Celtic_Minstrel]

You forgot the url for the [url] tag for where to view this on github.

Whoops! I was in a bit of a hurry when I composed this post. Fixed now.

Please, add key "variable=" to repeat

Code: Select all

[repeat]
  times=10
  variable=q
  [do]
    # ... ActionWML here
  [/do]
[/repeat]

I never use current REPEAT macro because it can not be nested (and also because I find dirty not be closed by NEXT). I strongly suggest [repeat] tag can be nested.

I totally understand your concern - the {REPEAT} macro uses an internal variable, the name of which cannot be changed, and because of that you can't nest one {REPEAT} macro inside another. However, this [repeat] tag uses no WML variables at all, so it should have no problems being nested. Thus, there's really no need for a variable= key.

It would be possible to add an optional variable= anyway, to allow people to check within the loop which iteration they're on; however, I'd argue that if they want to do that, they should be using something like this:

Code: Select all

[for]
  start,stop=1,10
  variable=iteration
  [do]
    # ... ActionWML here
  [/do]
[/for]

(You could just as easily change it to start,stop=0,9 if you prefer a 0-indexed iteration count.) The [repeat] tag is intended for when you want to do exactly the same thing several times in a row, ie a drop-in replacement for any current use {REPEAT} (excepting any that access the internal variable, which they shouldn't be doing anyway). If there's enough demand for variable= in [repeat] and the devs are not against it, I'd be willing to add it, but I don't think it's required.

EDIT: Also, for anyone who missed it, I'd like to know whether you prefer to have these with nested [do] tags (as the examples show) or without (putting ActionWML directly under [for], [foreach], or [repeat]).

[Pentarctagon]

I would say to leave the nested [do] tags. That keeps it consistent with the current [while]/[do] syntax and, I think, provides a useful separation from the loop conditions and what the loop is doing.

[tekelili]

EDIT: Also, for anyone who missed it, I'd like to know whether you prefer to have these with nested [do] tags (as the examples show) or without (putting ActionWML directly under [for], [foreach], or [repeat]).

I think is better keep [do] tag to keep WML "internal logic", and I am talking as someone that really dont like too much such syntax. What I do to "fix" a syntax I am not very agree is never indent [do] tag inside [while]

Code: Select all

[while]
    {CONDITION}
[do]
    {WML ACTION}
[/do]
[/while]

Edit: When storing actions in variables and using [insert_tag], it becomes also very handly have a [do] tag to insert action separately from [while] keys
Edit2. Off topic: In fact, I think should do a feature request for [do] tag be supported at [event] level as trivial tag, just for the sake of comfortable use of [insert_tag]

[Ravana]

Wouldnt [command] already do what you want from [do]?

[tekelili]

Wouldnt [command] already do what you want from [do]?

Errr... may be. I couldnt find in wiki [command] listed as WML action or tag supported by [event], but may be I missed it, sry

[Ravana]

http://wiki.wesnoth.org/ActionWML

When a tag permits ActionWML as content, this means that it permits a sequence of any of the following tags:
...
conditional actions (ConditionalActionsWML) which contain other actions, which then are (maybe) executed ([if], [switch], [while], [command])

[Celtic_Minstrel]

If you mean you could use [command] to insert actions into a looping tag independent of the loop conditions, yes, that would be possible regardless of whether the loops had a [do] tag. However... if you used [command], my implementation of break/continue would not work, as those tags need to be directly in the [do] tag (or the [for] / [foreach] / [repeat] tag, if the [do] tags are left out from the implementation). So given that, I think tekelili's argument for retaining [do] has some merit.

[Elvish_Hunter]

I agree with Pent's and tekelili's opinion: the [do] tag should stay, if nothing else for uniformity with the already existing [while] tag. As for [foreach] (but this applies to [for], [repeat] and maybe [while] as well), perhaps it may be useful to add something that allows breaking out of the cycle. I remember that you (or someone else?) on IRC talked about a [break_if] tag, that may be implemented like this:

Code: Select all

      for do_child in helper.child_range( cfg, "do" ) do
         local break_out = false
         for break_tag in helper.child_range( do_child, "break_if" ) do
             if wesnoth.eval_conditional( break_tag ) then
                 break_out = true
                 break
             end
         end
         if break_out then break end
         handle_event_commands(do_child, "loop")
      end

I wrote this code on the fly and it's completely untested, so there may be errors. Also, we need to decide if [break_if] must be evalutated before or after handle_event_commands (I'd say before).

[Celtic_Minstrel]

That causes a [break_if] anywhere within [do] to be evaluated before anything else in that [do]. My own implementation of [break_if] is handled within handle_event_commands (and doubles as a "return" statement if used directly within an event), though the change to the looping code is similar to what you've done.

I didn't mention [break_if] in the opening post because it was the [for] and (especially) the [foreach] that I really wanted feedback on.

[Elvish_Hunter]

That causes a [break_if] anywhere within [do] to be evaluated before anything else in that [do].

Yes, I was aware of that.

My own implementation of [break_if]

Interesting. I didn't think of altering handle_wml_commands for that purpose. You should really link it in the original post too

it was the [for] and (especially) the [foreach] that I really wanted feedback on

I understand. However, there isn't really much feedback that I can give you, since your version of the tag is very similar to mine - which wasn't implemented because of possible issues when modifying an array's length while iterating it (this is why I implemented a reverse= key). Then again, my point of view was and is the same as shadowm's:

Code: Select all

20150819 02:03:57< shadowm> The notion of modifying a container that's being iterated on isn't compatible with an opaque range iteration pattern, no.
20150819 02:04:58< shadowm> Even with a more transparent pattern like a subscript or iterator loop it's an easy way to shoot yourself in the foot, so only people who know what they are doing should do it -- both in WML and C++.
20150819 02:06:12< shadowm> So I really don't believe a prospective [foreach] tag should support that. Also, in hindsight the {FOREACH} name is a little bit unfortunate in that regard.
20150819 02:07:48< shadowm> Basically, if people want it, they should deal with the nitty-gritty by themselves and take full responsibility for any mistakes in their code rather than come back yelling at us.

I even remember that some languages (Visual Basic?) raise an error when altering an array while iterating, so perhaps we can introduce a similar check - or not?

[Celtic_Minstrel]

I even remember that some languages (Visual Basic?) raise an error when altering an array while iterating, so perhaps we can introduce a similar check - or not?

Hmmm. I think this might be a good idea, but I'm not quite sure how to go about it. I'll give it some thought though.

Based on something else shadowm said, I was wondering if anyone would like a reference_var= key which would work like this:

Code: Select all

# Using item_var:
[foreach]
  variable=my_array
  item_var=thing
  index_var=i
  [do]
    # Do something with the element's current value
    [message]
      speaker=narrator
      message=$thing.message
    [/message]
    # Change the value of the current element
    {VARIABLE thing.value new_value}
    # Note that $my_array[$i] is not updated until the end of the loop
  [/do]
[/foreach]
# Using reference_var:
[foreach]
  variable=my_array
  reference_var=ref
  index_var=i
  [do]
    # Do something with the element's current value
    [message]
      speaker=narrator
      message=$$ref|.message
    [/message]
    # Change the value of the current element
    {VARIABLE $ref|.value new_value}
    # Note that $my_array[$i] is has already been updated at this point
  [/do]
[/foreach]

It's admittedly weird syntax, though. Maybe it's better not to include it? The variable would basically contain the string "my_array[$i]" for each loop iteration. You wouldn't have to use it if you don't like it, but that's really not an argument to include it.

[Elvish_Hunter]

Hmmm. I think this might be a good idea, but I'm not quite sure how to go about it. I'll give it some thought though.

One possible way to go may be this one:

• At the start of the tag, when collecting the variables, you set a local Lua variable with the value of array_name.lenght
• Inside the outer for cycle, after the if not cfg.readonly block, you add another if check
• In this if check, you get again the value of array_name.length; if this value isn't the same as the one stored in the Lua variable, use helper.wml_error to raise and exit

This kind of check should be done at last, because the user may legitimately want to remove one element from the array, then use [break_if] to exit the cycle without doing any damage. In this case, the new if check should take a look to the return value of handle_event_commands as well.
Sounds good for a start?

It's admittedly weird syntax, though.

I think that the double dollar symbol will be especially confusing to beginners. Basically, we'll have two ways of doing the same thing. Don't forget that pointers are one of the hardest concepts to grasp in C (and by extension, references in C++), and such a thing in WML will surely confuse a lot of people.