Convert HeldObject to a List

[Jolly-Alpaca]

Automation Branch

• Checks if there is chest space in the network for all items in the HeldObject list
• Does nothing if there is not enough space for every item
• If enough space is found, stores each item in their respective chest, prioritizing existing stacks to empty chest space. Different items can be stored in different chests.
• TakeObjectFromMachine no longer called from TryPutHeldObjectInStorage. Instead, WorkFinder handles resetting the machine state by calling RemoveHeldObject after trying to store every item in the list if at least one item is stored successfully (more details in code documentation, but favors preventing duplication of items over ensuring all items are delivered).

Junimo Branch

• Checks if there is chest space for the first item in the HeldObject list
• Creates assignment if chest space is found
• Tries to store subsequent items in the HeldObject list in the same chest as the first item
• If unsuccessful, checks the network for another chest to store the item in, prioritizing existing stacks to empty chest space.
• If chest space is found, creates assignment to store item in found chest.
• Otherwise, defaults to existing behavior: JunimoQuitsInDisgust throwing all remaining items in the HeldObject list on the ground as debris

I chose to implement the two branches a bit differently because I thought the JunimoQuitsInDisgust logic is funny, but doesn't translate well to the automation route as the player is unlikely to notice right away and would come back to a shed full of debris. There is also less time between checking for storage space and actually storing the item in the automation route, meaning less chance of the state of storage changing before the item is stored, preventing it from being able to be stored.

Note: I haven't tested with garden pots yet, so the actual list aspect hasn't been tested yet, so some changes may be required once I start working on converting that. Existing machines appear to all work correctly.

[NermNermNerm]

I've only scanned over the code and don't have detailed comments yet, but, overall, the name 'HeldObject' probably needs to be changed. It was picked to be that to match the game code, but the game concept of "HeldObject" is kinda sloppy, and we're going to get into trouble if we just pluralize it. But it's probably more than just a name change...

1. Sometimes we're using 'HeldObject' just to test and see if the machine has any product, and I think querying HeldObject might just have side-effects in the GardenPot world.
2. Elsewise we're using it to say "Hey, can we put the output of the machine into this chest?" (There are probably more clear ways to express that idea).
3. There's already a separate function for actually taking the item out of the machine (TakeItemFromMachine) so it's already on some thin ice.

Is it going to be generally possible to ask what the full list of items a given machine is going to produce without actually processing it when Garden Pots arrive? I'm thinking in particular about the scenario where you've got a chest that's almost full - full enough to store the some of the products of the plant but not all of them. Ideally it would work like it does now - if you're in the room, no Junimo pops out (because a complete task can't be generated) and if you leave the room nothing happens either, and in both cases, the player can manually unload the machine.

I haven't looked at it as hard as you have, but I think it's going to be the case that (barring some serious shenanigans), we're not going to be able to know what all the side-products will be until we actually harvest it. However, I think we will know what the maximum number of different items that might pop out would be. (I think it's 2? One for the main product, and one for either Sunflower Seeds or random extra produce.)

If that's the case, then maybe GameMachine's methods & properties relating to this would be:

bool HasOutput
Item MainProduct
int PossibleByProductsCount
List<Item>TakeItemsFromMachine()

I'm thinking that we change GameStorage.IsPossibleStorageForMachinesOutput to take a GameMachine as its input, rather than an Item like it is now and have it look at PossibleByProductsCount and ensure that there's at least that many empty slots in the chest to qualify.

Seems to me like that would be the way to ensure we don't get sideways - at the expense of sometimes not automating stuff that could conceivably be automated.

I can definitely see some bug reports coming in where people think their Junimos are broken, when actually it's just that the chest is almost full, but it'd only be for machines that have that by-product idea, which isn't many. If we actually did get that kind of feedback, I think we could do something about it by having the Junimos walk over to the full chest every once in a while and emit a grump emote at it.

[NermNermNerm]

Oh, and random advice from an old programmer who's Made That Mistake Before(tm): Any time you find yourself having to deal with an exception like you get when you change a collection you're iterating over, it's usually a sign that your design could use another iteration. You can patch over it, but those patches will start to creep.

[Jolly-Alpaca]

You are correct. For garden pots, the max distinct output is 2: sunflower + sunflower seeds, fiber + mixed seeds, wheat + hay, and 1 varying quality crop + x base quality crops. You are also correct that we do not know what the actual output is until harvest because the secondary products are not guaranteed and the quality of the main crop and quantity of secondary products are determined at time of harvest. As such, I have changed the Garden Pot code to harvest the crops when checking for chest space so the correct chests can be found (as opposed to finding a chest for a base quality crop only for the harvest to end up being a different quality).

Of course this ends up with some hacky-ness to make it look good when the junimo is harvesting. For example, if we don't want the crop randomly going poof (being harvested) with nothing around it and, instead, want the harvest animations to be done when the junimo reaches the garden pot (and for the farmer to still be able to harvest the pot themselves), then this approach still requires serializing the list into modData, in case rerunning the harvest logic changes the output. Admittedly, during testing, if a pot produced sunflower seeds once, every sunflower I grew in that pot that day produced sunflower seeds. I think the quantity of secondary produce did vary on subsequent harvests, however.

This will also require suppressing/delaying animations and sound effects, which will require some reflection, at least for bushes, since animations are controlled by some private fields. I haven't finished that part yet, but will now hold off doing that since it may not be required depending on how the concept of "heldObject" is changed.

If junimos looking like they are broken when a chest is full is a concern, the output could just be turned into debris 😼. That will make it clear. For a legitimate idea, junimatic could broadcast a message to the player in the in-game chat that the junimo cannot store X because no storage space is available. Could even get creative with it and make it seem like the junimo itself is entering the message in the game chat. There could be a flag in JunimoPortal to prevent spamming the chat, but it may be tricky to figure out how to clear the flag. Day update? Every X in game hours? Using the machine that flagged the message wouldn't work since the player could manually empty the machine without emptying the chest(s). Clearing the flag upon a successful harvest of another machine wouldn't work either, as other machines may have output with existing stacks in the chests.

Regarding the changing the collection you're iterating over, it surprisingly never crashed or threw an error (in SMAPI at least). It just didn't do anything with additional items in the list.

[NermNermNerm]

I put my ideas into this PR:

https://github.com/NermNermNerm/Junimatic/pull/15/files

I'd REALLY like to make it so that the Junimo doesn't set off on its mission if there isn't a single chest it can go to with confidence it can find room. For the fully-automated case, I think that has to be how it works. For the animated case, I can sorta see my way clear to the idea that before the Junimo tosses its inventory and heads to the barn it at least looks to see if there's a reachable chest...

But really, the game's Chest code is frighteningly wonky, and for the benefit of being able to debug it, I'd really prefer it to be where a chest can either take everything or it's not considered.

Exactly how it conveys the full-chest problem isn't, to me, particularly important. I feel like players are just going to be able to see a mess of non-harvested stuff, look at the chest and see what the problem is without having to look at SMAPI logs or anything.

[Jolly-Alpaca]

Closing as second branch created for this change