Pyrel dev log, part 4

[Derakon]

Type/Subtype are still used in the creature code. Is its purpose there sufficient to leave it as is?

Here they serve two important purposes -- subtype for naming, and type for Banishment. That doesn't mean that they can't be handled in a different manner, but in the interests of avoiding scope creep it seems preferable to leave them as-is for now unless you have a pressing reason to change them.

Can it be done using sets instead? Except you can't keep a mutable object in a set, so working from that direction doesn't work either.

You can put mutable objects into sets so long as you override their __hash__ function (and possibly also __richcmp__), but I suspect this is considered bad style.

[AnonymousHero]

You can put mutable objects into sets so long as you override their __hash__ function (and possibly also __richcmp__), but I suspect this is considered bad style.

It is insanely easy to get hard-to-reproduce bugs by doing this, so I'd say it's a definite no-no, no matter how tempting it might be .

[Derakon]

It is insanely easy to get hard-to-reproduce bugs by doing this, so I'd say it's a definite no-no, no matter how tempting it might be .

I had a project where I did this because my game map grid was a dictionary that mapped positional vectors to the contents at that location, and the vectors were a custom class. Knowing how to make something eligible for hashing is useful information. But yeah, if you aren't careful it's very easy to "lose" something in your set/dict and be unable to get it back out, so generally you should only do this with nonmutable objects.

[Kinematics]

Here they serve two important purposes -- subtype for naming, and type for Banishment. That doesn't mean that they can't be handled in a different manner, but in the interests of avoiding scope creep it seems preferable to leave them as-is for now unless you have a pressing reason to change them.

Yeah, I was thinking similarly. I don't want to get into the creature code right now, so the fields themselves will remain.

[Kinematics]

Now that the factories are hashably named, and we'll never actually be using that name anymore (aside from debugging), need to adjust references of type/subtype, and change where appropriate.

Item name now passed completely over to the proc. Old code removed.

~Aside: Speaking of item, why is there onPickup and onDrop as proc names, but then "item use", "item wield" and "item removal"? Why not onUse and onWield and onRemove?

Theme/affix data files need to be changed to the new format. -- completed

Theme/affix allocators can be shifted from item type/subtype to item categories. -- completed

Item filter can be adjusted to use categories instead of type/subtype. -- completed

carrier.py not fully converted, while I work out the meaning of a couple variables, and if there are potential side effects. Seems overly complicated for what it does.

Item uses the Carrier mixin, and uses the default initialization. The Carrier inits itself to being able to carry 23 items of any type. Does this mean that every single item has the capability of carrying 23 items of every type?

Ah, but the factory then uses non-dat-file fields to store carrying capacity and the like. That means most items will get set to a capacity of 0 after the initial initialization to 23.

carriedTypes in the object dat files apparently were designed to use an arcane mix of type/subtype values that the factory could interpret and then convert to a form that was used by the Carrier. Since carriedTypes is now a simple category listing, I'm removing this intermediate step, and instead doing a direct carry-over of a canCarry category list.

I'm not making changes to the handling of the maxCapacity field at this time.

Found item duplication bug while testing containers. Reverting the changes didn't fix it, so it's not something I broke.

Bug: pick up a quiver of arrows. Inventory is a) quiver, b) arrows. Drop arrows. Arrows are now on the ground, as well as still in the quiver. Repeat ad infinitum.

However, I also found that picking up two stacks of 25 arrows didn't update the quantity I had in inventory. It was still 25.

In any case, the canCarry portion of the code change is done.

Still need to handle procedural flavors. I think that's the very last item on the list.

[Kinematics]

Ok, procedural flavors in place as well. Had to add a special flag that the grammar function could recognize to prevent it from prepending it with "and" or "of" during full name construction.

Added additional unicode checks in places, so that we can get proper text output with unicode characters (including prompt.py). The scroll title generation code includes a large number of unicode-form vowels, and it's also needed for various artifact names.

