NPC Shop

[maxsond]

Pull request for issue #85 to implement an NPC shop system based on the tutorial with some tweaks:

Work with Ainneve economy
Autolink shop to builder's current location when built
Enable shop maintainer to set prices of wares
Wares for sale are colored yellow when LOOKing in the storeroom
Only descendants of the Item typeclass can be placed for sale
Handle timing issue when an item becomes unavailable for purchase after the menu is opened
Place key to storeroom directly in builder's inventory upon building the shop

[Griatch]

Just a note for future reference - evennia.utils.utils.inherits_from(obj_or_class, instance_class_or_path) will check inheritance at any distance while accepting a wider range of forms for the things to compare. I imagine inspect.getmro works just fine in this case though.

[feend78]

I would second Griatch's recommendation to implement this using the inherits_from function from the evennia.utils.utils module.

[Griatch]

Looks like debug statements left in.

[maxsond]

Whoops, yes.

[Griatch]

Note that _SaverDict is a private implementation detail (the initial _ generally denote privacy - well as private as things get in Python). While unlikely to change it's a bit of bad form to rely on it for checking (I know the check was not introduced in this contrib but it's just where I happened to notice it). More Pythonic would be to not do a typecheck here but assume it's a dict and catch and handle the traceback if it's not.

[feend78]

This looks pretty good in general. I haven't had a chance to fire it up and play with it locally, but I'll try to do so later on today.

[feend78]

I would second Griatch's recommendation to implement this using the inherits_from function from the evennia.utils.utils module.

[feend78]

With this change, you can probably remove the if statement in line 74 too.

[feend78]

@maxsond

I have cloned your branch and have fired it up in my local copy of Ainneve. Overall, it works very nicely. I only have a couple things I'd like to see addressed, and a couple that are suggestions.

First, I was wrong in suggesting the if dst is not None: could be removed in economy.py. It broke the part of chargen where you are "purchasing" gear from no-one, because it still referenced dst.db.wallet. I know github recently added an option to allow maintainers to push changes to PRs, so I decided to see if I could just correct it and commit, and it appears to have worked.

The only thing I want to see changed is the data handling in the menu. It works the way it is, but as @Griatch pointed out in a recent post on a mailing list, there are potentially problems related to caching if you don't use the ndb handler to store non-persistent data, or in the case of a menu, on the ndb._menutree object to have it automatically cleaned up when your menu exits.

This is more just a nit-picky editing thing, but the help for the @buildshop command has one line that sticks out farther than the cyan line of dashes. Perhaps adjust the text wrapping to be more even.

From a usability perspective, I think it's all functioning perfectly, but I do have a suggestion that you think about adding aliases to some or all of the exits. I created a shop named Danny's Dungeon-Diving Delights and then in order to enter it, had to type out that whole ridiculous name. A builder could handle this by using the @alias command to set an alias on the entrance after the fact, but it might be nice to steal some of the @create or @open commands' syntax and accept a list of aliases separated by semicolons in @buildshop. You could also perhaps split the shop name string on spaces and just create an alias for each word, though accepting them in the build command is probably more flexible.

For your automatically generated "front door" and "back door" exits, I would also suggest including the aliases "front" and "back" respectively, for less typing on the user's part, and perhaps also "out" on the front door as well.

The alias things are just suggestions, and I'll leave it to you to decide if you want to include them. At a minimum, though, I'd like to see the wares list temporary storage changed, then it would be ready.

I may hold off on merging it anyway, until I can address the unit test failures. I believe they were caused by recent changes in upstream evennia.

[maxsond]

Yeah, I hear you on the long exit names. I thought about it because it was annoying me as well. The reason I held off on friendlier aliases was that I figured every developer using this as a base is going to have slightly different ideas about what they want as default aliases so it seemed like a more implementation-specific detail. But having let it sit for a bit and with your feedback, I do think we could be a little friendlier with the out-of-the-box defaults so it isn't quite as mandatory for everyone to replace them if they don't actually care. I also like the "accept aliases" approach best. Do I understand correctly that I should be doing something like caller.ndb._menutree.npc_shop_wares = wares instead of caller.npc_shop_wares = wares to clean that up when the menu is closed? I don't think I saw that post about the caching issue, but I'll see if I can find it when I get home.

…

