made tag_name is mandatory in [experimental_filter_ability(_active)] but remain optionnal in overwrite_specials[overwrite][experimental_filter_specials]

[newfrenchy83]

Fix #7992

[newfrenchy83]

@stevecotton could you review this PR, please?

[soliton-]

This looks like it's going in the right direction.

It would be helpful to have more explanations on what this does then that it's supposed to fix two issues. It's not great if there is just a code dump to guess the intention from.

I don't see how this addresses #7926 at all.

[newfrenchy83]

This looks like it's going in the right direction.

It would be helpful to have more explanations on what this does then that it's supposed to fix two issues. It's not great if there is just a code dump to guess the intention from.

I don't see how this addresses #7926 at all.

only for [filter_wml], for activity of abilities, it must using tag name of filter for don't add subtag in subtags, change name of tag is more easy for develloper what unique tag_name, 'active', and a subtag for check attributes

[stevecotton]

I'd like to avoid adding knowledge of abilities to the engine, or adding it to parts of the engine that don't currently have it (as in, avoid copying logic into more places). However, I'm happy to put some of the burden on people writing filters, because if they're filtering for specific properties of an ability then I assume they know something about that ability.

Which filters require this PR's current logic, but couldn't be written with the following set of rules (and applying them similarly to things other than value) which do not require and special handling based on the tag name:

• The filter must specify the tag_name itself
• Add support for value=default or value=unset, so that a filter for [heals]value=default,8 would match [heals][/heals] and [heals]value=8[/heals]
• When matching value, ignore add, sub, multiply and divide, regardless of tag name
• Allow filtering for calculated_value, which will take the value and apply add, sub, multiply and divide, however it will assume that the ability supports those without checking the tag name - it's the filter-author's problem if that tag doesn't support those. This would need some way for the filter to supply the default value too, so that the engine doesn't have to hardcode the starting value to apply that arithmetic to.

[newfrenchy83]

I'd like to avoid adding knowledge of abilities to the engine, or adding it to parts of the engine that don't currently have it (as in, avoid copying logic into more places). However, I'm happy to put some of the burden on people writing filters, because if they're filtering for specific properties of an ability then I assume they know something about that ability.

Which filters require this PR's current logic, but couldn't be written with the following set of rules (and applying them similarly to things other than value) which do not require and special handling based on the tag name:

* The filter must specify the `tag_name` itself

* Add support for `value=default` or `value=unset`, so that a filter for `[heals]value=default,8` would match `[heals][/heals]` and `[heals]value=8[/heals]`

* When matching `value`, ignore `add`, `sub`, `multiply` and `divide`, regardless of tag name

* Allow filtering for `calculated_value`, which will take the `value` and apply `add`, `sub`, `multiply` and `divide`, however it will assume that the ability supports those without checking the tag name - it's the filter-author's problem if that tag doesn't support those. This would need some way for the filter to supply the default value too, so that the engine doesn't have to hardcode the starting value to apply that arithmetic to.

in fact I would like not to add the recognition of the type of ability in the check of this or that attribute, and the proposals that you make concerning the numerical attributes amply justify the simplification of a code that I have made too complex in this PR and reworked on the digital attributes.

I still think to keep the tag_name check for certain attributes if it is specified in the filter (filter instead of cfg) as you suggest (damage_type attribute or plague), but instead of returning a false value, it there will be no check of the attribute but the check of the val attribute

[soliton-]

I'd like to avoid adding knowledge of abilities to the engine, or adding it to parts of the engine that don't currently have it (as in, avoid copying logic into more places). However, I'm happy to put some of the burden on people writing filters, because if they're filtering for specific properties of an ability then I assume they know something about that ability.

If we change to [filter_ability] [<ability type>] <ability specific key>=... then the schema can validate that under [drains] for example only the supported tags are used. IMO the feature to match a list of tag names is obscure anyway so no loss to remove that. And I do agree that the tag name should be mandatory.

[newfrenchy83]

If we change to [filter_ability] [<ability type>] <ability specific key>=... then the schema can validate that under [drains] for example only the supported tags are used. IMO the feature to match a list of tag names is obscure anyway so no loss to remove that. And I do agree that the tag name should be mandatory.

i don't agree with that because this code is used in [overwrite][filter_specials] and in this case the type of abiliti/special filteredis same what abilitie wo using overwrite_specials

[newfrenchy83]

another idea would be to remove overly specific attribute filters like 'type' or 'alternative_type' and use the filter_wml instead, but making it mandatory to use tag_name in the filter is too restrictive

[newfrenchy83]

@stevecotton
my reply for your suggestions are:

• The filter must specify the tag_name itself: if a developer uses
  [filter_ability]
  tag_name=poison
  value=20

and that a special [poison] has value=20 even if this attribute has nothing to do there then the filter will match and making the tag_name obligatory will not solve the problem and will add a constraint for nothing hence the idea of exclude [poison] or other from the 'value' check instead.

* When matching `value`, ignore `add`, `sub`, `multiply` and `divide`, regardless of tag name

the problem and that it is possible to functionally encode both 'value' and 'add' for example, and therefore prohibit the check of 'add' if 'value' match already is not relevant enough since even if this is an extreme case we can use them in the same ability

* Allow filtering for `calculated_value`, which will take the `value` and apply `add`, `sub`, `multiply` and `divide`, however it will assume that the ability supports those without checking the tag name - it's the filter-author's problem if that tag doesn't support those. This would need some way for the filter to supply the default value too, so that the engine doesn't have to hardcode the starting value to apply that arithmetic to.

the filter checks each ability one after the other, I just can't encode 'calculated_value'

* Add support for `value=default` or `value=unset`, so that a filter for `[heals]value=default,8` would match `[heals][/heals]` and `[heals]value=8[/heals]`

it's interesting, it's just the change in the validation scheme that bothers me a little, I'm going to work on it

[soliton-]

i don't agree with that because this code is used in [overwrite][filter_specials] and in this case the type of abiliti/special filteredis same what abilitie wo using overwrite_specials

An implementation detail of some other tag has pretty much no bearing on the design of this tag. Sounds like the implementation is trivially adaptable anyway.

[soliton-]

Please move the filter_wml addition to a different PR. I'd like to keep the PRs as small as possible.

Please move matches_ability_filter back to where it was or explain why it was moved.

[stevecotton]

* The filter must specify the `tag_name` itself: if a developer uses
  [filter_ability]
  tag_name=poison
  value=20

and that a special [poison] has value=20 even if this attribute has nothing to do there then the filter will match and making the tag_name obligatory will not solve the problem and will add a constraint for nothing hence the idea of exclude [poison] or other from the 'value' check instead.

Why is that a problem for filtering? The poison will only do 8 damage, and the author will eventually realise that value=20 isn't doing anything in the ability; however that's not related to filtering. Why shouldn't a filter looking for [poison]value=20 match an ability [poison]value=20?

[newfrenchy83]

Please move the filter_wml addition to a different PR. I'd like to keep the PRs as small as possible.

Please move matches_ability_filter back to where it was or explain why it was moved.

Please move the filter_wml addition to a different PR. I'd like to keep the PRs as small as possible.

Please move matches_ability_filter back to where it was or explain why it was moved.

the code was moved to config_filter to be able to be called from unit:: functions as well as attack_type:: functions and the transfer to config_filter is done in the interest of fairness with regard to these two types of functions (why a function called from attack_type should stay in unit.cpp?)

[newfrenchy83]

* The filter must specify the `tag_name` itself: if a developer uses
  [filter_ability]
  tag_name=poison
  value=20

and that a special [poison] has value=20 even if this attribute has nothing to do there then the filter will match and making the tag_name obligatory will not solve the problem and will add a constraint for nothing hence the idea of exclude [poison] or other from the 'value' check instead.

Why is that a problem for filtering? The poison will only do 8 damage, and the author will eventually realise that value=20 isn't doing anything in the ability; however that's not related to filtering. Why shouldn't a filter looking for [poison]value=20 match an ability [poison]value=20?

because poison don't use numerical value(it a boolean value)?

[newfrenchy83]

someone wants me to add such and such a thing, another wants me to remove [filter_wml] just to empty this PR of its substance if the attribute legality filter is not approved, try to get you to agree @stevecotton et @soliton-

I am willing to renounce the principle of 'legality' but [filter_wml] will remain in place

[soliton-]

someone wants me to add such and such a thing, another wants me to remove [filter_wml] just to empty this PR of its substance if the attribute legality filter is not approved, try to get you to agree @stevecotton et @soliton-

I am willing to renounce the principle of 'legality' but [filter_wml] will remain in place

The point is that the filter_wml part seems trivial and can probably be handled in short order instead of dragging it around until the remaining issues in this PR are addressed. I have not seen any disagreement about that. If you rather want to move everything else to a different PR that makes no difference to me.

As a general note it would certainly be soooo much better if there was agreement on how the things you implement should work before you even write the first line of code. So the introduction of [filter_ability] should have started with a clear description of how it should work and only once that is agreed upon the coding should start. Instead in my experience it's 1) code dump, 2) someone tries to decipher the code to guess the goal and 3) tries to review based on that guess.

[newfrenchy83]

@stevecotton and @soliton-

Here's what I'm going to do to complete the code