Added a couple more test items to the genTown to see that things worked.


At this point, all the work that I know of for handling naming and display of item names is done, along with cleanup of item structure so that carryovers from original V code could be removed and streamlined. Further refinements are possible, but as a patch, it's 'complete' (unless review brings up some bugs).

I didn't dig into the issue of duplicating items; that can be dealt with elsewhen.


125 commits affecting 26 files. Sorry for the review headache it'll be.

[Kinematics]

Hmm. Might have missed a bit in the loot files. Have to dig into that.

Edit: Ok, loot patched up for item categories.

Edit2: And one more small bug fix for some calls that didn't get fixed to match the changed functions.

[Magnate]

125 commits affecting 26 files. Sorry for the review headache it'll be.

You're not kidding!!! I'll try to set aside some time over the weekend to get my head around it. Do you think you could write a little guide for it, along the lines of http://bitbucket.org/derakon/pyrel/w...m%20generation ... I would find such a thing very helpful when looking at the new code.

Many thanks for all your hard work.

[Derakon]

I'll be taking a look as well when I have the time. As Magnate says, some high-level overview would be very helpful so I don't have to just dive into the diff and intuit how different changes relate to each other.

Thanks for all your hard work!

[Kinematics]

Overview of the changes made:

Updated the data files to match new formatting requirements and usages:

• Text decoration only occurs in fields under nameInfo. It's been removed from anything that may be considered a 'data' field.
• Types still exist (only included on templates, not items), but aren't actually used. These can probably be removed entirely at a later date.
• Subtypes were removed from all but a few items (left on for money and spellbooks, for now; can be removed from those once sure they're not of any use).
• Categories added to all items and their base templates. Each item and template inherits all the categories its parent templates used.
• Templates can inherit from other templates now. This allows for a chain of inherited categories. EG: item > armor > body armor > hard armor > chain mail > augmented chain mail.
  • Note that any given template is only applied once. An item marked as inheriting the [artifact] and [helm] templates would have individual template sequences of [artifact < unique < item] and [helm < head armor < armor < item], but [item] is only factored in once. This is important so that you don't overwrite fields that have already been overwritten. [item > unique > item] would set the name proc of "default name" > "unique name" > "default name", which we don't want to happen.
  • I added a few extra intermediate templates for useful additional categorization: all magic items (wands/rods/staves/potions/scrolls) inherit from [magic item]. The three body armor types now inherit from [body armor]. I added a [head armor] as a parent to helms and crowns (and may potentially add a 'caps' group), and a [foot armor] as a parent to boots (hard boots and greaves) and shoes (sandals, slippers, etc), though I'm not sure which one to put soft boots in (it's in boots for now). Also: [weapons] as parent for sword/polearm/hafted/diggers (may be better to leave diggers out of that grouping? would simplify a lot of filters used), [ammo] as parent for arrow/bolt/shot, and added bow/crossbow/sling as children to [launchers].
• itemType (field that was used to determine filtering, such as allowing an affix to apply to all hard armor except plate mail) has been removed, since it depended on the type/subtype system. It's been replaced by two fields: allowCategories and denyCategories. These allow filtering in the same way as http and smtp permission filters: reject anything under deny, allow anything under allow, reject anything else (though an empty allowCategories field is treated as "all"). Since each item inherits all of its base template categories, this allows easy general delineation and fine-grained control.
• nameInfo allows specification of:
  • baseNoun - eg: "gauntlets", "wand~"
  • modifierNoun - eg: "Set~ of"
  • flavorTypes - eg: "Woods" [this means the flavor_types.txt isn't needed, since the flavor type is now directly accessible on the template used; haven't removed it yet, though]
  • flavorPosition - prefix/suffix position, or whether a flavor is a 'material' adjective (a Brown Potion) or a naming suffix (a Scroll titled "xxx yyy"). It may be possible to obsolete this and move the logic of determining what position it holds into the flavor generation code, since it's more directly obvious there which is which.
  • type - the 'type' of item (eg: Amulet of Charisma), used as part of the item's name
  • proc - the name of the proc function to use to generate an item's name. Currently only has two values: "default name" and "unique name", for determining whether to use a definite article or not.
