Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New priceid-sell plugin #3

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

New priceid-sell plugin #3

wants to merge 6 commits into from

Conversation

Sec42
Copy link
Contributor

@Sec42 Sec42 commented Nov 15, 2012

also improved priceid-buy to know about helmets/gloves/boots.
Priceid-sell by default only displays stuff.

  • set $sell_id_tab=1 to enable auto-nameing.
    It requires you to stand on an empty square, and is a bit scary - no guarantees!
    If it bugs out on you, please tell me, so I can try to improve it.

@doy
Copy link
Member

doy commented Nov 15, 2012

Invisibility and magic resistance should be 60, not 50, in the first commit.

As for the sell price-id plugin, it uses data (%price_table and $shirt) from the buy price-id plugin (so it won't work if that isn't loaded), and also duplicates the calc_base function for no reason. A better option here would probably be to move calc_base, %price_table, %short_names, and $shirt (along with the extended_command for #shirt) to a separate plugin (priceid-utils.pl or something), and have both priceid-sell.pl and priceid-buy.pl include it (see botl.pl, for instance).

As for the actual functionality, I don't see any reason why this shouldn't just use the normal make_tab functionality if $sell_id_tab is true - it would make the information displayed more in line with how everything else works (knowing exactly what text will be sent, rather than just "press tab to name it", is important, especially when the plugin is potentially flaky).

Also, it doesn't use the %short_names functionality like priceid-buy.pl does.

Other than that, this does look like a good idea, and it's something that's been on the todo list for a while now, so thanks for working on it!

@Sec42
Copy link
Contributor Author

Sec42 commented Nov 16, 2012

Cloaks didn't work yet anyway. But yes, fixed that (was a copy&paste error).
Te include thing was already fixed (I snuck it in by amending my original commit) the first commit was a mistake of the unfinished version.
I totally forgot about the shortnames thing, added it already.
I understand why you want tab to show what it sends -- but I feel the real danger is more in that nethack is not completely behaving like I want it (e.g. the square you are standing on is not empty, or encumbrance state changes by pickup etc.) - I tried to tweak the key sequence so these two cases don't break it, but there might be other things like a monster attacking, or a scroll of scare monster desintegrating that I didn't test enough yet.
I did make it behave like the other tab things now unless you set $sell_id_tab_nice. The reason why I don't directly use make_tab is that if $sell_id_tab is unset, it just annotates and doesn't define tab at all.

Oh, and in the meantime another commit (Enhance $topline) made it in. I didn't originally intend it to be in this pull request, but it seems I can't exclude a single commit. -- It's also very useful if you are buying/selling long named scrolls, or if you are burdened while buying/selling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants