Bug - Fixed Ability to not display some consumables (specifically Bounty foods) in Item Manager

roippi

roippi

Developer
No hard feelings, I had my grumpy pants on earlier when I made that post.

Greenen72 said:
but assume is trivial to fix, considering it was working as intended in the previous release.
Click to expand...

Was it? If so, zeroing in on when this broke would indeed trivialize the fixing. If someone wants to do that legwork that'd be swell.
 
H

heeheehee

Developer
Staff member
Bale said:
(Though I do it more thread manipulation than fronobo and with less fanfare, such a when I split the FotM thread from the Snow Suit thread. Ugh, what an awful combination of New Content. I could have split the two FotM apart also, but it would have been more work and the Unconscious Collective was already implemented by then so it didn't matter.)
Click to expand...

FotYs, you mean?

roippi said:
Was it? If so, zeroing in on when this broke would indeed trivialize the fixing. If someone wants to do that legwork that'd be swell.
Click to expand...

Oh, how I wish svn had an equivalent to git-bisect. Oh wait. I'll try checking it out; I've always needed an excuse to play around with git-svn.

edit: Interesting. With or without the coinmasters box checked, it only shows me the booze, not the food. Yes, I have lucres in inventory, no, I do not have either item in inventory.
double edit: ignore me, I just wasn't seeing the bowl of bounty-Os. Too many other foods of same quality.
 
Last edited:
H

heeheehee

Developer
Staff member
Intriguing.

Code: 
> ash available_amount($item[bounty-o])

Returned: 0

So that works just fine.

Patch attached; this brings up an interesting point. Namely: autoSatisfyWithCoinmasters (and related features) is only intended for use with retrieve_item and the internal equivalent; the GUI is a perfectly acceptable way to use Mafia as a user (hence the "U" in "GUI"). I'm not sure that it makes sense to accept this feature, in this case. Then again, it doesn't list all items that can be bought in the mall, while it does list items that can be bought from NPC stores. Not entirely sure which way to go with this, then. Might amend patch to also affect NPC stores.

Code: 
Index: src/net/sourceforge/kolmafia/request/CoinMasterPurchaseRequest.java
===================================================================
--- src/net/sourceforge/kolmafia/request/CoinMasterPurchaseRequest.java (revision 11817)
+++ src/net/sourceforge/kolmafia/request/CoinMasterPurchaseRequest.java (working copy)
@@ -39,6 +39,7 @@
 import net.sourceforge.kolmafia.KoLConstants.MafiaState;
 import net.sourceforge.kolmafia.KoLmafia;
 import net.sourceforge.kolmafia.RequestThread;
+import net.sourceforge.kolmafia.preferences.Preferences;
 
 public class CoinMasterPurchaseRequest
        extends PurchaseRequest
@@ -73,7 +74,6 @@
                this.cost = AdventureResult.tallyItem( token, price, true );
 
                this.limit = this.quantity;
-               this.canPurchase = true;
 
                this.timestamp = 0L;
 
@@ -121,7 +121,7 @@
        @Override
        public boolean canPurchase()
        {
-               return this.canPurchase && this.affordableCount() > 0;
+               return Preferences.getBoolean( "autoSatisfyWithCoinmasters" ) && this.affordableCount() > 0;
        }
 
        @Override

edit: ack, I have no idea what I was doing with this patch. Presumably the point of this.canPurchase was so that it could be overridden in the case of coinmasters that are no longer available.
double edit: I should probably stop commenting on the code before actually reading all of it.
 

Attachments

  • coinmaster.patch
    1,022 bytes · Views: 25
Last edited:
C

Catch-22

Active member
heeheehee said:
this brings up an interesting point. Namely: autoSatisfyWithCoinmasters (and related features)
Click to expand...

If you debugcreate an item that requires items in your closet but you haven't allowed KoLmafia to "autoSatisfyWithCloset", debucreate will say it can't make any. The same should apply to "autoSatisfyWithCoinmasters" but it doesn't. I don't think you're patching the problem at the correct place, but you're on the right track.
 
H

heeheehee

Developer
Staff member
I mean, the above patch works both in ItemManager as well as with debugcreate. *shrug*

although... looking at the code for ConcoctionDatabase.refreshConcoctions(bool), it apparently still treats the Hermit as an NPC store, as opposed to a coinmaster. This is completely unrelated and I should probably make a FReq or something. Maybe in the morning if I still remember.
 
Last edited:
C

Catch-22

Active member
I wasn't saying your patch doesn't work. I just don't feel as though the problem is solved within the correct scope, but a dev might disagree with me.
 
H

heeheehee

Developer
Staff member
Well... the other option looks like attacking ConcoctionDatabase#refreshConcoctions. While that's what's done (in addition to some code in GenericRequest, UneffectRequest, and HermitRequest) to enforce autoSatisfyWithNPCs, it seems like it'd be harder to find, and there are more places to put it.
 
Greenen72

Greenen72

Member
Sorry for the bump, but this still seems to be an issue. Coinmaster acquisition is still off, but now it's showing items from the Terrified Eagle Inn as well as the Bounty Hunter Hunter

(Hopefully bumping was the right thing)
 
Theraze

Theraze

Active member
The last point this got to was discussing whether or not to consider this actually as a bug or not. Since there was a patch submitted which was not included in main, that suggests the end response was that displaying the coinmasters was useful and consistent with NPC store behaviour.
 
Greenen72

Greenen72

Member
I thought the toggle for "Buy items using X when needed" was supposed to be the deciding factor in whether or not they showed up?

If this is not the case, then it'd be really nice to have something like that. I really hope I'm not alone in this, because closeting all lucre/krugerhands/isotopes/whatever seems like it should be annoying more people

I mean, I see some code that I can't understand, but I assume that justification is coming from what the code does, and not what the button is named. I'm also assuming that a button named "Buy item from X when needed" should not show coinmaster items when there are none in your inventory. Please tell me if I'm wrong on either of those
 
Last edited:
Veracity

Veracity

Developer
Staff member
The items should not be "creatable" if you have "use coinmasters" disabled, and if an item is not creatable, it should show up in the concoction list only if you have some in inventory. Sounds like there is a bug there.
 
Theraze

Theraze

Active member
And if that's the case, then NPC stores should also be brought into line, since they also ignore your setting per hee^3 in post 23.
 
S

slyz

Developer
Veracity said:
The items should not be "creatable" if you have "use coinmasters" disabled
Click to expand...
In Heeheehee's words:
heeheehee said:
this brings up an interesting point. Namely: autoSatisfyWithCoinmasters (and related features) is only intended for use with retrieve_item and the internal equivalent; the GUI is a perfectly acceptable way to use Mafia as a user (hence the "U" in "GUI"). I'm not sure that it makes sense to accept this feature, in this case.
Click to expand...

I agree that the "buy stuff from X" settings shouldn't apply to the GUI, just like they don't apply to direct commands like "buy" or "create". Maybe their description doesn't clearly convey their exact purpose.

What is really requested here, then, is another filter. There was a bit of debate about which filters we should have in the Usable section of the Item Manager. In the end, we (or was it just Roippi?) plumped for a simpler "no create" filter, with a separate "no-summon" filter. Adding more would start to seriously cramp the interface.
 
L

Laserrobotics

Member
So, status report and modified request. Posting this here because I'm not sure if it's different enough to warrant its own thread.
Status:
The Food and Booze panes of the Usable section in the Item Manager still behave as reported before i.e. with 'no create' unchecked, they will display coinmaster items regardless of the coinmaster item acquisition setting, but will respect the npc store item acquisition setting. With 'no create' checked, they will display neither of the mentioned categories. Presumably all this is still considered WAI, and it's not quite what I'm asking about anyways.

New Request: The Cookable and Mixable panes of the Creatable section in the Item Manager behave in an equivalent manner to the above, except there is no equivalent of the 'no create' filter (for obvious reasons). As such, the NPC store item acquisition setting determines whether or not it will display recipes that would require items you do not have but are in NPC stores. However, whether or not the coinmaster item acquisition setting is true, the Creatable section will always display items purchasable from the coinmasters, and the available filters (in-a-boxes, repair on explosion, use closet) are not relevant.

So what, exactly, is my request? One of two options that I can see, though you may think of others:
1) stick another checkbox in the panes of the Creatable tab that filters out 'recipes' that use coinmasters
2) make the Creatable panes respect the coinmaster item acquisition setting as they do with the NPC store item acquisition setting (the Usable panes already have the 'no create' filter which is plenty)