• Note that since all of those are inherited from template to template, many don't need to be explicitly specified. For example, the only templates that need to specify proc are [item] and [unique] (which inherits from item and just changes the proc and adds the 'unique' category).
• flavors.txt added a Titles group.

Then the changes to code:

• record - Created generalized functions for constructing and applying record data to an object, rather than duplicating code across other files. Also allows adding JSON list data to sets, rather than clobbering the base field. (JSON doesn't have any data type for sets) Made the loader into a yield-type enumerator instead of loading all the values into a single array to return.
• extend - Added a class for a couple utility subclasses, one for dict (for a well-designed groupby) and one for set (adds a membership test function that can handle multiple types of data without throwing errors).
• filter, itemAllocator - Changed to depend on categories rather than type/subtype values.
• theme, loot - Updated to match data file structure changes
• affix_meta, itemLoader - Added fields for sorting groups of affix types: those that go before the modifier noun, those that go between the modifier and base noun, and those that go after the base noun.
• affix, theme - Changed so that they don't try to set the name of the object. Expanded the info they store on the item about their respective values, though.
• item - During early development I added some stuff such as decoratedType/Subtype, but eventually that was all culled as things moved to categories and nameInfo structs. During middle development, added functions for category membership tests, but removed after creating the categories subclass in extend.
• itemFactory - Updated to support the new data file structure, and pass appropriate values to created items. Automatically adds a subtype to any object that doesn't have a subtype defined (uses the category of the current, not parent, record). This is only relevant for the factory name. Adds a function to generate its own name based on: templateName (if any), properName (if any), or its template and subtype and name 'type'. Done this way for a 'friendly' value that's relatively easy to reference in debug construction and Wizard mode.
• proc - Add the new name procs to the available lists.
• nameProc - Major new function to construct an item's displayed name based on nameInfo, flavor, name 'type', affixes and themes.
• grammar - Holds functions for assembling a grammatical version of an item's name based on the pieces of the name provided: quantity, modifier adjectives, modifier noun, base adjectives, base noun, suffixes, and uniqueness.
• grammar_test - A unit test for the functions in grammar

Since this patch includes lots of intermediate progress changes, you may want to consider getting a clone and looking at the diff between the current and the original for each file for a real overview. There are a lot of individual commits, but I tried to make each one very specific; it's just that some changes were superceded by other changes later on.

[Magnate]

Many thanks for this - totally essential reading.

I added a few extra intermediate templates for useful additional categorization: all magic items (wands/rods/staves/potions/scrolls) inherit from [magic item]. The three body armor types now inherit from [body armor]. I added a [head armor] as a parent to helms and crowns (and may potentially add a 'caps' group), and a [foot armor] as a parent to boots (hard boots and greaves) and shoes (sandals, slippers, etc), though I'm not sure which one to put soft boots in (it's in boots for now). Also: [weapons] as parent for sword/polearm/hafted/diggers (may be better to leave diggers out of that grouping? would simplify a lot of filters used), [ammo] as parent for arrow/bolt/shot, and added bow/crossbow/sling as children to [launchers].

Good - I think that's where I ultimately wanted us to end up. Stuff like V's distinction between helms and crowns always bugged me, because there really ought to be a simple way to refer to the combined set of both - now there is. This category system gives us the best of both worlds - easy high-level sets and easy fine-grained control. Thank you.

[*]itemType (field that was used to determine filtering, such as allowing an affix to apply to all hard armor except plate mail) has been removed, since it depended on the type/subtype system. It's been replaced by two fields: allowCategories and denyCategories. These allow filtering in the same way as http and smtp permission filters: reject anything under deny, allow anything under allow, reject anything else (though an empty allowCategories field is treated as "all").

