Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add either heals= or [heals] to Standard Location Filter #4233

Open
wants to merge 1 commit into
base: master
from

Conversation

@stevecotton
Copy link
Contributor

commented Aug 15, 2019

One of the features requested in #4227.

The core terrains' villages and oases all heal 8 HP per turn, but the
engine supports other values. So the expected usage to match all
healing terrains is:

heals=1-{INFINITY}

This doesn't support filtering for negative values of heals, however
I believe no-one is using them (see #4232 ). A harms= key could be
added, but if this is needed then maybe this PR should add a [heals]
child to SLF. Or ranges could be expanded to support negative numbers.

@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

I would expect most common expectation would be heals=yes

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

I agree that that will be the common expectation, but it's simple to document that it's a range, and I would hope that UMC authors might use the customizability for custom terrains during the 1.15/1.16 cycle.

@AI0867

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

People will still likely use heals=yes, and may not care about the specific value, given that in most situations, the only value that appears is that of a village.

Therefore, I propose that heals=yes be a special case or a shorthand for heals=1-{INFINITY}.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

It makes sense, but at the same time no other filter works like that (AFAIK), so I'm a bit torn between supporting or not supporting "yes" and "no". What would you feel about making it [heals]amount=, where the amount defaults to 1-infinity, similar to [filter_adjacent_location]count=?

@jostephd

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Therefore, I propose that heals=yes be a special case or a shorthand for heals=1-{INFINITY}.

I don't like special cases. Having something like this means that every addon that has lua code that handles WML will need to implement parsing the special cases. I would prefer @stevecotton's idea of [heals] amount=.. with amount defaulting to "any value" if omitted. Another idea (perhaps not as good, but I'll throw it out there anyway) is to let heals take a range with optional endpoint, like git commit ranges: heals=5.. would mean heals >= 5, heals=..5 would mean 1 <= heals <= 5 , and heals=.. would mean 1 <= heals <= infinity.

Regardless of which idea we choose, I would recommend to figure out now what the syntax for filtering negative heals values would be, so if this issue is fixed before #4232, we won't have to figure out support for filtering negative values with the added burden of compatibility with the fix to this issue.

@jostephd

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Regardless of which idea we choose, I would recommend to figure out now what the syntax for filtering negative heals values would be, so if this issue is fixed before #4232, we won't have to figure out support for filtering negative values with the added burden of compatibility with the fix to this issue.

Actually, how about supporting a heals attribute or [heals] key with implicit boundaries of 1 and infinity, and a harms attribute or [harms] key with implicit boundaries of minus infinity and -1, and... something... for terrains that don't give healing? Maybe that would be just [not][harms][/harms] [heals][/heals][/not] though.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

To match terrains that don't give healing, I suggest heals=0 or amount=0.

For matching harming terrains, how about extending the range syntax (utils::parse_range) to allow:

  • 2-3 as now, matches 2,3
  • -2--1 matches -2,-1
  • -2-1 matches -2,-1,0,1

I've a bad feeling that -2 is currently parsed as 0-2 though.

@AI0867

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Using - for both negation and separating the ends of a range is like asking for confusion and mistakes.

An alternative would be a harms=range key. That would make matching ranges like -3..3 harder, but I don't think that will be a common use case.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Although -3..3 is an unlikely use case, it sounds a good test case and documentation case. How about this:

[heals]
    amount=0-3
[/heals]
[or]
    [harms]
        amount=0-3
    [/harms]
[/or]

Either (but not both) of the 0-3 ranges could be replaced with 1-3, with the same overall effect.

@slavrenyuk, what do you think about this?

@stevecotton stevecotton force-pushed the stevecotton:slf_heals branch 2 times, most recently from 8e5d74c to 8f38f3e Aug 18, 2019

@slavrenyuk

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@stevecotton

[heals]
    amount=0-3
[/heals]
[or]
    [harms]
        amount=0-3
    [/harms]
[/or]

I like this approach.

As I understand

[heals]
[/heals]

is the same as

[heals]
  amount=1-infinity
[/heals]

I would ask to document edge case where amount=0. From user perspective, it is not obvious if [harms]amount=0[/harms] matches terrain that provides healing or not. It could be read as "terrain that doesn't harm, no matter if it heals or not". And vice versa for zero heals amount.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Honestly, I don't like the idea of using a subtag for something like this, and even less the idea of using a separate subtag for negatives, but if you're gonna use a subtag anyway, why not support separate min and max keys instead of range syntax? So basically...

[heals]
    amount=8
[/heals]

That would only match standard villages/oases that heal 8 HP, but...

[heals]
    min=-3
    max=3
[/heals]

That would match terrains that "heal" -3..3 HP.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Using a subtag seems necessary to me, as otherwise we'll want to support both heals=yes and heals=<range>.

Assuming that we do use a subtag, there seems consensus that [heals][/heals] with no attributes should match terrains that heal 1-infinity hp, no matter whether that will eventually be implemented as [heals]amount=1-99999, [heals]amount=1.. or [heals]min=1, and no matter whether there will also be a [harms] subtag.

My feeling is that adding the [heals] key with no attributes would be worthwhile for 1.15.1 or 1.15.2.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

I would expect the following to be equivalent:

[heals][/heals]
[heals]min=1[/heals]
[harms]max=0[/harms]
[not][harms]min=1[/harms][/not]

etc. (I probably made an error or two in my logic, but you get the idea)

I don't like "amount=1...99999" even though we see it in existing code, because it's a bug laying in wait. "amount=1..." seems harder to parse and error prone (too visually similar to "amount=1").

@slavrenyuk

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@GregoryLundberg

I don't like "amount=1...99999" even though we see it in existing code, because it's a bug laying in wait.

Could you explain that? The bug is related to the upper bound and will happen if heals is more than 99999?

@slavrenyuk

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

Btw, do you see any use cases that requires filtering on healing amount? I don't, it is too exotic imo. Could we stick with heals=yes/no and harms=yes/no (I could actually imagine interesting use cases for harming terrain). And update or deprecate/remove this in case if there will be found important use case that requires filtering on healing amount.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

I don't like "amount=1...99999" even though we see it in existing code, because it's a bug laying in wait. "amount=1..." seems harder to parse and error prone (too visually similar to "amount=1").

How about adding a special-case to utils::parse_range to support infinity so that something=1-infinity is valid WML? However, that would suggest that special-casing yes and no in the same code might also be worthwhile.

@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

How you put the ((number)-(number or infinity)) into schema validator?

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

How you put the ((number)-(number or infinity)) into schema validator?

Argh, SchemeWML really needs more documentation, and I don't want to take on that responsibility. But, like this:

diff --git a/data/schema/game_config.cfg b/data/schema/game_config.cfg
index 0b0dce53712..be2b289710b 100644
--- a/data/schema/game_config.cfg
+++ b/data/schema/game_config.cfg
@@ -72,6 +72,7 @@
             [/type]
         [/union]
     [/type]
+    # definition of range_list and s_range_list, before macros are expanded
     {LIST_TYPE_COMPLEX range (
         [element]
             [list]
@@ -83,6 +84,9 @@
                 [/element]
             [/list]
         [/element]
+        [element]
+            value="\d+-infinity"
+        [/element]
     )}
     [type]
         name=alignment
@ProditorMagnus

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

Was just wondering if it is needed to bring new syntax into language.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Btw, do you see any use cases that requires filtering on healing amount? I don't, it is too exotic imo. Could we stick with heals=yes/no and harms=yes/no (I could actually imagine interesting use cases for harming terrain). And update or deprecate/remove this in case if there will be found important use case that requires filtering on healing amount.

At the moment, only for filtering messages saying "ouch, this floor is hot", "ouch, this floor is burning", etc (probably followed by the narrator saying exactly what the stats are). However, I'd prefer to have the ability to add support for it without deprecating an old version.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Using a subtag seems necessary to me, as otherwise we'll want to support both heals=yes and heals=<range>.

Well, personally I don't see an issue with this syntax, but whatever...

Assuming that we do use a subtag, there seems consensus that [heals][/heals] with no attributes should match terrains that heal 1-infinity hp

Yeah, it makes sense for the default to be "any positive value".

Argh, SchemeWML really needs more documentation, and I don't want to take on that responsibility.

Uhh... is there something wrong with the wiki page?

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

Uhh... is there something wrong with the wiki page?

The "finding the definition of a type" section was written after my previous message. ;-)

A general issue with the wiki page is that the schema makes heavy use of macros, but these aren't documented on the wiki.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

My feelings about handling healing and harming similarly have changed - I now think that we should treat them as two separate features. More reasoning in #4232 (comment) .

Fire-based terrains seem an obvious example, and players' intuition says that drakes would take less damage from it than other races, and fire elementals probably no damage at all. I think [SLF][harms] would end up being a complex question that depends on which unit would be on the hex.

@slavrenyuk

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

@stevecotton

Fire-based terrains seem an obvious example, and players' intuition says that drakes would take less damage from it than other races, and fire elementals probably no damage at all.

Are you talking about different damage types similarly to the [harm_unit] damage_type attribute https://wiki.wesnoth.org/DirectActionsWML#.5Bharm_unit.5D ?

Add [heals] to the Standard Location Filter
The core's villages and oases all heal 8 HP per turn, but the engine and
TerrainWML support terrains with other values. The new [heals] is a key instead
of an attribute so that filtering based on the healing amount can be added (if
wanted), without breaking WML that already uses [heals].

@stevecotton stevecotton force-pushed the stevecotton:slf_heals branch from 8f38f3e to 3f00451 Aug 21, 2019

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Elsewhere we're talking about [harms] tags as well. I've not kept up with the current state of things but if we're adding heal= we probably want the inverse harms= as well.

@stevecotton stevecotton changed the title Add heals= to Standard Location Filter Add either heals= or [heals] to Standard Location Filter Aug 21, 2019

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

I have no objection to adding [harms], but it should be a separate PR, with the current PR simply avoiding anything that would have to be deprecated to add [harms].

@slavrenyuk

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Just in case - currently amount key inside the [heals] tag is not supported in the PR. Otherwise, it looks good.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

There's literally no point in adding a tag here if it can't have any content. If you want to go that route, just make it a key. If you really want a tag, it should have at least two possible subkeys (which may be mutually exclusive).

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

The point in making it a tag is that I later want to add amount= or min,max= attributes, but the details of those attributes could still be discussed after the basic feature is implemented. It also avoids a dependency on #4255 for the current PR.

To make it an attribute while still adding support for filtering by amount requires supporting the heals=yes|no|1-7 syntax, which both @jostephd and I feel is an unnecessary special case.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Personally I'd prefer the tag and its attributes to be merged at the same time...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.