Common set of functions to manipulate unit inventories and advancements needed

[Dugy]

All modifications of units are currrently stored in units' modifications arrays. However, they are used quite rarely, while the whole array is copied often. It would be useful not to store them at all and store only the ids of advancements and numbers of items (in some cases, also sorts). All other information can be obtained elsewhere.

This transition would be quite wide and the code is not ready for it. To prepare for the transition and make it as fluid as possible, it will be necessary to isolate all code that accesses the items.

The functions should accept either a WML array or unit's id as their first arguments and use the variable type to decide what to do with the first argument. The other arguments would depend on the function. The plan is to make them all accessed by ids, but the WML tables might be needed with the current implementation.

Further thought will be needed regarding the specific functions.

[matsjoyce]

This sound like a good idea. It would reduce save size and make changes we make appear in the next scenario. Currently changing an item requires next scenario + unequip + equip, which makes savefile hacking a bit more difficult.

[edwardspec]

I will provide a sketch on the API and underlying architecture of this.

The migration idea is to implement this API twice: 1) one will work via the new efficient Lua array-based storage (with indexes and all), 2) one will work the old-fashioned way, via user.modifications[].

At first, the API would only use (2). Then all new code can use the API, even though a lot of WML still uses user.modifications[] directly. Later, after all the old WML is rewritten to use the API too, we can use implementation (1) as default, with (2) as fallback for old saves. Even later, (2) is removed.

[edwardspec]

Oversimplified view is that we have an array

A = { 'user_id1' => { data1, data2, data3, ... }, 'user_id2' => ... }

... where data is an arbitrary Lua array (not only the items can be stored there).

There will be search methods by certain fields in data[], and some of them won't just loop through the entire A array, they will use indexes that I will implement (caller of the API doesn't need to know about them).

[edwardspec]

If data is an item, then it probably just needs number, sort (because crafted items can be of different sort with the same number) and description keys (because description is slightly different due to locked/unlocked latent bonuses, which are coloured differently). The rest is the same as loti.item.type[number].something.

Other types of data (not items) can have other keys. Implementation of API (except the item-specific search indexes) won't be dependent on assumption that it's an item.

[matsjoyce]

The description is usually generated on the fly isn't it?

[edwardspec]

The description is usually generated on the fly isn't it?

Currently it's calculated in update_stats()/add_set_effects() in stats.lua and saved in user.modifications as object.description.

