Add modification count and ids in formula(postpone 1.19)

[newfrenchy83]

Can resolve https://github.com/wesnoth/wesnoth/issues/8179 issue

while waiting for the release of the use of the number of advancement or their identity to be decided, I added both in the formulas

[CelticMinstrel]

It looks like you added a function that already exists – modification_count is already defined. Also, you consistently misspelled "length", but that doesn't really matter so much because you shouldn't really be using that word for this anyway. Use "count" instead, like the pre-existing function does.

[newfrenchy83]

It looks like you added a function that already exists – modification_count is already defined. Also, you consistently misspelled "length", but that doesn't really matter so much because you shouldn't really be using that word for this anyway. Use "count" instead, like the pre-existing function does.

the preexisting function only works by counting modifications of a given type and with a specified id whereas here, we want to count all modifications of this type

[CelticMinstrel]

I don't think I have any remaining objections here… we should add something like this to the Lua API too, but that can be done separately, as we probably want it to be a little more sophisticated.

[newfrenchy83]

I don't think I have any remaining objections here… we should add something like this to the Lua API too, but that can be done separately, as we probably want it to be a little more sophisticated.

i added something to lua, it is that what you wanted?

[CelticMinstrel]

About the name, I think I like "advancements_taken" better than "advancements_used".

i added something to lua, it is that what you wanted?

No, not even close. The Lua API should be more sophisticated, as I mentioned. I have some ideas of what I'd like it to look like, but returning a list of IDs is definitely not that.

[newfrenchy83]

About the name, I think I like "advancements_taken" better than "advancements_used".

i added something to lua, it is that what you wanted?

No, not even close. The Lua API should be more sophisticated, as I mentioned. I have some ideas of what I'd like it to look like, but returning a list of IDs is definitely not that.

then i will remove commit and work in lua for another PR

[newfrenchy83]

No, not even close. The Lua API should be more sophisticated, as I mentioned. I have some ideas of what I'd like it to look like, but returning a list of IDs is definitely not that.

What ideas was be? It is for another PR after merging of this.

[CelticMinstrel]

Well, here's a partial list of things that I wish we could do in Lua with respect to modifications.

Expression	Meaning
#unit.modifications	Total number of modifications on the unit (all types)
#unit.modifications.traits	Total number of traits on the unit
#unit.modifications.objects	Total number of objects on the unit
#unit.modifications.advancements	Total number of advancements on the unit
unit.modifications[n]	The nth modification on the unit (regardless of type) – returns a "modification object"
unit.modifications.foo	The first (or maybe last?) modification with id=foo – returns a "modification object"
unit.modifications.traits[n]	The nth trait on the unit – returns a "modification object"
unit.modifications.traits.foo	The first (or maybe last?) trait with id=foo – returns a "modification object"
unit.modifications.objects[n]	The nth object on the unit – returns a "modification object"
unit.modifications.objects.foo	The first (or maybe last?) object with id=foo – returns a "modification object"
unit.modifications.advancements[n]	The nth advancement on the unit – returns a "modification object"
unit.modifications.advancements.foo	The first (or maybe last?) advancement with id=foo – returns a "modification object"
#modification.effects	The number of effects on the given modification
modification.effects[n]	The nth effect on the given modification
modification.effects.attack[n]	The nth effect with apply_to=attack on the given modification
modification.__cfg	The raw config of the modification (which includes the effect tags)

[newfrenchy83]

@CelticMinstrel and how using list of IDs in formulas?

[CelticMinstrel]

The list of IDs seems okay for formulas. I think the primary use-case for it would be a formula like reduce(advancements_taken, 0, if(b = 'amla_default', a + 1, a)) to count how many times a specific AMLA has been taken.

[gfgtdf]

I really think if a name of a property like modifications exists in formula and in both lua it should also do the same thing in both. Everything else it just confusing for the umc devs to remember. If we intend to to something else with modifications in lua then we should also use a different name for that property in wfl.

[CelticMinstrel]

I really think if a name of a property like modifications exists in formula and in both lua it should also do the same thing in both. Everything else it just confusing for the umc devs to remember. If we intend to to something else with modifications in lua then we should also use a different name for that property in wfl.

This makes sense, but I don't think it has much if anything to do with this PR…?

[newfrenchy83]

I really think if a name of a property like modifications exists in formula and in both lua it should also do the same thing in both. Everything else it just confusing for the umc devs to remember. If we intend to to something else with modifications in lua then we should also use a different name for that property in wfl.

This makes sense, but I don't think it has much if anything to do with this PR…?

"traits" is already a id list in both formula and lua, and modifications is not used, if but it is also true what "advancements" is used in lua, i prefer what "advancements_taken" list if ID remain and what modifications.advancements used for number of advancements taken( and idem for other type)

[newfrenchy83]

I can't use a compound term like modifications.advancements in a formula because it doesn't work. The current terms must be retained and serve as the basis for any extension to lua for a future PR

[CelticMinstrel]

I can't use a compound term like modifications.advancements in a formula because it doesn't work. The current terms must be retained and serve as the basis for any extension to lua for a future PR

Ah, it's definitely possible for that to work… I just have my doubts about it being worthwhile in formulas.

"traits" is already a id list in both formula and lua

I was thinking about deprecating the Lua "list of trait IDs" property though, as the "modifications" API I proposed would render it completely redundant.

[newfrenchy83]

I can't use a compound term like modifications.advancements in a formula because it doesn't work. The current terms must be retained and serve as the basis for any extension to lua for a future PR

Ah, it's definitely possible for that to work… I just have my doubts about it being worthwhile in formulas.

me too, and i like what this PR was commited before what i can work on lua, but i doubt to could all use.

[CelticMinstrel]

The new _ids-suffixed names are not acceptable – if you're adding such a suffix, the base needs to become singular. There's also no point in doing that rename on the Lua side, because you'd be deprecating it only to deprecate it again later.

[newfrenchy83]

@Pentarctagon @stevecotton ,could you review this PR,please?

[CelticMinstrel]

I think this is ready for merge. I would merge it myself, but I'm not sure if it's suitable for 1.18.

[newfrenchy83]

I think this is ready for merge. I would merge it myself, but I'm not sure if it's suitable for 1.18.

i wait comment of @Pentarctagon for know if i must or not specify pospone 1.19

[Pentarctagon]

Given this is a new feature that isn't necessary to solve the mentioned issue, only to solve it better, I think it should go into 1.19.

[Pentarctagon]

@newfrenchy83 there's a conflict now, so can you rebase against master and resolve that?

@CelticMinstrel I assume this is still fine to merge as-is?

[newfrenchy83]

@newfrenchy83 there's a conflict now, so can you rebase against master and resolve that?

@CelticMinstrel I assume this is still fine to merge as-is?

yes

[CelticMinstrel]

Agreed, I don't see any issues.