Feature - Implemented familiar Data Type knows about drop counters explicitly

soolar

Member
There are a lot of familiars that can drop a certain amount of items per day, and they each have a property that tracks how many have dropped that day. A lot of scripts have to manually set up a switch case associating every familiar that has a drop with its specific drop counter, and then they also have to update that list every time a new one is added.

I think it would be awesome if familiars had two extra fields: a string that says what the drop property is named, and an int that said how many could drop in one day. That or just two ints, a max drop, and a drops so far. Maybe also an item saying what it is that drops.

Also cool would be a function, boolean [familiar] drop_familiars() that just returns a list of all familiars that can drop anything, but that's not really necessary since one could just iterate through all familiars and check for drops that way if needed.

Or, if you want to avoid bloating the familiar data type, just some functions that operate on familiars and return info about their drop progress, if applicable, would be very much appreciated.

I dunno what the general policy is on letting people contribute to KoLmafia, but I'd be willing to implement any of these myself.
 

lostcalpolydude

Developer
Staff member
I dunno what the general policy is on letting people contribute to KoLmafia, but I'd be willing to implement any of these myself.

Anyone can post patches. Someone that's interested in looking over them will likely eventually check it out (or not so quickly, depending on many random factors mostly coming down to interest and available time), and code is likely to go in slightly altered from what's posted. It could just be whitespace fixing, or good code might be moved to somewhere else to better fit how things are organized.

Consistently posting good patches is how people tend to get commit access for the project.

That said, I don't have any thoughts on this particular feature.
 

fronobulax

Developer
Staff member
Not sure whether I care about the feature or but I have walked several patches through commit. If you post a patch and badger me with PMs I will either commit it, change and commit or tell you why I won't. The last two times one has been committed and one is in limbo because I saw some funny stuff while testing it and neither I nor the contributor have taken the time to track that down. My suspicion is that the patch exposed a previously undocumented feature of KoLmafia but...
 

soolar

Member
I'll look in to making and posting a patch when I run out of ideas for other additions to chit.
 

soolar

Member
Alright, I whipped up a patch. It was a fair bit more work than I expected because drop familiars were all being tracked on a case by case basis. I unified how drop familiars are tracked (except for ones with complex stuff involved like the organ grinder), which made a fair bit of code elsewhere much nicer to maintain. Any time a new drop familiar is added now, barring any extra complexity to how it drops stuff, all you need to do is add some stuff in FamiliarData.java and hook in tracking the drops, so, two files to fiddle with instead of quite a few.

And then I exposed all that to ash as well.

Let me know if I made any mistakes, I don't have all of these familiars so I can't test every single one. Also lemme know if I made any formatting mistakes and whatnot, since I am fairly interested in continuing to contribute.
 

Attachments

  • famdrops.patch
    27.2 KB · Views: 55

lostcalpolydude

Developer
Staff member
So the first issue is with whitespace. Blank lines between groups of imported packages are removed in your patch in DailyDeedsPanel. I don't see any other whitespace issues.

For the stuff in FamiliarData, each familiar has to be listed 3 times. I think that would be better as an Object[][] with a block for each familiar. Update the functions accessing that data, and everything outside of FamiliarData can stay the same for accessing it. Some examples of that are in ClanLoungeRequest.

I haven't looked too closely at the changes in DailyDeedsPanel or CharPaneDecorator, but I assume those sections give the same GUI output as before.
 

soolar

Member
Yep, same GUI output as before (at least, I am 99.9% sure it will also be the same as before for the familiars I don't have).

That does sound like a better idea, I will look in to doing that.

As for the whitespace, huh, I didn't do that... But from looking at it, it looks like Eclipse automatically added the import for FamiliarDatabase there, so it must have "cleaned up" the whitespace while it was at it. My bad.

I'll address all of that and get back to you!
 

soolar

Member
Alright, here's the revisions! I made double sure that eclipse didn't randomly delete a bunch of whitespace this time -_-

Also, rather than an Object[][] I made a new class inside FamiliarData called DropInfo, because that way it's much more clear. So I went with a DropInfo[].
 

Attachments

  • famdrops2.patch
    25 KB · Views: 54

Veracity

Developer
Staff member
OK, this was very nice. I did make some changes for revision 15959:

- minor formatting: we do not use "if(" or "for(". Instead, we use "if (" and "for (". Yes, I understand that DailyDeeds had examples of the first of those. I fixed them, too. :)
- methods - other than constructors - start with a lower case letter. So, getDropInfo(), not GetDropInfo()
- proxy fields which have multiple words separate those words with "_". So, "drop_name", not "dropname"
- For any familiar which drops a single kind of item, I added that as an AdventureResult, and added the drop_item proxy field to fetch it. Unfortunately, that is insufficient to tell if a familiar has a drop, since the Fist Turkey and the Cotton Candy Carnie can drop multiple things, but for the others, it might help a script in setting a goal, say.

I really like the code simplification in the compact sidepane and Daily Deeds panel that this provides.

Thank you!
 

lostcalpolydude

Developer
Staff member
I just added Puck Man support in 15961. This required reloading DROP_FAMILIARS at every login, which required a small amount of refactoring, but I was surprised at how little was needed.

The issue now is that Daily Deeds will show an entry for power pills twice if you have both familiars, I think. I don't plan to ever have both familiars, so I can't really test any fix for that.
 

Veracity

Developer
Staff member
I bought one Puck Man with a Mr. A I donated for and a second Puck Man with a Mr. A that I purchased in the mall. That's the only time I have ever done that, but it just wasn't worth it to me to spend more money out of my pocket to get something that I only need for testing with KoLmafia.

The good news is that I do have both familiars, so I can do that kind of testing.
 

Veracity

Developer
Staff member
Revision 15962 will not show a counter which shares a tracking setting (_powerPillDrops, for example) with a familiar we've already shown.
 

soolar

Member
Awesome, thanks for the feedback Veracity, I will keep that in mind when I think of something else I'd like to write a patch to mafia for! :D
 
Top