Legend_of_the_Invincibles
Legend_of_the_Invincibles copied to clipboard
Dugy
Common set of functions to manipulate unit inventories and advancements needed
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.
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.
I will provide a sketch on the API and underlying architecture of this.
The migration idea is to implement this API twice:
- one will work via the new efficient Lua array-based storage (with indexes and all),
- 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.
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).
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.
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.
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
datais 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)?
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", ... } }, ... }
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.
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.
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.
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.
I implemented readonly methods of (2): iterators and describe_equipped_item. How about you implement other methods of (2) and I implement (1)?
I can, but at the moment, I am writing a function to generate item descriptions.
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.
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.
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.
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...)
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.
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.
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?
It is the same for all units, but the advancement is added to a specific unit.
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().
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?
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:
-
first unitdata.lua (+ testsuite) will be merged. It doesn't impact anything and doesn't cause merge conflicts.
-
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.
Question 1: can we remove
get_type_advancement()inloti.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 fromupdate_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.