1 add the 'default' value to filter value to be able to check the default value of an ability without having to add its value (50 for drains or 0 for heals)

2 make the use of tag_name in the filter mandatory except in the case of [overwrite][experimental_filter_specials] where the type of ability checked is always that of the ability using overwrite_specials

I hope this suits you both

[stevecotton]

@soliton- 's point of "please discuss before a code dump" still stands - I don't think those changes will answer the question of what this is fixing, and they won't solve my worries (the rest of this comment) about adding special cases to the engine.

because poison don't use numerical value(it a boolean value)?

Why does the engine's filter implementation need to know that?

If someone is trying to use an ability with [poison]value=20 then there are multiple ways that they can discover that the value doesn't affect the amount of damage taken. The behavior of the engine's filter implementation (whether it allows it or not) is unlikely to matter in how quickly they find the bug and fix it.

However, consider the effect of incorrect knowledge in the engine's filter implementation; for example if [regenerate] was incorrectly added to the list of things that only use boolean value. It's easy to write a unit test that still misses that, and only find the bug after the next stable release has frozen the API.

[newfrenchy83]

@soliton- 's point of "please discuss before a code dump" still stands - I don't think those changes will answer the question of what this is fixing, and they won't solve my worries (the rest of this comment) about adding special cases to the engine.

because poison don't use numerical value(it a boolean value)?

Why does the engine's filter implementation need to know that?

If someone is trying to use an ability with [poison]value=20 then there are multiple ways that they can discover that the value doesn't affect the amount of damage taken. The behavior of the engine's filter implementation (whether it allows it or not) is unlikely to matter in how quickly they find the bug and fix it.

However, consider the effect of incorrect knowledge in the engine's filter implementation; for example if [regenerate] was incorrectly added to the list of things that only use boolean value. It's easy to write a unit test that still misses that, and only find the bug after the next stable release has frozen the API.

@soliton- 's point of "please discuss before a code dump" still stands - I don't think those changes will answer the question of what this is fixing, and they won't solve my worries (the rest of this comment) about adding special cases to the engine.

because poison don't use numerical value(it a boolean value)?

Why does the engine's filter implementation need to know that?

If someone is trying to use an ability with [poison]value=20 then there are multiple ways that they can discover that the value doesn't affect the amount of damage taken. The behavior of the engine's filter implementation (whether it allows it or not) is unlikely to matter in how quickly they find the bug and fix it.

However, consider the effect of incorrect knowledge in the engine's filter implementation; for example if [regenerate] was incorrectly added to the list of things that only use boolean value. It's easy to write a unit test that still misses that, and only find the bug after the next stable release has frozen the API.

I have already removed the loops for special cases as well as certain attributes that are too specific to return to the simplest, I still have to put what you suggested (compulsory tag_name and 'value'=default)

[newfrenchy83]

I've already mentioned factoring out the filter_wml part into its own PR since it can likely be merged in a day simplifying this PR.

As far as I understood the utils::config_filters namespace the matches_ability_filter function doesn't make sense there. It should contain helper functions for filters not the complete filter logic.

The biggest part needed is figuring out how exactly [filter_ability] should work. IMO #7926 shows how the syntax should look so what remains is figuring out the semantics. The documentation at https://wiki.wesnoth.org/StandardAbilityFilter looks like a good start. I need to read that a couple times to see if it makes sense to me and then I could review the code against the documentation.

I actually removed filter_wml from this PR and moved the namespace into abilities.cpp, as for your proposal to change the syntax with

[filter_ability]
active = yes | no
type = heals | regenerate | ...
type_value (really needs a better name) = add | sub | ...
[ability]
affect_self = yes | no
...
[/ability]
[/filter_ability]

the answer is no, the only thing that could justify it is the activity check in addition to the tag_name except that there is only one case where we can check an ability possessed by a unit or an influenced unit by it, it is [filter][filter_ability(_active] in the events or ability.

In [overwrite][experimental_filter_specials] the abilities/specials are necessarily active, in [effect]apply_to_=attack[remove_specials], which is still in PR, the special check is encoded in the attack without regard for its activity and in [filter_weapon ][filter_special_active] only active specials are checked (no non-active equivalent as for [filter_ability]). see #7814

As for type_value , I seem to have already removed it so if only tag_name remains, it's not worth it.

[newfrenchy83]

type_value removed by @stevecotton in 30eb439 commit

[newfrenchy83]

other suggestion?

[newfrenchy83]

@stevecotton @soliton- don't forget this PR, please

[newfrenchy83]

@soliton- @stevecotton I don't want to seem like I'm rushing you but I would like to be able to add [filter_wml] and since I'm supposed to do it in another PR, I need this one to be merged into master before I can do that and rebase
https: //github.com//pull/8546