This is excellent too, because it makes ONLY_ITEM simpler: reject money, allow all.

nameInfo allows specification of:

• baseNoun - eg: "gauntlets", "wand~"
• modifierNoun - eg: "Set~ of"
• flavorTypes - eg: "Woods" [this means the flavor_types.txt isn't needed, since the flavor type is now directly accessible on the template used; haven't removed it yet, though]
• flavorPosition - prefix/suffix position, or whether a flavor is a 'material' adjective (a Brown Potion) or a naming suffix (a Scroll titled "xxx yyy"). It may be possible to obsolete this and move the logic of determining what position it holds into the flavor generation code, since it's more directly obvious there which is which.
• type - the 'type' of item (eg: Amulet of Charisma), used as part of the item's name
• proc - the name of the proc function to use to generate an item's name. Currently only has two values: "default name" and "unique name", for determining whether to use a definite article or not.

Wait a minute - why do we have "type" here? I thought the whole point of categories was to remove type and subtype? Surely "of Charisma" is simply another category? So if we want a way to refer to potions of Charisma and amulets of Charisma together, we can.

affix_meta, itemLoader - Added fields for sorting groups of affix types: those that go before the modifier noun, those that go between the modifier and base noun, and those that go after the base noun.

? I don't think this works. The type of an affix should not determine its position. For example, 'make' affixes will normally be prefixes (Dwarven, Elven etc.) but could also be suffices (of Elvenkind, of The Drow, whatever).

Each affix defines its position explicitly - can we not change that affix position field to accept values of "before modifier", "after modifier" and "after basenoun"?

itemFactory - Updated to support the new data file structure, and pass appropriate values to created items. Automatically adds a subtype to any object that doesn't have a subtype defined (uses the category of the current, not parent, record). This is only relevant for the factory name. Adds a function to generate its own named based on: templateName (if any), properName (if any), or its template and subtype and name 'type'. Done this way for a 'friendly' value that's relatively easy to reference in debug construction and Wizard mode.

In addition to my misgivings above about "type", this strikes me as a bit unwieldy. I thought the idea was to get rid of subtypes, so why this workaround with creating them when they don't exist? Do we just need to think of a better system for naming factories?

[Kinematics]

Wait a minute - why do we have "type" here? I thought the whole point of categories was to remove type and subtype? Surely "of Charisma" is simply another category? So if we want a way to refer to potions of Charisma and amulets of Charisma together, we can.

It's called 'type' because I couldn't think of a better name for it. (note: see bottom) It's not a type in the same way as the previous type/subtype was. From some other notes I was writing up:

Obviously the name is expected to reflect what the item does, but it's not necessarily intrinsic. The mods and other data are specified in separate fields; I could change the 'type' to "Beauty", or "the Movie Star" or whatever, and it would have no effect on the stats of the item itself. That's why it's part of the nameInfo struct, and not put in as a data value.

Essentially, 'Charisma' is not a categorical type, it's a naming artifact. You could add it to the [charisma] category if you wanted to be able to group it with things like potions of charisma, for all items that affect charisma, while at the same time changing the name 'type' to "Paris Hilton". In other words, the name is not the same thing as the effect.

EG:
"Amulet of Charisma", +2 to CHR
"Amulet of Paris Hilton", +2 to CHR

Both could be in the [charisma] category, but that's a separate aspect.


I'd welcome a different name for that field, but there were no comments when I was musing about it earlier.

? I don't think this works. The type of an affix should not determine its position. For example, 'make' affixes will normally be prefixes (Dwarven, Elven etc.) but could also be suffices (of Elvenkind, of The Drow, whatever).

Each affix defines its position explicitly - can we not change that affix position field to accept values of "before modifier", "after modifier" and "after basenoun"?

That would certainly be more flexible, though there's still some caveats. You could specify the general position in the affix itself (eg: "Elven" as baseAdjective, "of Elvenkind" as suffix) and that would take care of part of the issue.

