Is this a bug or intended?

The place to post your WML questions and answers.

Moderators: Forum Moderators, Developers

Forum rules
  • Please use [code] BBCode tags in your posts for embedding WML snippets.
  • To keep your code readable so that others can easily help you, make sure to indent it following our conventions.
Post Reply
User avatar
beetlenaut
Developer
Posts: 2390
Joined: December 8th, 2007, 3:21 am
Location: Washington State
Contact:

Is this a bug or intended?

Post by beetlenaut » May 27th, 2014, 11:50 am

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?
Campaigns: Dead Water,
The Founding of Borstep,
Secrets of the Ancients,
and WML Guide

User avatar
iceiceice
Developer
Posts: 1056
Joined: August 23rd, 2013, 2:10 am

Re: Is this a bug or intended?

Post by iceiceice » May 27th, 2014, 1:33 pm

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

Re: Is this a bug or intended?

Post by Anonymissimus » May 27th, 2014, 2:40 pm

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.
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

Max
Posts: 1449
Joined: April 13th, 2008, 12:41 am

Re: Is this a bug or intended?

Post by Max » May 27th, 2014, 5:39 pm

[store_locations] clearly states:
...The array will include any unreachable border hexes, if applicable
[store_reachable_locations] sounds like an easy workaround...

User avatar
beetlenaut
Developer
Posts: 2390
Joined: December 8th, 2007, 3:21 am
Location: Washington State
Contact:

Re: Is this a bug or intended?

Post by beetlenaut » May 27th, 2014, 6:26 pm

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.
iceiceice wrote: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.
Campaigns: Dead Water,
The Founding of Borstep,
Secrets of the Ancients,
and WML Guide

User avatar
iceiceice
Developer
Posts: 1056
Joined: August 23rd, 2013, 2:10 am

Re: Is this a bug or intended?

Post by iceiceice » May 27th, 2014, 6:34 pm

beetlenaut wrote: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?

User avatar
beetlenaut
Developer
Posts: 2390
Joined: December 8th, 2007, 3:21 am
Location: Washington State
Contact:

Re: Is this a bug or intended?

Post by beetlenaut » May 27th, 2014, 7:48 pm

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.
Campaigns: Dead Water,
The Founding of Borstep,
Secrets of the Ancients,
and WML Guide

User avatar
ivanovic
Lord of Translations
Posts: 1146
Joined: September 28th, 2004, 10:10 pm
Location: Germany

Re: Is this a bug or intended?

Post by ivanovic » May 27th, 2014, 8:08 pm

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.

User avatar
beetlenaut
Developer
Posts: 2390
Joined: December 8th, 2007, 3:21 am
Location: Washington State
Contact:

Re: Is this a bug or intended?

Post by beetlenaut » June 1st, 2014, 12:12 pm

ivanovic wrote: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.)
Campaigns: Dead Water,
The Founding of Borstep,
Secrets of the Ancients,
and WML Guide

User avatar
iceiceice
Developer
Posts: 1056
Joined: August 23rd, 2013, 2:10 am

Re: Is this a bug or intended?

Post by iceiceice » June 1st, 2014, 12:54 pm

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

Re: Is this a bug or intended?

Post by Anonymissimus » June 1st, 2014, 2:35 pm

beetlenaut wrote:
ivanovic wrote: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.
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
beetlenaut
Developer
Posts: 2390
Joined: December 8th, 2007, 3:21 am
Location: Washington State
Contact:

Re: Is this a bug or intended?

Post by beetlenaut » June 2nd, 2014, 12:25 am

Anonymissimus wrote: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.
Campaigns: Dead Water,
The Founding of Borstep,
Secrets of the Ancients,
and WML Guide

User avatar
iceiceice
Developer
Posts: 1056
Joined: August 23rd, 2013, 2:10 am

Re: Is this a bug or intended?

Post by iceiceice » June 2nd, 2014, 12:29 am

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

Re: Is this a bug or intended?

Post by Anonymissimus » June 2nd, 2014, 1:16 pm

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).
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

Post Reply