On Wed, Nov 30, 2016 at 4:17 PM, Dan Feeney ***@***.***> wrote: @maxsond <https://github.com/maxsond> I have cloned your branch and have fired it up in my local copy of Ainneve. Overall, it works very nicely. I only have a couple things I'd like to see addressed, and a couple that are suggestions. First, I was wrong in suggesting the if dst is not None: could be removed in economy.py. It broke the part of chargen where you are "purchasing" gear from no-one, because it still referenced dst.db.wallet. I know github recently added an option to allow maintainers to push changes to PRs, so I decided to see if I could just correct it and commit, and it appears to have worked. The only thing I want to see changed is the data handling in the menu. It works the way it is, but as @Griatch <https://github.com/Griatch> pointed out in a recent post on a mailing list, there are potentially problems related to caching if you don't use the ndb handler to store non-persistent data, or in the case of a menu, on the ndb._menutree object to have it automatically cleaned up when your menu exits. This is more just a nit-picky editing thing, but the help for the @buildshop command has one line that sticks out farther than the cyan line of dashes. Perhaps adjust the text wrapping to be more even. From a usability perspective, I think it's all functioning perfectly, but I do have a suggestion that you think about adding aliases to some or all of the exits. I created a shop named Danny's Dungeon-Diving Delights and then in order to enter it, had to type out that whole ridiculous name. A builder could handle this by using the @alias command to set an alias on the entrance after the fact, but it might be nice to steal some of the @create or @OPEN commands' syntax and accept a list of aliases separated by semicolons in @buildshop. You could also perhaps split the shop name string on spaces and just create an alias for each word, though accepting them in the build command is probably more flexible. For your automatically generated "front door" and "back door" exits, I would also suggest including the aliases "front" and "back" respectively, for less typing on the user's part, and perhaps also "out" on the front door as well. The alias things are just suggestions, and I'll leave it to you to decide if you want to include them. At a minimum, though, I'd like to see the wares list temporary storage changed, then it would be ready. I may hold off on merging it anyway, until I can address the unit test failures. I believe they were caused by recent changes in upstream evennia. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#88 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB2zWSxc6EkFwuUWc_gEV4cQfAt2HekUks5rDaGUgaJpZM4K-EP6> .

[feend78]

You are correct in your description of the change. Here's a link to the post I was referring to:

https://groups.google.com/forum/#!category-topic/evennia/NY8eHrC15ZU

In Griatch's first reply to the original poster.

[maxsond]

Oh yeah, and I forgot to mention in that description that I also added some aliases to the intra-shop exits. I leave it to the builder to explicitly assign any aliases to the shop entrance using the semicolon syntax.

[feend78]

I see a slight problem with this approach to highlighting the StoreRoom's stock, which is that the "(" that you are looking for in the description only appears for the superuser or the objects owner (creator). It may be better to do something like

    if any(line.startswith(w) for w in wares):
        ...

Also, at the top of this module, you have defined a get_wares() function that could probably be used in place of lines 260-263.

One last suggestion: it may be nice to append the cost to the item's line in the description in addition to the yellow highlight, since a non superuser/owner shopkeep has the price command, but won't have examine to be able to check the current price.

Overall, this looks really good. Thanks for the contribution!

[feend78]

In ongoing testing, I think I found a problem with the price command. I spawned a whip in the storeroom and tried to change its price, but I just get the message

I can't find a sellable Item called 'whip' here.

I think it's because the typeclass argument is narrowing the results too much; returning only Items and not its subclasses.

[maxsond]

Great catches. With this new commit, I take advantage of the fact that the room contents in the room's return_appearance are in the same order as they're listed in the room's "self.contents" property. This lets me add wares to my own list in the same order, then pop the value at the zeroth element as we go down the return_appearance list. Could also reverse my list and pop off the tail of the list, but that might be more confusing.

I'm not entirely sure how great a solution that is, but it seems to correctly handle:
Multiple items with the same key but different prices
Ineligible items (like the storeroom key) in the middle of the contents list

...Just as long as nobody goes in and decides to reorder how a room's contents are displayed.

[feend78]

I left a few thoughts about return_appearance inline again. Not totally sure the best way forward. Your thoughts?

[feend78]

I think popping the items off the wares list might be a little risky. How about something like

