Is this a bug or intended?

[beetlenaut]

If you store a location on the edge of the map with radius=1, the border hexes get stored as well even though they are not usable parts of the map.

For example, if you do:

Code: Select all

[store_locations]
	variable=loc
	x,y=1,15
	radius=1
[/store_locations]

loc will contain hexes (0,15) and (0,14). This seems like a bug. Is it?

[iceiceice]

beetlenaut: I just looked at the pathfinder code recently. From what I remember, most of the pathfinding stuff (esp. in lua) has an optional "with border" argument. I guess it's been made optional since we decided to phase out the map borders? For example:

https://github.com/wesnoth/wesnoth/blob ... r.lua#L298

However some of the lua functions just don't have this... The implementation of store_locations is here, using wesnoth.get_locations:

https://github.com/wesnoth/wesnoth/blob ... s.lua#L532

wesnoth.get_locations is here. the "with_border" argument to terrain filter is always set to true:

https://github.com/wesnoth/wesnoth/blob ... .cpp#L3130

https://github.com/wesnoth/wesnoth/blob ... r.cpp#L380

So I guess the right fix is to add "with_border" parameters to more of the lua tags and propogate them down? I don't know if it's a bug rather than a design flaw / oversight, hard to say.

Edit: An alternative is, add "with_border" as a boolean parameter to location filters? That makes more sense imo than adding boolean parameters to all of these functions.

[Anonymissimus]

Hexes with x=0 or y=0 (That way one can include/exclude them from SLFs, using [not] as neccessary.) are needed for modifying shroud/fog and/or terrain based on location filters. It has visual effect and is very likely the reason for including them. It's only that units cannot be placed there.
In the past, there have been black edges at times, in scenarios which remove shroud over stored locations. It was considered a bug.

[Max]

[store_locations] clearly states:

...The array will include any unreachable border hexes, if applicable

[store_reachable_locations] sounds like an easy workaround...

[beetlenaut]

Oh. Check the wiki. Right.

This is causing a bug in NR. When a white mage dies, and the other is near the edge, the dying mage can be told to respawn on a border hex. It vanishes from the game at that point. [store_reachable_hexes] wouldn't work either if the unit was surrounded by enough enemies, which I'm sure is possible in NR. But, check_passability in [unstore_unit] would probably work well enough in this case.

An alternative is, add "with_border" as a boolean parameter to location filters?

Yes, that would make it easier. Otherwise, you might have to store the dimensions and check for out of bounds results in both directions, which is kind of tedious.

[iceiceice]

Otherwise, you might have to store the dimensions and check for out of bounds results in both directions, which is kind of tedious.

That's pretty easy though. http://wiki.wesnoth.org/InternalActions ... ensions.5D

I guess we could add with_border to location filters for 1.12... but there has been anxiety lately about making WML syntax changes at this point, and that would be kind of a big one. Is this really going to be very inconvenient?

[beetlenaut]

I'm not the maintainer and wasn't planning on fixing this, so it's not at all inconvenient to me!

If I counted right, it's 14 lines of code. It's not bad, but a little annoying. Does something really count as a big change to WML if it's optional and defaults to the current behavior? I'm just asking in general.

[ivanovic]

There should have been NO syntax changes since the start of the beta phase since those tend to heavily break compatibility. Changes are only allowed to fix bugs, not completely alter features...

In case you really need a syntax change to fix a bug, please send a mail to the dev-ml. If there are other ways, then that is fine. If you just want to alter things to make them "better": do so in master but not in the 1.12 branch.

[beetlenaut]

If you just want to alter things to make them "better": do so in master but not in the 1.12 branch.

Yeah, I didn't mean for 1.12.

I have another one: According to the wiki, standard unit filters always check on the recall list. However, neither [transform_unit] nor [modify_unit] work unless the unit is on the map. They both ignore the recall list. Is this a bug or incorrect information on the wiki? (You can use [store] and [unstore] to work around this, but I figured the other tags existed to make that unnecessary.)

[iceiceice]

beetlenaut: I think the concept of "standard unit filter" only actually exists in the implementation for wml tags implemented in lua. For wml tags implemented in C++, afaik "standard unit filter" doesn't really mean anything at the level of the code, so you should assume that there will be slight differences.

So I guess the answer is, probably we intended to eventually write those two in lua, and then it would have automatically worked the way the other filters do. Or, we should backport the standard unit filter handler to C++ and then make the lua tags call the C++ version.

That doesn't address your question of whether the *behavior* is a bug -- I would say that strictly speaking if it conflicts with the docs in a clear-cut way like that, then it's a bug. But maybe not a very important one.

If there's a bug in the standard unit filter for one of the tags implemented in lua, then that's actually much more annoying because it would mean that most if not all of the tags have the same bug. (Assuming that I have understood correctly.)

[Anonymissimus]

According to the wiki, standard unit filters always check on the recall list.

AFAIK and IMHO this is wrong. Only a few tags filtering on units include the recall list instead, and their respective documentation should mention that. For instance, have_unit never filtered on recall list units until it got implemented (the search_recall_list= attribute).

This is so since on-map units and recall list units are different data structures inside of the C++ engine, filtering both needs additional code.

EDIT
An example is a SUF in a moveto event - recall list units cannot move, it doesn't make sense to filter on them. So it's not possible to say a SUF should generally also filter on recall list units.

[beetlenaut]

Only a few tags filtering on units include the recall list

Is there a way to tell which is which short of testing every single one? I might find the time to change the wiki, but I don't know which tags do include the recall list.

[iceiceice]

I think you should just skim the code.

If not all of them, I believe that substantially all of the wml tags are implemented either in this file:

https://github.com/wesnoth/wesnoth/blob ... on_wml.cpp

or this file

https://github.com/wesnoth/wesnoth/blob ... l-tags.lua

so just search for the one you want.

[Anonymissimus]

It seems that in the past I followed the "rule" I mentioned above, that is, by default, a SUF filters only on-map units, and I documented it at some places in the wiki if a tag includes the recall list, for the ones I wrote or know such as modify_unit. That way it's not necessary to write that it excludes it but only it includes it (in case it does for the tag in question).