This is efficient (we don't need to recalculate it every time we view the Items menu). We won't store it in the save files though.

[Dugy]

At first, the API would only use (2)...

Yes. There is no need to implement the first version yet.

Oversimplified view is that we have an array..

I thought of something like that, but there is a problem - tag names cannot contain whitespaces, while unit ids contain them very often.

If data is an item, then it probably just needs...

I agree. For advancements, I suggest to store their id, quantity (if can be taken more than once) and order (if there are more with the same id).

The description is usually generated on the fly isn't it?

Item descriptions are set in items.cfg at the moment, which has to be rewritten before the rest.

What I find essential at the moment is not the way to store it outside the units, but the API. This is my suggestion:

loti.unit.items(unit) -- iterates over items
loti.unit.advancements(unit) -- iterates over advancements
loti.unit.effect(unit) -- iterates over effects
loti.unit.add_advancement(unit, advancement_id, advancement_order)
loti.unit.remove_advancement(unit, advancement_id, advancement_order)
loti.unit.remove_all_advancements(unit)
loti.unit.add_item(unit, item_number, item_sort)
loti.unit.remove_item(unit, item_number, item_sort)
loti.unit.remove_all_items(unit) -- returns a list of items that were removed
loti.unit.describe_equipped_item(unit, item_number, item_sort) -- describes an item, marking set effects

Does it include everything that can be needed without having to do things with unnecessary complexity (such as removing all advancements by linearly searching for each one)?

[edwardspec]

tag names cannot contain whitespaces, while unit ids contain them very often.

This array is internal to Lua. It doesn't need to be a valid WML table. We just need to make a WML table from it in wesnoth.persistent_tags.<something>.write function (only when the game is saved).

For example, the Lua array may look like this (assuming 'Prince Konrad' is the unit.id):

unitdata = { 'Prince Konrad' => { ... }, 'some_other_unit_id' => { ... } }

... however write() function will save it as the following WML:

{ { "unit", { id = "Prince Konrad", ... } }, ... }

[Dugy]

True. That should work.

Now, how about the API?

[edwardspec]

API is generally what we need. Some minor syntax notes:

iterates over effects

Returns iterator or (reference to) Lua table? for _, item in loti.unit.items(unit) or for _, item in ipairs(loti.unit.items(unit))?

loti.unit.add_advancement(unit, advancement_id, advancement_order) loti.unit.remove_advancement(unit, advancement_id, advancement_order)

Unclear name of advancement_order parameter. (the number of times this advancement was taken? Then advancement_level) Not intuitive if it's the new level (2 = "unit will have exactly 2 resist_fire advancements") or an increment (2 = "add 2 more resist_fire advancements").

If it's not an increment, then caller might sometimes need to count "how many resist_fire advancements do we already have".

effect(unit) -- iterates over effects

"effects", since other iterators (items, advancements) use plural.

loti.unit.describe_equipped_item(unit, item_number, item_sort)

Doesn't need item_sort.

[Dugy]

Returns iterator or (reference to) Lua table?

An iterator, the table would have to be created and there is no need for that.

Unclear name of advancement_order parameter.

There can be more advancements with the same id. It is useful to force the player to pick only one. I am not sure if there is a unit that uses it. So it is the number of the one with that it in the order as written in the file.

I don't think there is a need to check how many times an advancement is taken.

"effects", since other iterators (items, advancements) use plural Yeah.

Doesn't need item_sort.

Not at the moment, but it could be used to actually show reduced resistances on crafted helmets/gauntlets/boots.

[edwardspec]

So it is the number of the one with that it in the order as written in the file.

Feels undeterministic. Let's skip this parameter for now (may add it later if we find a real situation where it is needed).

It is useful to force the player to pick only one.

[advancement] now supports exclude_amla=id-of-another-advancement,id2,id3,... parameter, which allows to make several advancements mutually exclusive.

Not at the moment, but it could be used to actually show reduced resistances on crafted helmets/gauntlets/boots.

Ok.

[Dugy]

Feels undeterministic. Let's skip this parameter for now (may add it later if we find a real situation where it is needed).

[advancement] now supports exclude_amla=id-of-another-advancement,id2,id3,... parameter, which allows to make several advancements mutually exclusive.

Let us replace all usage of advancements with same ids replace with exclude_amla when the time comes, then.

[edwardspec]

I implemented readonly methods of (2): iterators and describe_equipped_item. How about you implement other methods of (2) and I implement (1)?

[Dugy]

I can, but at the moment, I am writing a function to generate item descriptions.

[Dugy]

I have completed those functions and rewritten yours so that they would return the data from unit types and item data rather than from the unit's modifications in commit ea56bf3

If you find the result satisfying, you can merge it, since it's not used in the code at the moment. Switching to using it will be another phase.

[edwardspec]

Will write an automated test for them and merge them into master.

[Dugy]

I am currently rewriting stats.cfg to use them. I had to add two additional functions while doing so. It will allow me to find the bugs. I might finish it as early as today.

Please don't merge it with master before that.

[Dugy]

Sorry, I am not done yet. The effects iterator turned out to be quite hard to implement without tons of copy/paste code and I kinda got lost in it. Now it almost works, but just set effects seem to fail to find its prerequisites.

[Dugy]

I am done. Should I merge it into master?

[edwardspec]

Not yet. I want to write an automated test for it first (a Lua function that would create a test unit, add/delete/list some items/effects/advancements to it via this API, and then compare results to expected), then I will merge it.

It will help me with developing non-WML-based implementation later. (I could just run the same test again...)

[Dugy]

Okay.

[edwardspec]

I apologize for the delay. The branch is not ready for merge yet (the testsuite is far from completion, but already caught some errors, I fixed what I noticed so far). Will know more as I continue with the tests.

[Dugy]

No problem. You could not foretell how much time was it going to take. Engineering problems take unpredictable amounts of time to solve.

Good to hear that you haven't abandoned it.

[edwardspec]

Question: why is add_advancement() applied to a specific unit? Isn't the list of advancements the same for all units of a certain type?

[Dugy]

It is the same for all units, but the advancement is added to a specific unit.

[edwardspec]

Testsuite now covers add_item(), add_advancement(), items() and advancements().

$ loti.testsuite()

Testsuite: running unitdata.test.lua
PASS    add/list advancements (on unit ID)
PASS    add/list advancements (on WML table)
PASS    list effects on empty unit (on unit ID)
PASS    add/list items (on unit ID)
PASS    add/list items (on WML table)
PASS    list effects on empty unit (on WML table)
Test results: passed: 6, failed: 0.

Next will add tests for remove_item(), remove_advancement() and effects().

[edwardspec]

Question 1: can we remove get_type_advancement() in loti.unit.advancements()? The unit itself has a copy of unlocked advancements (full WML tables of [advancement] tag), so isn't get_type_advancement(unit.type, elem.id) the same as elem?

(I removed a similar call of get_type_advancement() when rewriting effects(), seems to work correctly without it)

Question 2: I removed "multiply defence of crafted non-chest armours by 1/3" logic from effects() iterator (because defence is not an effect, it's property of the item - it doesn't affect anything returned by this iterator), but I see that this code was removed from update_stats(), was there an intention to move this logic somewhere else?

[edwardspec]

There are also leftovers of mods variable in update_stats(), but I'll handle those.

The merge (when ready) will be performed in two steps: 1) first unitdata.lua (+ testsuite) will be merged. It doesn't impact anything and doesn't cause merge conflicts.