EDIT: adding a few things I realize I missed after reading the procedures sticky, sorry :(
I'm using v17.1, did not test in any of the daily builds since then but none of them mention Coinmasters and considering the age of the last post here I find it unlikely this topic has been revisited anyways.
I would find this feature useful because I would rather not have to stuff all of my currencies in the closet and then have to pull them whenever I actually did need to use them. Having all the coinmaster recipes displayed makes it difficult to determine what I can make with what I have, which isn't currently a filter possibility on any screen and requires that I pull up the wiki description for any items I'm not 100% sure don't require purchasing from a coinmaster.
 
Last edited:
L

Laserrobotics

Member
If I have somehow offended with my bluntness, I apologize. I tried my best to present how I see the issue and what I would like to see done, and hoped I would at minimum get a response saying why you don't want to do this. Is it too different from the original post, and as such a mistake to revive this thread rather than make a new one?
 
Veracity

Veracity

Developer
Staff member
You were not offensive in bumping this. I expect that it just didn't interest devs sufficiently to look at it again; everybody just assumed somebody else would look at it.

I've now looked back over the whole thread and I think the solution is to just make ConcoctionDatabase.refreshConcoctions filter out coinmasters based on the setting the same as it filters out NPC purchases based on the setting. That was mentioned several times.

heeheehee had a patch which changed the usage of the canPurchase method for a CoinMasterPurchaseRequest. That was not correct. He also observed that the Hermit seemed to be controlled by NPC purchases rather than Coinmaster purchases. That observation was correct, but the reason is that a worthless item (the Hermit's currency) is essentially purchasable from the General Store, since you can trade chewing-gum-on-a-string for worthless items.

I have a simple patch which I am testing...
 
Veracity

Veracity

Developer
Staff member
Well, it turns out that CoinMasterPurchaseRequest did need some work in canPurchase handling. The preference checking still goes in ConcoctionDatabase.refreshConcoctions, though.

Revision 16401
 
Crowther

Crowther

Active member
Veracity said:
He also observed that the Hermit seemed to be controlled by NPC purchases rather than Coinmaster purchases. That observation was correct, but the reason is that a worthless item (the Hermit's currency) is essentially purchasable from the General Store, since you can trade chewing-gum-on-a-string for worthless items.
Click to expand...
So I'm not as crazy as you had me believe. At least about this one issue.
 
You must log in or register to reply here.
Top