Pyrel dev log, part 4

[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.

[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]

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.

[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!

[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.

[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.

[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]

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]

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.

[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.

[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]

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.

[Kinematics]

More subtype revision:

Since I'd decided that subtype can logically be considered the same as the category added at the object's level, I can actually set the value based on the category being there, rather than duplicating values in the object.txt file.

So, if the subtype is not specified in the object's record (not including ancestor templates), and a category is provided, the subtype is assigned the first category value.

Since the factory name already accounts for nameInfo['type'], I won't bother duplicating that.

This will generate subtypes on templates as well, but since templates are ID'd by the template name, it will have no effect.

So next level of revision is elimination of explicit subtypes, and only using categories.

[Kinematics]

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

The factory name needs to be easily identified, in order to provide a lookup mechanism that can be used elsewhere (eg: when generating loot drops).

item.categories (combined with parent template) is too burdensome for creating a unique name. Rather than (hard armor, chain mail) if using type/subtype, you'd have (hard armor + hard armor, body armor, armor, chain mail), plus any others that may get added.

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.

In what cases is this used?

1) Basic init/genTown for debugging makes items with a direct call that requires knowing the exact factory hash. Not a standard case.
2) Wizard Mode, same thing.
3) The only call that explicitly gets a specific factory is when creating an artifact. Artifact identification (and thus hashing) is trivial, depending only on the artifact's name.
4) getTypes, getSubTypes and getArtifacts are only accessed through Wizard Mode. (getArtifacts needs to be rewritten, as well)
5) itemAllocator does a loop through all the items in the ITEM_FACTORY_MAP, so knowing specific hashes doesn't matter.

It seems that having a simple, predictable hash is only a requirement for debugging and wizard mode. Otherwise a simple combination of categories and type is sufficient to make a unique key. Unfortunately that's not predictable across sessions, since the categories aren't provided in any particular order (though they could be forced alphabetical).


After some thought, we could leave the subtype field, and treat it as such: the subtype is the category added by the current object instance.

The hash for an object would thus be: templates, subtype, nameInfo['type']

This makes it reasonable to use in Wizard/Debug mode, but has minimal effect on the main play code.

[Kinematics]

Another major update to the pull request.

Cumulative changes:

Type/subtype have been completely subsumed in the naming system, but are still used in other systems. Transition in progress.

An item's naming details are specified in the nameInfo structure, including: modifier noun, primary noun, flavor types, flavor position, 'type' of item, item name (eg: artifact), and 'proc' -- the string that maps to the type of proc to use when trying to get the item's name.

The nameInfo struct (and the modifierNoun and baseNoun entries, in particular) is the -only- place where plural-decorated strings belong. Plural decorations should not be needed in any reference to an object. Ongoing cleanup.

A proc is created per item factory that gets attached to any items created. There are currently two procs: one for normal items, and one for unique items.

The proc code handles determining what bits belong where when describing the item, including dealing with 'type's, flavors, affixes, themes and names.

The grammar code handles assembling all the parts of the name together, with commas, 'of's, 'and's, articles, etc. All(?) such extraneous text has been removed from the object files.

The grammar code also has a subtantial unit test written for it.

object.txt, object_templates.txt, and artifact.txt have all been updated to match the new revisions, though they still retain type/subtype for now.

Templates can be based on other templates (to any depth). This allows a little extra flexibility in item construction, though I doubt it will be used much. Its primary value is neatly accumulating categories that an item belongs to.

Misc code tweaks throughout to make sure everything works nicely together.



Still to be done:

Clean up flavor-generating code, particularly for procedurally-created flavors.

Clean up affix/theme creation code to be sure no extra work is done trying to name items.

Find all uses of type/subtype and adjust as needed.

Remove obsolete class fields, primarily from item and itemFactory.

Verify and decide on effect of capitalization within object data files.

Outside review of the changes to see how well they fit in with expectations and intent.


Consider unit tests for other aspects of the code.

Major future work: handling of player knowledge, and how that affects what's displayed.