I moved that into the affix_meta, though, because there still needs to be an ordering within each subsection (eg: material before make when in the baseAdjective position). I can see that that would break naming with the above example, though.

Yeah, that needs to be rethought.

Ok, we can have each affix specify its affixType and its position (modAdj, baseAdj, suffix). However we still need some means of ordering within that position. Perhaps a positionWeight value? The lower the weight value, the closer it is to the item it's modifying?

Affix          Position    Weight
Elven           baseAdj         1
Leather         baseAdj         2
of Elvenkind     suffix         1
Resist Cold      suffix         5
Strength         suffix         3
Blessed          modAdj         1

Would be laid out as:

1       mod      2     1  base            1         3               5  
Blessed [] Leather Elven Boots of Elvenkind, Strength and Resist Cold

Can provide default values in affix_meta, perhaps, and then allow override per suffix, if relevant. In this case, affixType 'make' would be weight 1 for either of its positions, so that's convenient.

Potentially confusing. Have to play with it a bit. Maybe name it modifierDistance or something, instead of weight.

In addition to my misgivings above about "type", this strikes me as a bit unwieldy. I thought the idea was to get rid of subtypes, so why this workaround with creating them when they don't exist? Do we just need to think of a better system for naming factories?

The point of this was to make it easy for a user to specify a factory name. It's completely unnecessary if its only ever used by code, but when making items explicitly for debugging, or for Wizard mode, it needs a certain amount of 'friendliness'.

In this case, subtype refers to the primary category that was added for the item in question. Since categories are sets, they have no intrinsic order, so I needed some way to pull that out of the original record (which is a simple list) and hold onto it. I repurposed the subtype field for that.

The nameInfo 'type' is a little confusing due to the name. It's used for identifying specifically named variants.. actually, that might be a nice way to name it: 'variantName', instead of 'type'. I like it.

[Derakon]

Wait a minute - why do we have "type" here? I thought the whole point of categories was to remove type and subtype? Surely "of Charisma" is simply another category? So if we want a way to refer to potions of Charisma and amulets of Charisma together, we can.

The issue is in how to decide the name of an Amulet of Charisma -- which category do you pick out as the one worthy of deciding the name? "item"? "amulet"? How do you tell that any given category is the "maximally-specific" one that should be used for naming?

In addition to my misgivings above about "type", this strikes me as a bit unwieldy. I thought the idea was to get rid of subtypes, so why this workaround with creating them when they don't exist? Do we just need to think of a better system for naming factories?

The names of factories are only ever explicitly referred to in debugging code. Everyone else doesn't really care -- they select factories based on item allocation rules.

[Kinematics]

To change:
'type' nameInfo value to 'variant' or 'variantName'

'prefix' positions separated into 'modPrefix' (for adjectives before the modifier noun) and 'basePrefix' (for adjectives before the base noun).

Additional field: positionDistance. Values of:
0 - invalid; equivalent to the base noun itself
1 - immediately adjacent
2 - next most adjacent
etc

I don't see any particular need for them to be only integers, should there be any specific need for a special insertion distance, but the default setup should just be integer values.

Default values:
quality: 1
material: 2
make: 1
combat: 2
physical: 3
mental: 4
elemental: 5
arcane: 6
holy: 7

variantName would get inserted at suffix distance 1. Flavors would be basePrefix 2 or suffix 10 (ie: some high value after everything else). Any specific affix can change its relative position or distance, if needed. In the case of multiple adjectives at the same distance, go by alphabetical order.

[Magnate]

The issue is in how to decide the name of an Amulet of Charisma -- which category do you pick out as the one worthy of deciding the name? "item"? "amulet"? How do you tell that any given category is the "maximally-specific" one that should be used for naming?

Ah well this I have had the answer to for a couple of years now - affixes. The item is an amulet, it has one affix: of Charisma. Problem solved.

If you are content, I'm happy to implement this. Items will still look like V's, but it will be quite different under the hood.