Bug - Fixed multi-usable information needs updating
- Thread starter Theraze
- Start date
Fixing this is as simple as changing "usable" to "single" in items.txt, but ItemDatabase.isMultiUsable() should probably agree with item.multi. I'm guessing it's the proxy field that needs to be changed. I'll look into that tomorrow if no one beats me to it.
Bale
Minion
All of the DoD potions need updating also!
Using 1 blessed large box...
milky potion is multiusable, but KoLmafia thought it was not
You acquire an item: milky potion
swirly potion is multiusable, but KoLmafia thought it was not
You acquire an item: swirly potion
bubbly potion is multiusable, but KoLmafia thought it was not
You acquire an item: bubbly potion
smoky potion is multiusable, but KoLmafia thought it was not
You acquire an item: smoky potion
cloudy potion is multiusable, but KoLmafia thought it was not
You acquire an item: cloudy potion
effervescent potion is multiusable, but KoLmafia thought it was not
You acquire an item: effervescent potion
fizzy potion is multiusable, but KoLmafia thought it was not
You acquire an item: fizzy potion
dark potion is multiusable, but KoLmafia thought it was not
You acquire an item: dark potion
murky potion is multiusable, but KoLmafia thought it was not
You acquire an item: murky potion
Finished using 1 blessed large box.
Using 1 blessed large box...
milky potion is multiusable, but KoLmafia thought it was not
You acquire an item: milky potion
swirly potion is multiusable, but KoLmafia thought it was not
You acquire an item: swirly potion
bubbly potion is multiusable, but KoLmafia thought it was not
You acquire an item: bubbly potion
smoky potion is multiusable, but KoLmafia thought it was not
You acquire an item: smoky potion
cloudy potion is multiusable, but KoLmafia thought it was not
You acquire an item: cloudy potion
effervescent potion is multiusable, but KoLmafia thought it was not
You acquire an item: effervescent potion
fizzy potion is multiusable, but KoLmafia thought it was not
You acquire an item: fizzy potion
dark potion is multiusable, but KoLmafia thought it was not
You acquire an item: dark potion
murky potion is multiusable, but KoLmafia thought it was not
You acquire an item: murky potion
Finished using 1 blessed large box.
Last edited:
ItemDatabase.isMultiUsable() assumes that restores and things that are consumed in multiples are multi-use, unless explicitly marked single. It assumes that other things are multi-use unless explicitly marked multiple.
The proxy record multi only considers whether things re explicitly marked multi-use.
If you want them consistent, I guess we have the following options:
1. Change ItemDatabase.isMultiUsable() to no longer make assumptions. Upside - we'll get a lot more reports and can fix the database. Downside - far more server hits til we do.
2. Change proxy record multi to make the same assumptions as ItemDatabase.isMultiUsable().
3. Change the existing records where neither multiple or single are currently marked using the assumptions in ItemDatabase.isMultiUsable(), choose either single or multiple as the assumption for future items, rather than usable, wait for the reports to fix things. Remove the assumptions from ItemDatabase.isMultiUsable() (subset of option 1).
For the scale of this, currently we have 1080 items marked usable, 725 marked multiple and 5 marked single.
The issue with the ! potions was that they were marked both multiple and single! Removed single.
r13365 for ! potion and Pup Tent fixes.
The proxy record multi only considers whether things re explicitly marked multi-use.
If you want them consistent, I guess we have the following options:
1. Change ItemDatabase.isMultiUsable() to no longer make assumptions. Upside - we'll get a lot more reports and can fix the database. Downside - far more server hits til we do.
2. Change proxy record multi to make the same assumptions as ItemDatabase.isMultiUsable().
3. Change the existing records where neither multiple or single are currently marked using the assumptions in ItemDatabase.isMultiUsable(), choose either single or multiple as the assumption for future items, rather than usable, wait for the reports to fix things. Remove the assumptions from ItemDatabase.isMultiUsable() (subset of option 1).
For the scale of this, currently we have 1080 items marked usable, 725 marked multiple and 5 marked single.
The issue with the ! potions was that they were marked both multiple and single! Removed single.
r13365 for ! potion and Pup Tent fixes.
Both of those need to match whatever is in UseItemRequest (I'm guessing that's where it's at), which decides whether to actually multi-use an item.
Ok, I think I've worked it out.
Single in items.txt refers to being singly usable in combat, not from inventory. isMultiUsable() should therefore not be looking at ATTR_SINGLE, it should if anything be looking at ATTR_USABLE, and possibly should just be returning a check if ATTR_MULTIPLE is present. It appears that isMultiUsable() is only used by the check against the Rel string and was added by Veracity in 13342 and updated in 13359.
I'm going to chance ATTR_SINGLE in that function to ATTR_USABLE, and put single back on the ! potions, and take it off Pup tent, replacing it with USABLE.
I'll leave the decision on what else to do to Veracity, who must have had good reasons to do things that way.
Single in items.txt refers to being singly usable in combat, not from inventory. isMultiUsable() should therefore not be looking at ATTR_SINGLE, it should if anything be looking at ATTR_USABLE, and possibly should just be returning a check if ATTR_MULTIPLE is present. It appears that isMultiUsable() is only used by the check against the Rel string and was added by Veracity in 13342 and updated in 13359.
I'm going to chance ATTR_SINGLE in that function to ATTR_USABLE, and put single back on the ! potions, and take it off Pup tent, replacing it with USABLE.
I'll leave the decision on what else to do to Veracity, who must have had good reasons to do things that way.
Well, isMultiUsable looks (mostly) good:
MULTIPLE, HP_RESTORE, MP_RESTORE, and HPMP_RESTORE are assumed to be multiple - unless explicitly given the USABLE attribute. (I'll note that it makes no sense for a MULTIPLE item to be given that attribute; it was intended for the restores.)
Anything else CAN be multiusable - if it is given the MULTIPLE attribute. So, probably this:
We could go either way in whether restores should be multi-usable by default.
- if we say "yes", we will get a bug report when a user tries to multi-use one and it doesn't work. We will also be told by the new "relstring" code that we have it wrong.
- If we say "no", we will get a bug report when a user tries to use several of them and complains that KoLmafia is using them one at a time. Again, the new "relstring" code will tell us that we have it wrong.
The so-called "good reasons" I had for assuming restoratives were multi-usable was that we'll get bug reports whichever way we go, but most of them are multiusable, so we'll get fewer bug reports that way.
The "good reason" to go the other way is that single-using a multi-usable thing works, whereas multi-using a single-use thing does not. So, I might have made the wrong decision.
Similarly, we used to get lots of bug reports of the latter sort when we assumed that "potions" or "candy" things were multi-usable - because the large majority of them are - and people ran in to the occasional ones that were not. Now, when we see a new potion, unless we can test it right away - by looking at the inventory page - we enter them in as "usable" and change them later.
That is the thing, though - we don't automatically mark an item as "hp", "mp", or "hpmp". We initially enter tham as "usable" and then change them to be restoratives - and we could/should check at that time whether they are single or multiusable, and if the default is multi, enter them as, say, "hp,usable".
Well, isMultiUsable should be (slightly) changed as I listed above. We'll have to decide if we want to remove the "restoratives are multi-usable" default - and then add "multiple" to all of the ones that currently are. I'm inclined to not do that and just assume the dev who marks restoratives as such in items.txt will mark them as single usable, as I mentioned, if they are not multiusable.
Huh. I should just make the "log new items using relstring" function mark items as multiple if the relstring says they are - rather than just saying "usable", logging them to the user - and then changing them after the fact. That will make it easy to decide if you need to say ",usable".
MULTIPLE, HP_RESTORE, MP_RESTORE, and HPMP_RESTORE are assumed to be multiple - unless explicitly given the USABLE attribute. (I'll note that it makes no sense for a MULTIPLE item to be given that attribute; it was intended for the restores.)
Anything else CAN be multiusable - if it is given the MULTIPLE attribute. So, probably this:
Code:
switch ( useType )
{
case KoLConstants.CONSUME_MULTIPLE:
return true;
case KoLConstants.HP_RESTORE:
case KoLConstants.MP_RESTORE:
case KoLConstants.HPMP_RESTORE:
return ( attributes & ItemDatabase.ATTR_USABLE ) == 0;
default:
return ( attributes & ItemDatabase.ATTR_MULTIPLE ) != 0;
}- if we say "yes", we will get a bug report when a user tries to multi-use one and it doesn't work. We will also be told by the new "relstring" code that we have it wrong.
- If we say "no", we will get a bug report when a user tries to use several of them and complains that KoLmafia is using them one at a time. Again, the new "relstring" code will tell us that we have it wrong.
The so-called "good reasons" I had for assuming restoratives were multi-usable was that we'll get bug reports whichever way we go, but most of them are multiusable, so we'll get fewer bug reports that way.
The "good reason" to go the other way is that single-using a multi-usable thing works, whereas multi-using a single-use thing does not. So, I might have made the wrong decision.
Similarly, we used to get lots of bug reports of the latter sort when we assumed that "potions" or "candy" things were multi-usable - because the large majority of them are - and people ran in to the occasional ones that were not. Now, when we see a new potion, unless we can test it right away - by looking at the inventory page - we enter them in as "usable" and change them later.
That is the thing, though - we don't automatically mark an item as "hp", "mp", or "hpmp". We initially enter tham as "usable" and then change them to be restoratives - and we could/should check at that time whether they are single or multiusable, and if the default is multi, enter them as, say, "hp,usable".
Well, isMultiUsable should be (slightly) changed as I listed above. We'll have to decide if we want to remove the "restoratives are multi-usable" default - and then add "multiple" to all of the ones that currently are. I'm inclined to not do that and just assume the dev who marks restoratives as such in items.txt will mark them as single usable, as I mentioned, if they are not multiusable.
Huh. I should just make the "log new items using relstring" function mark items as multiple if the relstring says they are - rather than just saying "usable", logging them to the user - and then changing them after the fact. That will make it easy to decide if you need to say ",usable".
OK, revision 13367 does that. If we register a new item via its relstring, the line we print out that will go into items.txt will say "usable" or "multiple" as appropriate. If we later decide that the item is an hp, mp, or hpmp item, we can add the "usable" attribute if it was registered as usable. If it was registered as multiple, that continues to be the default for a restorative, so we don't need to add the secondary attribute.
Both item.usable and item.multi could use some love. I'd say that they should call ItemDatabase.isUsable and ItemDatabase.isMultiUsable, respectively, except I am a little confused about why isUsable considers food, drink, familiars, and such as "usable". The only place where that is called is in DebugDatabase.checkPotions, which looks for "usable" things that might grant an Effect, and registers it if it is previously unknown. I can't remember the last time I used the "checkpotions" command, but, seems like that doesn't really want to look at familiars or zapping wands, anyway.
I think the proxy field accessors should call isUsable and isMultiUsable and we should make sure that those last two return exactly what makes sense.
I think the proxy field accessors should call isUsable and isMultiUsable and we should make sure that those last two return exactly what makes sense.