Reaper does not pick up harvested crops

[x-qq]

Describe the bug

Reaper (item) description seems to indicate that it's supposed to cut down and pick up crops

https://github.com/CleverRaven/Cataclysm-DDA/blob/master/data/json/items/vehicle/farming.json#L31

However, currently it only cuts down crops and leaves them on the ground.

There seems to be some code specifically for reaper picking up the harvested items

https://github.com/CleverRaven/Cataclysm-DDA/blob/master/src/vehicle_use.cpp#L1461

but for reasons unknown it does not seem to be working.

Steps to reproduce

1. Spawn a Reaper Tractor and add a storage container to it (box/trunk/etc)
2. Turn on the reaper and drive over a ready-to-harvest crop
3. Harvested crops remain on the ground

Expected behavior

Everything that is harvested by reaper should end up in vehicle's storage spaces as long as there is free space.

Screenshots

Versions and configuration

Game verson: cdda-linux-tiles-x64-2022-06-12-1440 Mod list: default

Additional context

No response

[mqrause]

Regular reapers don't have any cargo space, so they can't pick anything up. The vehicle part mentions installing cargo space elsewhere in the vehicle. I'm not super confident in my understanding of the vehicle code, but it looks like that still wouldn't work with the current code, because it only checks the reaper itself. It should work with advanced reapers.

[x-qq]

The vehicle part mentions installing cargo space elsewhere in the vehicle.

That has been done and mentioned in the step 1 of reproduction steps

[PatrikLundell]

/confirmed

Spawned auto tractor. Had it reap a crop with the auto reaper and saw the harvest stored in the reaper itself.

Reloaded the save, spawned an auto tractor, removed the auto reaper and installed a regular reaper. The regular reaper couldn't be installed where the auto reaper had been installed, but had to be installed behind the vehicle (not in a frame). Run over the same crop with the reaper activated. Verified that the crop wasn't collected into the storage space the description implies it should be moved to. Performed a few additional tests and came to the conclusion that the "storage elsewhere" description is completely misleading. The crop is left on the ground even if there is a storage compartment that moves over it later (reaper in the center front with a trunk in place of the advanced reaper).

Thus, remove the text talking about storage from the reaper and replace it with something saying that you have to pick the crops up manually (and make sure not to run them over, as they'll get crushed and destroyed).

The auto reaper is the tool that should be used for automatic collection, and the descriptions should match that. I haven't tested if the auto reaper is capable of filling storage elsewhere in the vehicle once its own compartment gets full.

[x-qq]

I assume by "auto reaper" you mean an advanced reaper? Because both reaper and advanced reaper have "automatic reaper" in their description text.

[PatrikLundell]

Correct. I meant the advanced reaper. As far as I can tell the only "auto" in the regular reaper is the ability to lower and raise it. That makes real world sense, as that's how reapers generally work, with collection being a more advanced evolution of the original implementation.

[natsirt721]

I'd hesitate to call this a bug. The conditional if( vp.has_feature( "CARGO" ) ) { is working as intended - the reaper vehicle part does not have storage space, so the crops do not get picked up. As was mentioned, the advanced reaper does have builtin storage space, and the crops do get picked up. The use of the word 'prior' in the part description is with regard to the mechanized farming process - it would just say 'and pick them up' if it was capable of doing that.

The "storage elsewhere" line on the vehicle part description is vague at best, suggesting that the reaper will put the harvested plants in another space if one is available. Its probably trying to tell you that you need some other vehicle part(s) to pick up and store the crops. I'd probably just remove that line entirely.

[PatrikLundell]

Well, tested the advanced reaper as well (again spawning an auto tractor), and it doesn't really work either...

Initially when moving over a row of corn it randomly leaves the harvested tile empty or with a pile of corn cobs on the ground (but no seeds). After a short while (with 20L of storage left out of the pitiful 87L original: a capacity of less than a single line of corn on a two tile high field), it leaves both cobs and seeds on all harvested tiles, and nothing gets loaded into the two useless tiles of storage in the vehicle. It can also be noted that the advanced reaper annihilates the withered plants that are produced when harvesting manually.

Spawned a reaper tractor and verified that it also eliminates the withered plants (using ordinary reapers). That's good if you hate the huge amount of time it takes to move these buggers one by one, but bad if you actually had a use for them. Real world modern equipment seems to gather straw (left behind by other cereals) into bales of hay dropped behind the harvesters on the fields, with "newer" ones (i.e. the last 20 or more years) covering the bales in plastic in the process. Note that I've got no farming experience: it's based on observing what seems to happen on fields during commute over the years. I also don't live in the US and haven't witnessed corn harvesting.

These tests were performed on an experimental that's a couple of weeks old, but I doubt things have changed in the mean time.

[natsirt721]

That is odd. I think I see what's happening though.

First, the lack of withered plants. The call to iexamine::get_harvest_items has the byproducts argument set to false, so that is 'working as intended'. Not sure if that's realistic or not; I would probably expect the byproducts to be left on the ground but I'm no expert.

Second, the not-picking-things-up sometimes. There's a limit to the size of an item that it will pick up, hardcoded to 1/20 of the part's 'size'. I'm not sure if this size value is the total volume of storage on the part or if its just the remainder, I assume the former? Either way, given the cargo size of the reaper (87.5L) that value is never larger than 4.375L. I think what's happening is that it is treating the entire stack of corn on the tile as a single item, which is occasionally exceeding this value. Given that a single corn cob is 0.75L, this means stacks of six or more will not be able to be picked up. Meanwhile, all the seeds will be because they have negligible volume, at least until the harvester fills up.

I wasn't able to duplicate it not picking anything up at all with space remaining - it just leaves more and more on the ground as the stack size of the corn produced becomes too large vs. the remaining available volume. But eventually it will harvest a stack with only one cob in it and pick that up.

The fix is to divide the stack volume by the charges of product (and seed), and perform the comparison one charge at a time, adding each charge to the part individually. Personally, I'd refactor the code a bit to just spawn the items right into the cargo space if possible instead of dropping them and picking them back up again. This should be pretty trivial to implement; I don't have a dev environment set up or I'd try it myself.

Finally, there is nothing in the code that suggests that it will put the harvested products in OTHER cargo spaces on the vehicle. That would best be fulfilled as a separate feature request I think.

[PatrikLundell]

Based on memory, when harvesters pour the harvested cereals through a pipe onto a vehicle running along it, the straw is left in strings on the ground, so just discarding the straw/withered plant is probably not correct: removing them without a trace would require some kind of incinerator technology which I don't think exists.

Your analysis of why cobs sometimes are left behind is convincing: it matches what I've seen (I haven't checked the size of the stacks left behind, but I would assume it matches).

I think the logic should add the full stack if possible and then calculate the number of items it can pick up otherwise, rather than iterate over the items. I'd be careful with skipping the item dropping step as item handling in the game seems weird. Some operations seem to rely on the items existing somewhere in the world or it can blow up when just given an item that doesn't correspond to one that exists in the environment (I tried to fix faction camp planting but gave up as I couldn't get stacks to split properly because the split off stack wasn't found in the environment down the chain, and dropping it could cause it to merge back with the original stack and all of the stack was moved). It may be that the operations involved doesn't use the same messy (and buggy) process companion missions do, however.