Bug - Fixed Unexpected behavior when using two items with name "rock" in KoL as keys in a map

[heeheehee]

No.

We used to have different names - that did not agree with KoL - for effects that had the same name. A Little Bit Evil, for example. We went to a lot of trouble to make KoLmafia deal with effects by effect id, always. Ditto for items.

If you want to have map_to_file deal with disambiguated items, use the unique id.

By that argument, should we collapse spooky vampire and spooky vampire (Dreadsylvanian)?

 

[Veracity]

I edited my note to talk about how monsters were an exception to that argument.
You apparently responded while I still editing.
Sorry.

 

[fronobulax]

If you want to have map_to_file deal with disambiguated items, use the unique id.

So anyplace I declare and use a map indexed by item (such as string [item] needDC) should be converted to use an int (string [int] needDC) and the corresponding operations dealing with the map should use an item id?

Thanks.

 

[heeheehee]

Hm. That does seem like an unfortunate casualty. IMO, if some serialization / deserialization pair of primitives doesn't restore a data structure perfectly, then that's a bug.

> ashq int[item] map; foreach it in $items[8042, 2108] map[it] = it.to_int(); map_to_file(map, "test.txt"); file_to_map("test.txt", map); print(to_json(map))

{ "rock" : 8042 }

 

[Veracity]

Value.java:

	public void dumpValue( final PrintStream writer )
	{
		writer.print( this.toStringValue().toString() );
	}

Like so much of this, this was written long before KoL had duplicate item names (or duplicate effect names, probably) and the "string value" of an item or effect was the unique name.

It would be easy enough to put in type checks and make items, effects, and anything else that has a unique ID print the ID instead - assuming that we can coerce integers into the item. The corresponding code in CompositeValue.read():

		Type dataType = type.getDataType( key );
...
		// Parse the value and store it in the composite

		Value value = ( index < data.length - 1 ) ?
			dataType.parseValue( data[ index + 1 ], true ) :
			dataType.initialValue();

And in Type.java:

	public Value parseValue( final String name, final boolean returnDefault )
	{
		switch ( this.type )
		{
		case DataTypes.TYPE_BOOLEAN:
			return DataTypes.parseBooleanValue( name, returnDefault );
		case DataTypes.TYPE_INT:
			return DataTypes.parseIntValue( name, returnDefault );
		case DataTypes.TYPE_FLOAT:
			return DataTypes.parseFloatValue( name, returnDefault );
		case DataTypes.TYPE_STRING:
			return DataTypes.parseStringValue( name );
		case DataTypes.TYPE_ITEM:
			return DataTypes.parseItemValue( name, returnDefault );
		case DataTypes.TYPE_LOCATION:
			return DataTypes.parseLocationValue( name, returnDefault );
		case DataTypes.TYPE_CLASS:
			return DataTypes.parseClassValue( name, returnDefault );
		case DataTypes.TYPE_STAT:
			return DataTypes.parseStatValue( name, returnDefault );
		case DataTypes.TYPE_SKILL:
			return DataTypes.parseSkillValue( name, returnDefault );
		case DataTypes.TYPE_EFFECT:
			return DataTypes.parseEffectValue( name, returnDefault );
		case DataTypes.TYPE_FAMILIAR:
			return DataTypes.parseFamiliarValue( name, returnDefault );
		case DataTypes.TYPE_SLOT:
			return DataTypes.parseSlotValue( name, returnDefault );
		case DataTypes.TYPE_MONSTER:
			return DataTypes.parseMonsterValue( name, returnDefault );
		case DataTypes.TYPE_ELEMENT:
			return DataTypes.parseElementValue( name, returnDefault );
		case DataTypes.TYPE_COINMASTER:
			return DataTypes.parseCoinmasterValue( name, returnDefault );
		case DataTypes.TYPE_PHYLUM:
			return DataTypes.parsePhylumValue( name, returnDefault );
		case DataTypes.TYPE_BOUNTY:
			return DataTypes.parseBountyValue( name, returnDefault );
		case DataTypes.TYPE_THRALL:
			return DataTypes.parseThrallValue( name, returnDefault );
		case DataTypes.TYPE_SERVANT:
			return DataTypes.parseServantValue( name, returnDefault );
		}
		return null;
	}

So, DataTypes.parseItemValue and DataTypes.parseEffectValue would have to recognize that a string which is an integer is the ID. Looking at both of those:

		if ( StringUtilities.isNumeric( name ) )
		{
			int itemId = StringUtilities.parseInt( name );
			name = ItemDatabase.getItemDataName( itemId );

			if ( name == null  )
			{
				return returnDefault ? DataTypes.ITEM_INIT : null;
			}

			return new Value( DataTypes.ITEM_TYPE, itemId, name );
		}

(and the equivalent for effects), I see that that would hold.

So, it seems:

- Change Value.dumpValue() to print the id field - the contentLong - for ITEM and EFFECT values, and we will generate maps with integers for items and effects, which will be read back correctly. No backwards compatibility needed; existing dumped maps with names will still be read back - although the current parsing ambiguity will be unavoidable - but when the map is written out again, it will be fixed.

If any other datatype needs to eventually use the ID to be dumped/restored correctly, we do this:

