pyanodon / pybugreports

Central bug-report repository for pymods
The Unlicense
5 stars 1 forks source link

Empty Barrel recipe is still broken, prod-wise. #574

Closed EigensheepLambda closed 1 day ago

EigensheepLambda commented 1 month ago

Mod source

Factorio Mod Portal

Operating system

=Windows 10

What kind of issue is this?

What is the problem?

The recipe accepts prod but the only product is catalyst-flagged (thanks to logic to globally flag them that way in recipes that take full barrels), so the prod progress bar doesn't produce extra barrels. This has come up in the past and been closed as fixed, but currently isn't. (Unless it's been fixed in the sex branch and deemed too annoying to backport, in which case, everything's probably fine so disregard this.)

Steps to reproduce

No response

Additional context

No response

Log file

No response

maryrivlet commented 1 month ago

I wasn't looking for this, but I guess it's this.

-- Skip if recipe only produces the item, not uses it as a catalyst. Fluids are probably blacklisted for other reasons.
if #recipe.results ~= 1 or recipe.results[1].type ~= 'fluid' then

The type check seems to be reversed. (I'm not aware that there are any blacklisted fluids anyway, but I don't know much about the code base...)

oorzkws commented 1 month ago

Close, but it's actually that the config.NON_PRODDABLE_ITEMS includes empty barrels. That's probably unnecessary now that we are properly flagging catalysts.

https://github.com/pyanodon/pypostprocessing/commit/f60365084c51d2595fe1d7d73b6b7ea2d606778e

maryrivlet commented 1 month ago

Sorry, I wasn't trying to call you out or anything, and I don't want to argue about this, but I don't really understand.

Obviously the whole NON_PRODDABLE_ITEMS post processing thing could be unnecessary if everything was properly flagged. I don't know whether it's still needed or not; fine. Yes, this processing can flag the empty barrel recipe result as a catalyst.

But everything in the change I linked indicates to me that it was intended to specifically work around this issue without actually removing anything from the blacklist ("Fix productivity blacklist being overly general and including recipes like empty barrels") and I don't understand any other purpose. It seems to me that it would do that except that the check is mixed up. It skips applying the blacklist if there is exactly one fluid result. It seems it intended to skip applying the blacklist if there was exactly one solid result which would seem to be the case for the empty barrel recipe. (I guess checking against "fluid" rather than "item" is good to implicitly support list-like result entries (if those are even a thing) with no type tag as in the related code.)

oorzkws commented 1 month ago

Sorry, I wasn't trying to call you out or anything, and I don't want to argue about this, but I don't really understand.

No worries, it's reasonable to wonder why things are the way they are. The check is applying the blacklist if the first item is NOT a fluid, and there's more than one result. A more sane version of the check would be written like this:

if #recipe.results == 1 or recipe.results[1].type == 'fluid' then
    goto NEXT_RECIPE
end

In reviewing this for your comment I realized the real issue: this code isn't running! I pushed the fix to main instead of Frozen and thus it never ended up on the portal. I'm going to see if the fluid check is necessary before I push the fix.