Feature - Implemented Add "discardable" and "questitem" attributes for Item Database

[Catch-22]

After looking into how we could implement this, I came to the realization that KoLmafia currently has no properties for items which have "cannot be discarded" in their description or items which have "Quest Item" in their description.

In order to implement a discard command properly, I think there first needs to be a discardable property in the database.

I can't actually think of use case for a "questitem" property (most of them are untradeable and not discardable already). I am proposing it for both future-proofing purposes and for KoLmafia data to be analogous with KoL data. If I were implementing this, I'd rather do them both at the same time than one now and one later when a use case might come up.

I'd be happy to take a crack at this myself, but I'd like some feedback from the community/devs first.

 

[Bale]

No need for a quest item property, We already have !is_displayable() which serves that purpose. Quest items are the only items which cannot be displayed.

Discardable is another matter. I'd very much like it since right now OCD requires the following fuction:

boolean is_discardable(item it) {
	switch(it) {
	case $item[useless powder]:
	case $item[burst hellseal brain]:
	case $item[shredded hellseal hide]:
	case $item[torn hellseal sinew]:
	case $item[Instant Karma]:
		return true;
	}
	return false;
}

 

[wrldwzrd89]

Completely agreed on the need for discardable support. KoLmafia ought to also have a "discard" CLI command for dealing with such items.

 

[Theraze]

There's actually a FReq about the CLI command already. That might be part of why the proxy/datafile request is being made...

 

[Fluxxdog]

No need for a quest item property, We already have !is_displayable() which serves that purpose. Quest items are the only items which cannot be displayed.

Discardable is another matter. I'd very much like it since right now OCD requires the following fuction:

boolean is_discardable(item it) {
	switch(it) {
	case $item[useless powder]:
	case $item[burst hellseal brain]:
	case $item[shredded hellseal hide]:
	case $item[torn hellseal sinew]:
	case $item[Instant Karma]:
		return true;
	}
	return false;
}

Heh, worthless items fall on that list too as does the slime convention swag bag. Not saying it's a good idea to discard them, but, you know, for thoroughness sake.

 

[Catch-22]

I refactored the way access types are stored in the database for this one. I think it's better this way and hopefully the devs will agree (I put a fair bit of time in to getting it right).

Note, this is the database refactoring only, it adds no new commands to ASH at this stage.

In a nutshell, I got rid of the "all" type and now the type can be any combination of "display" (not a quest item), "gift", "trade" and "discard". If the item has none of these attributes, it's "none" (so that would be a quest item that can't be traded, gifted or discarded). This allows us to mirror the flags that KoL has, so we can have a quest item that is also considered discardable, for example.

The patch file exceeds the .patch file attachment size, because it also contains all the changes to tradeitems.txt which reflects the new format for multiple accessType attributes.

 

[Veracity]