- Change Value.dumpValue to dump out the ID
- Change Datatypes.parseXXX to have a comparable isNumeric check to parse the ID.

We could do that with monsters, for example, by using the ID if it is non-zero, otherwise KoLmafia's (disambiguated) monster name, and parseMonster would look up by ID if isNumeric, otherwise lookup by name.

 

[Veracity]

> ashq int[item] map; foreach it in $items[8042, 2108] map[it] = it.to_int(); map_to_file(map, "test.txt"); file_to_map("test.txt", map); print(to_json(map))

{ "rock" : 2108, "rock" : 8042 }

test.txt:

2108	2108
8042	8042

Try revision 1636

 

[chown]

Huh. So, if I understand correctly, map_to_file will no longer produce human-who-doesn't-know-KoL's-item-numbers-readable files? Doesn't that seem like a bit of an overreaction? I certainly thought that was a nice feature to have. Or, is this just for 2108 and 8042, in some way that I missed?

 

[Veracity]

Doesn't that seem like a bit of an overreaction?

Apparently, no good deed goes unpunished; you either want correctness, or you don't want correctness, and when I give you the former, you complain.

How, exactly, do you propose that KoLmafia should know that "when I write this item, I can use the name, but if I write this other item, I have to use the number"? Hint: your solution cannot include a hard-coded list of items that will become incorrect at the whim of KoL.

I throw up my hands.

 

[Crowther]

I don't really mind them being less readable, but a solution would be to check to see if the names are a duplicate and only use the numbers then. I don't like that, but it does get around the hard-coded list. I don't like it, because a map can break when KoLmafia is updated.

 

[Veracity]

Exactly. As coded now, the map will always be readable, but with a mixture of names and numbers, your map file breaks at KoL's whim - as you already experienced when your file with names broke.

If we wanted to "check to see if the names are duplicate", we'd have to build a Set of duplicate names when we read items.txt and statuseffects.txt and look up the ASH item name in that set every time we want to dump the value. Not a huge effort, but if the only purpose is to make less reliable maps that KoL can break some time down the road, I don't see the point.

 

[fronobulax]

I throw up my hands.

I'm sorry, but I want things right. If that means my files are less readable then I'm ok with that. Since most of my maps are of records I'm just going to make sure the string version of an item is part of the record and the net effect will be to add one column to my files, one that contains the numeric item number. I'm fine with that and I appreciate the work, so Thank You.

 

[Theraze]

If you tell it to save an item map to file, it will save an item map. As items are named by their item ID, rather than by their name, it uses that unique identifier.

If you want your item map to have human readable names, simply define that as one of your record fields.

 

[heeheehee]

Would it be absurd to suggest something like

[2108]rock	2108
[8042]rock	8042

where the name is technically irrelevant but still meaningful to someone who wants to glance at the datafile?

 

[Veracity]

Why not? Revision 16139.

I feel a sense of attachment towards ASH and want it to be "correct", but I will leave fixes of this sort to others, henceforth.

 

[Bale]

Why not? Revision 16139.

I am deeply thankful for this change.

Purely for my own self-interest I like being able to have both item numbers and names. I like the numbers because they are necessary and I accepted that when you did it even though it meant that it would no longer be as easy to check my OCD files in a text editor. Now that the name is there I rejoice at not losing the simple readability of that while still having the usefulness of item numbers.

This was an awesome solution.

 

[Darzil]

Nice, was going to suggest this, as it's how we handle it internally with data files, for the same reason.

 

[Crowther]

Saving both seems obvious now that someone mentioned it.

 

[chown]

That is the sort of solution that I had hoped for.

I'm very sorry, Veracity. I guess you've taken my complaints as being about decisions that you had made. I wasn't really even complaining per se; I just wanted to make sure that whatever decision you ultimately come to is one that you've thought through enough to defend against such complaints. I was really just trying to protect myself from needing to adjust my own coding practices multiple times as a result of an unnecessarily-drawn-out sequence of changes in Mafia, that all stem from a single (essentially) change in KoL. Personally, I think it's a good idea to try to hash out these issues sooner rather than later.

I want to make it clear that I'm immensely grateful for all of the work you've already put into Mafia. I don't expect anything more from you. You don't need to fix bugs. Even without doing so, you have my gratitude and respect. Even if the original bug that I reported here had been left untouched, I would have dealt with it, and I would have continued to find Mafia to be a great tool! (and due in no small part to your own efforts in particular!) Your original fix was unquestionably an improvement over that. Personally, I find the current approach to be even better. But I want to make it abundantly clear that I was already very grateful for the original fix, and even the switch to all-id files. I'm sorry that I didn't make that clear in my earlier responses.

I intend (at least, at the moment) to continue to report bugs, make suggestions about changes, and yeah, even criticize things that I don't like about Mafia. But I'm aware that I could wind up hurting peoples' feelings in doing so. I'm not always as courteous or cognizant of other peoples' feelings as I should be. Please, please, if anything I post here makes you unhappy, I am so sorry, and I want you to know how much I appreciate what you've already done! You can just ignore what I write, or respond brusquely, or whatever works for you. This goes for everyone in the Mafia community. I'm an unhappy person who complains about everything. It's just how I am. For what it's worth, I'll try to curb my impulse to complain, and I'm sorry when it gets the best of me.