for line in desc.split("\n"):
    for ware in wares:
        if line.find(ware.get_display_name(caller)) > -1:
            line = "|y{item} |w{price}".format(item=ware.get_display_name(caller), 
                                               price=format_coin(ware.db.value))

Then again, it seems like there's always going to be the chance of getting mismatches, just from the nature of searching through the parent return_appearance output for matches. Non-builder characters can recog anything as anything else, and giving multiple items the same recog names would confuse my sample code and possibly just not be highlighted in your current branch code.

I wonder if it might just be easier to re-implement return_appearance by grabbing the parent implementation, and doing your own sorting of exits, wares, characters, and other items, and constructing the appearance string, rather than modifying what comes out of the parent implementation.

[maxsond]

Yeah, I was bending over backwards trying not to re-implement Room's return_appearance method because I really would've liked to keep StoreRoom tracking with any changes later developers make to Room. But I suppose this approach is just as vulnerable to changes if someone decides they want to represent room contents differently from how they're currently represented.

What about calling three component methods from Room's return_appearance:

1, Room description (can handle room properties like weather, season, darkness, etc. and can be shared with StoreRoom)
2. Exits (Can also be safely shared between Room and StoreRoom)
3. Contents (with StoreRoom overriding this method with its own custom implementation)

My thinking is the only thing that's unique about StoreRoom's return_appearance is what it's doing with its contents display. So if we can inherit everything else from Room, that will keep it tracking with changes to Room (even new room appearance components) while providing the custom contents implementation?

[feend78]

I think this is a worthy intention, but the problem is that the parent's return_appearance is monolithic. There's no way to 'call' the components you list outside of just calling output = super(...).return_appearance() and parsing output, which obviously has its pitfalls.

One question about this: how important is it really for StoreRooms to track with other changes to Room? It is, in a sense, a highly-specific utility room, rather than a backdrop for game action. It may not be a problem if the output eventually looks a little "legacy" and/or needs a manual update to stay in sync with the parent.

Another consideration is the source of these potential upstream changes. I would guess that most changes to return_appearance in Ainneve will be initiated in this code base, rather than because upstream Evennia has changed how the vanilla DefaultRoom class does things. This means we'll have the opportunity to evaluate and port changes if they are important.

The more I think about it, the more I think we just bite the bullet and copy/modify the base return_appearance. However, if you can come up with something more elegant, I'm still totally wiling to entertain the idea.

[maxsond]

Here's a quick sketch of what I'm talking about. I don't think you need to use super if you use this pattern: https://gist.github.com/maxsond/bee981a3e0d80a60e591b6faf0c5c3ba

[maxsond]

As for how important it is, I don't know. Right now I don't think it matters at all, but for future developers using this as a base to build from I think it varies by implementation.

The reason I'd like to make it track with other changes to Room is that I think that is the natural assumption one would make who was coming to this code with no prior knowledge. I'd like to support that assumption insofar as I can, because I think it makes it more future-friendly. That's not necessarily a hard requirement, but just a value I'm factoring in when considering how to solve this problem. If it has to be sacrificed then that's a valid trade-off, but I think the gist above shows a way we can support it with relative ease.

[maxsond]

Ah, never mind this after all. For some reason I though return_appearance was defined in Ainneve's Room class but now I see that's not the case and this would actually require rejiggering how Evennia itself does room appearances. So, yeah. Custom return_appearance method it is.

[feend78]

So I wrote the following last night but forgot to submit it before your comment this morning:
-=-=-

Given the example in your gist, then we really are talking about fully re-implementing return_appearance as far as Ainneve rooms are concerned, but with more extensible ways of organizing the data. I can get behind that idea, but it's starting to range beyond the scope of creating a usable NPC shop.

Since Ainneve is using contrib.rpsystem components, maybe the right way to move forward for now, is to grab the guts of ContribRPObject.return_appearance and modify it within the StoreRoom class to get it working and finish off this PR.

Then, create a new issue/branch where you can address re-organizing how the base Room typeclass marshals its data and make it easier for subclasses to reformat their own return_appearance output.

[feend78]

This looks good. For consistency, please change the old {-style color codes to |-style.

Also, perhaps consider adding a "Wares" heading, moving users to below wares, and removing pose=True from the get_display_name call for wares only. It's up to you, though. Aside from the color codes, these other things are just aesthetic suggestions.