2) changes to stats.lua, etc. will be merged. They were changed on master, but I'll merge it properly. They also need extensive manual testing: update_stats() is (so far) too complex to fully cover it with automated tests within reasonable time, this shouldn't delay the current issue at hand.

[Dugy]

Question 1: can we remove get_type_advancement() in loti.unit.advancements()?

No. The plan is to remove all copies of equippable objects and advancements from the unit to avoid duplicities in unit WML when the data is already available from elsewhere. This call is indeed superfluous at the moment.

Question 2: I removed "multiply defence of crafted non-chest armours by 1/3" logic from effects() iterator (because defence is not an effect, it's property of the item - it doesn't affect anything returned by this iterator), but I see that this code was removed from update_stats(), was there an intention to move this logic somewhere else?

The plan is to place that functionality outside of stats.cfg, because it would cause the items to be described automatically correctly when obtained through the function call and there would be no need to remember changing those values anywhere else. However, it was probably a mistake that it appeared in the effects iterator rather than in the item data provider.

The merge (when ready) will be performed in two steps:

All right. Regarding testing, I think that the best way would be not to publish to the server shortly after this part in some possibly impactful way. There's no way one human can manually test it all without forgetting to test a half of it.

[edwardspec]

Ok. Will return get_type_advancement() to effects() too.

there's no way one human can manually test it all

That's why I write automated tests.

In case of update_stats(), possible approach to manually testing them is to load a save like this: LotI-many-geared-units.gz - and trigger update_stats() on several units (e.g. by unequipping one random item). Because of how many items/effects/advancements these units have, if anything is broken big time, it has a high chance of showing immediately. It's also a real save (from an earlier version of addon), not a synthetic testcase.

[Dugy]

That's why I write automated tests.

You wrote you couldn't do it here.

In case of update_stats(), possible approach to manually testing them is to load a save like this: LotI-many-geared-units.gz - and trigger update_stats() on several units (e.g. by unequipping one random item). Because of how many items/effects/advancements these units have, if anything is broken big time, it has a high chance of showing immediately. It's also a real save (from an earlier version of addon), not a synthetic testcase.

If you have an idea how to get that working within a testing framework, then it's very good.

[edwardspec]

You wrote you couldn't do it here.

Not what I meant. There is no practical difficulty to test update_stats() automatically. Just (1) create a unit with certain state, (2) call update_stats(), 3) analyze the unit afterwards (comparing if its new properties are as expected).

I meant it would take too much time to write all the checks (because there are MANY things to check on step 3), and I don't want to keep this branch not merged for so long.

Probably the only thing we can't test automatically are GUI2 dialogs, because Wesnoth doesn't provide a way to simulate clicks.

[Dugy]

There is no practical difficulty to test update_stats() automatically. Just (1) create a unit with certain state, (2) call update_stats(), 3) analyze the unit afterwards (comparing if its new properties are as expected).

If you broke it into smaller blocks for testing, it would be necessary to test if those blocks work together. So I guess there's really no other way.

I meant it would take too much time to write all the checks (because there are MANY things to check on step 3), and I don't want to keep this branch not merged for so long.

I don't think writing a huge quantity of tests here is worth it. If something breaks, it doesn't break everything.

Probably the only thing we can't test automatically are GUI2 dialogs, because Wesnoth doesn't provide a way to simulate clicks.

Good to know.

[edwardspec]

Ok, I tested and merged unitdata.lua (and its testsuite). All these functions are working and can now be used in other Lua files.

A new branch unitdata-usage has been created to prepare changes in stats.lua for merging. (I cherry-picked them from the old branch)

Regarding the testsuite, there is an automated test for everything except item_with_set_effects() (this function is not an operation on the unit, its test doesn't belong in unitdata testsuite). The test of effects() includes some checks of the set bonuses (which are calculated by item_with_set_effects()).

[Dugy]

All right. Let's get ready for a flood of bug reports when it's merged.

[edwardspec]

Note: discussion of #250 contains additional information about this task.

[edwardspec]

I have implemented basic API (loti.unit.something) for Lua-based storage of unitdata. (see above). The commit message lists "not yet implemented" things. The new code has passed the automated testsuite. (with modification due to effects() being ordered in a different way, and testsuite being strict to order of effects)

Note that effects() of Lua-based storage still check WML for modifications, because some are not added by our code (e.g. unit traits like "strong"). However, it would be possible to move the most heavy stuff (like advancements or items) away from the unit's WML.

[Dugy]

Good job.

Traits cannot be really moved away, so let's keep only items and advancements away.

[edwardspec]

Items/effects can already operate on Lua implementation alone. (tested in Gladiators)

The only problem with migrating advancements is how "redeem" attacks are currently added (as effects appended to another advancement). It should be replaced with, for example, the unit having "redeem_level=7" variable, and redeem attack being added in update_stats().

[Dugy]

Great.

However, I somehow think it awkward to place the redeem information into variables. Maybe a dummy advancement could represent redeem and the properties would be added by lua?