Ever looked at a "rel string"? When you get an item in a fight (and perhaps other ways) it often appears as an attribute of the table that holds the image and acquisition message. Look at the code in FightRequest:

		else if ( name.equals( "table" ) )
		{
			// Items have "rel" strings.
			String cl = node.getAttributeByName( "class" );
			String rel = node.getAttributeByName( "rel" );
			if ( cl != null && cl.equals( "item" ) && rel != null )
			{
				AdventureResult result = ItemDatabase.itemFromRelString( rel );
				ResultProcessor.processItem( true, "You acquire an item:", result, (List) null );
				return;
			}
...

What's in a rel string? Look at ItemDatabase.itemFromRelString() and see:

	private static Pattern RELSTRING_PATTERN = Pattern.compile( "([\\w]+)=([^&]*)&?");

	// "id=588&s=118&q=0&d=1&g=0&t=1&n=50&m=0&u=.&ou=use"
	//   id = item Id
	//   s = sell value
	//   q = quest item
	//   d = discardable
	//   g = gift item
	//   t = transferable
	//   n = number
	//   m = multiusable
	//   u = how can this be used?
	//	 e = Eatable (food/drink) (inv_eat)
	//	 b = Booze (inv_booze)
	//	 q = eQuipable (inv_equip)
	//	 u = potion/Useable (inv_use/multiuse)
	//	 . = can't (or doesn't fit those types)
	//  ou = "other use text" which is used to make drinks show
	//	 "drink" instead of "eat" and for items which over-ride
	//	 the word "use" in links, like the PYEC or scratch 'n'
	//	 sniff stickers.

I mention this only in response to your comment about "mirroring the flags that KoL has". Perhaps it would be more compact to actually use KoL's flag letters and list only those that are non-zero. So, "q,d" would be a discardable quest item. Anything without "q" would be displayable. And so on.

Not that I've seen your patch, yet, so I don't know how compact or not your data file is currently.

Edit: I looked at your patch, a bit. I like it, in general.

I think you need to update DebugDatabase.parseAccess() to return a new-style access string, so that when we do "checkitems" it will validate our "access" flags against the actual item desc text. I would have assumed you'd have done that anyway, to generate the new access fields. Did you update the data file by hand?

 

[Catch-22]

Ever looked at a "rel string"?

I mention this only in response to your comment about "mirroring the flags that KoL has". Perhaps it would be more compact to actually use KoL's flag letters and list only those that are non-zero. So, "q,d" would be a discardable quest item. Anything without "q" would be displayable. And so on.

I did see some of the relstring stuff, I didn't want to change the structure too much for people who are used to the existing format. I know "display" means basically "not a quest item" and I figured that's just the way we were going to do things. Wouldn't be too much trouble to change though, if not now, later down the track.

I think you need to update DebugDatabase.parseAccess() to return a new-style access string, so that when we do "checkitems" it will validate our "access" flags against the actual item desc text. I would have assumed you'd have done that anyway, to generate the new access fields.

That is exactly what I did do (I don't blame you for missing it in the fairly lengthy patch file, if it's really not in there, let me know). Editing that file by hand would just be silly for a lazy programmer such as myself

Also here's attached the patch for an "is_discardable" ASH command, which becomes super easy with the above patch applied.

 

[Veracity]

What I would like is this:

The "access" field is a series of flags:

q = quest item
g = gift item
t = tradeable
d = discardable

public static final int ATTR_QUEST = 0x00000001;
public static final int ATTR_GIFT = 0x00000002;
public static final int ATTR_TRADEABLE = 0x00000004;
public static final int ATTR_DISCARDABLE= 0x00000008;

Note that this requires shuffling some flags. It also gets rid of ATTR_DISPLAYABLE; that is simply ~( attrs & ATTR_QUEST )
(You don't actually NEED to shuffle flags. It just seems a bit neater to my eye. No biggie.)

The following code in DebugDatabase to generate it from the item description:

	private static final StringBuilder appendAccessType( StringBuilder b, String type )
	{
		if ( b.length() > 0 )
		{
			b.append( "," );
		}
		return b.append( type );
	}

	public static final String parseAccess( final String text )
	{
		StringBuilder b = new StringBuilder();

		if ( text.contains( "Quest Item" ) )
		{
			appendAccessType( b, "q" );
		}

		if ( text.contains( "Gift Item" ) )
		{
			appendAccessType( b, "g" );
		}

		if ( !text.contains( "Cannot be traded" ) )
		{
			appendAccessType( b, "t" );
		}

		//We shouldn't just check for "discarded", in case "discarded" appears somewhere else in the description.
		if ( !text.contains( "Cannot be discarded" ) && !text.contains( "Cannot be traded or discarded" ) )
		{
			appendAccessType( b, "d" );
		}

		return b.length() == 0 ? "none" : b.toString();
	}

And corresponding code in ItemDatabase to parse that kind of string and set it in the item attrs.

We do not need an is_discardable() ASH function. Instead, we need item proxy fields in ProxyRecordValue.java:

item.quest
item.gift
item.tradeable
item.discardable

We already have is_tradeable(), is_giftable(), and is_displayable(). We can keep those for backwards compatibility.

 

[Catch-22]

What I would like is this...

Yes, I like those ideas too. The only suggestion I'd make in this case is that we ditch the comma separation entirely, go with "qgtd" instead of "quest, discard, giftable, tradeable", I think "q,g,t,d" looks kinda ugly.

Edit: Oh, and get rid of "none" seeings as it becomes irrelevant. If an item is "not tradeable, not a quest item, not discardable, not a gift" then the accessType string is just empty.

We do not need an is_discardable() ASH function. Instead, we need item proxy fields in ProxyRecordValue.java:

item.quest
item.gift
item.tradeable
item.discardable

We already have is_tradeable(), is_giftable(), and is_displayable(). We can keep those for backwards compatibility.

Funny thing is, I went to add "discardable" as a proxy record and realized "tradeable" and "giftable" weren't there, so I figured the ASH functions were what we'd be using.

 

[Veracity]

The only suggestion I'd make in this case is that we ditch the comma separation entirely, go with "qgtd" instead of "quest, discard, giftable, tradeable", I think "q,g,t,d" looks kinda ugly.

Well, yeah, "q,g,t,d" looks ugly, but "qgtd" looks worse, in my opinion. With the commas, it looks like the letters are flags - as they are.

Is "gtd" the same as "tdg"? Yes - although we should probably have a "canonical" order for the flags - the order that DebugDatabase generates them. To my eye, "g,t,d" and "t,d,g" look more likely to be equivalent than if we had single characters all run together.

Note also that multiple usages are comma separated.

And note finally that if we have commas, we have the option of adding additional flags, a la the "rel" string - perhaps with values, even.

So, I still like commas.

 

[Catch-22]

I guess I should've waited a little before going back to it...

I went without commas because it allowed for cleaner code. I can treat the "flags" string as a charArray and iterate over it in a very simple way.

I also added the proxy records in, they work well. I'll hold off posting for now until I find time to move the flags back to using commas, no time at the moment.

 

[Veracity]

One other thing to note: this is a non-backwards-compatible change to tradeitems.txt. That's not a problem, but you need to increment the file format version - the number on the first line of the file. Currently 3. Make it 4. And change

	public static final int TRADEITEMS_VERSION = 3;

in KoLConstants.txt to correspond.

 

[Rinn]

You should probably use an enum instead of the hardcoded string comparisons.

 

[Veracity]

...which will take a hard-coded string comparison to determine which enum is involved, since we are looking at text which is read from a file.

Unless I am missing what you are suggesting.

 

[Rinn]

There are multiple checks against the same string, so to prevent possible typos they should probably be defined once elsewhere.

 

[Catch-22]

You should probably use an enum instead of the hardcoded string comparisons.

I agree. It's something I should make more a habit of doing, really.

I've attached the latest patch, hopefully this covers everything. I got rid of the isDisplayable function internally (it's pretty pointless now that we have isQuestItem), so anywhere that was using isDisplayable internally is now using !isQuestItem (and anything using !isDisplayable, is now using isQuestItem). For ASH scripters, the is_displayable() function will be business as usual.

 

[Veracity]

Revision 11432. Thanks.

 

[Bale]

Thanks. I've added it to OCD, but there's no functional difference until KoL adds a new item that is practical to discard so I won't roll out a new version yet. Someday though, I'll be really happy when a new item shows up as discard bait in mafia's data files.

boolean discard_bait(item it) {
	return (is_discardable(it) && autosell_price(it) < 1);
}

 

[Rinn]

Useless powder? Oh never mind using it removes it from your inventory.