check_passability for [modify_unit]?

[Mathemagician]

Is there any equivalent to [unstore_unit]'s check_passability key when using [modify_unit]? I tried placement=map and passable=yes from SingleUnitWML, but that had no effect.

[beetlenaut]

This doesn't make a lot of sense. There is no possibility of a unit moving when you use modify_unit (like there is with unstore_unit), so it shouldn't need to check the terrain. And placement options won't do anything because the unit is already placed. If you mean that the modification could suddenly make the terrain under the unit impassible for it, you will have to separately check for that after the modification and move the unit yourself.

[Mathemagician]

The wiki specifically mentions that you can update the x and y coords of a unit with modify_unit, and that works just fine. Basically,

Code: Select all

[modify_unit]
    [filter]
        ...
    [/filter]
    x=3
    y=5
[/modify_unit]

but x and y are calculated in my case.

[beetlenaut]

OK, you're right. I missed that you could do that. Still, the [modify_unit] tag is not made for moving the unit, so it doesn't make a lot of sense for it to check the terrain. You should do your other modifications with [modify_unit], then use something else to move the unit. You could use [teleport] (you can turn off the animation) or [move_unit]. Both of those do have passability checks because they are made for moving the unit.

[Mathemagician]

Ooooh, this looks promising! teleport seems to be what I want, with one exception: it only applies to a single unit. Right now, I can say

Code: Select all

[modify_unit]
    x="$($this_unit.x...)"
    y="$($this_unit.y...)"    
[/modify_unit]

to move several units to distinct locations. But with teleport, I now have to store_unit and iterate over the array, doubling the amount of code I have to write. The gold standard would have the following properties:

1. Implements check_passability
2. Doesn't remove the unit from play if the formula puts it out of bounds
3. Doesn't animate
4. Can be applied to multiple units

So far, move_unit forces animation, while teleport, modify_unit and unstore_unit all require me to reimplement functionality that wesnoth already provides. Is there any way to get everything I want?

As an aside, I think modify_unit should implement passability checks, simply because unstore_unit does that, and the wiki recommends using modify_unit instead of unstore_unit for these sorts of things.

[beetlenaut]

As an aside, I think modify_unit should implement passability checks

I don't think so, because we already have two separate tags for moving a unit, and we don't need a third. Actually, [teleport] and [move_unit] should be combined into a single tag that has all the features of both. For example, [move_unit] could have a teleport=yes/no key (allowing you to skip the pathing), and animate=yes/no. Then there would be no reason for [teleport] to exist. I would make that feature request: The developers are more likely to want to remove a tag than add a new key to an existing one.

[Mathemagician]

If you take that argument to it's logical conclusion, you could argue that both tags should be folded into [modify_unit]. There's a bit of a philosophical debate lying under this, about whether a unit's position should be considered one of their properties, which I think we disagree on. But my main gripe is the inconsistency between [unstore_unit] and [modify_unit].

Anyway, thanks for the help, beetlenaut! It may not be exactly what I wanted, but I'm still better off than I started.

[lhybrideur]

The problem with merging [modify_unit] and [teleport/move_unit] is that the first rebuilds the unit, while the seconds do not.
And that it would be using a cannon to kill a fly.

[Celtic_Minstrel]

While I can kind of understand that [modify_unit], [teleport], and [move_unit] have some overlaps in what they do, they are basically three different things. (Although, [move_unit] is technically redundant; more on that in a moment.)

• [modify_unit] modifies the unit's stats and variables.
• [teleport] moves the unit to another location.
• [move_unit] is a redundant wrapper that combines [move_unit_fake] with [teleport].
• [move_unit_fake] plays a movement animation across a specified series of hexes.

So, in essence, the entire point of [move_unit] is to animate a unit moving to another tile (and actually move it to that tile as well). Therefore, it's obviously not what you need here. Although [modify_unit] can also move the unit, that's not really its purpose – it's kind of more of a side-effect of how it's implemented. (Although, I don't expect that the ability would ever be removed even if the implementation changes, as I'm sure people depend on it.) Therefore, [teleport] is the correct answer.

But as you've noticed, it doesn't support moving multiple units in a batch. Unfortunately, there's nothing you can do about that within the confines of WML – you'll have to loop and teleport them one-by-one, as you've noticed. If you can stomach a little Lua though, it's not that hard to make a version of [teleport] that can handle multiple units. In essence, the loops you were describing end up inside the tag's implementation, instead of in your WML.

About [modify_unit] implementing passability checks, I agree that there's no good reason to do this. [unstore_unit] implements them for the same reason that [unit] does – it's spawning a brand-new unit on the map. But [modify_unit] is modifying a unit in-place on the map. Other than exotic cases where you modify the unit's movement costs (or its location), it's not going to end up on an impassable space all of a sudden. And if you're in one of those more exotic cases, it can't be that hard to do the check yourself manually… or in the movement case, use [teleport].