Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should not prefer inaccessible recipes: Treat as disabled #258

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DaleStan
Copy link
Collaborator

@DaleStan DaleStan commented Sep 4, 2024

I have two possible implementations of this fix, but neither is perfect. This one does not mess with the cost calculations, and does exactly what it is intended to do, but those intentions sometimes have unexpected consequences. The behavior is that inaccessible recipes are initially treated as disabled, but they are enabled if they are the only production or consumption for a given item/fluid.

In the attached project, everything works as expected, until you add a link for Used comb. If you add that link, Yafc will notice the inaccessible Residual oil recipe is the only source for Used comb, and enable the recipe. It then returns to the previous inaccurate cost estimation, and picks the Residual oil recipe instead of Bitumen.
seaweed-turd-test.zip

@DaleStan DaleStan requested a review from Dorus September 4, 2024 02:14
@@ -74,19 +74,21 @@ private void Setup(List<RecipeRow> allRecipes, List<ProductionLink> allLinks) {
}
}

private static void ClearDisabledRecipeContents(RecipeRow recipe) {
private static void ClearRecipeContents(RecipeRow recipe, bool recurse) {
Copy link
Collaborator

@Dorus Dorus Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If i read this right, calling this method with (recipe, false) is short hand for

            recipe.recipesPerSecond = 0;
            recipe.parameters = RecipeParameters.Empty;

?
if so, i would just extract a method for these 2 lines, and replace all calls with ClearRecipeContents(recipe, false); with that new method. Removing recurse boolean from this methods parameters. I dont think i saw any place in the code where this method is called with a variable instead of a constant.

@Dorus
Copy link
Collaborator

Dorus commented Sep 4, 2024

Running this side-by-side with latest master, one of my production sheets is no longer computing:

Left is this branch, right is the latest master.
afbeelding

inR0c+YKKMrPSk0uCUhMT+Uy0LPQM9Qz4OLiAgAAAP//7VlRb9owEP4ryM8EkY5ujKet1SpVWqVq6ss09cGxL8HCsVPHpo0Q/32XODAI6dZ03aZm8GTO953Pn++Os1kRppUFZW+KDMiMfEXV0ZXmIEcI4I5ZodUNjSSQIUmc4KgThqdxNGERi8PxBMJ3UzqN2PTtm+n7KT2Z0AlqCrSKmjfA5kpLnRQjau4oxxlF03Kdj+ZuoHRgnSmFtQ9ktiLwkFHFAdexxsGQSKEWOZl9W5FEa44jcmkh3ZqjqXYlMMShTLQRdp6S2Xg9bNMPUpok2u7Axr+AQZrZImA6jTqA/FqQJJ0xdw5AdUDNtYLiWd5VyCCixoB8GvhC4unvojvT2NFbv+A9fXhUPTxYyOXAux5XLKTsjtoecqAg/1lI3Q6JASYy8FHsx2jlSzUYMcxHHnhrIZrBNBC2wPlP1WBDuFhCkC7GpUbs8Mhm5FrfgxmBxJw2gpUYnBIPwM+ckFyoJK98qUQXFSSmModacqkSA1xUSaeclLW4zvmNLEJTdseel4Iq68E2Qy1NqpVShMpylysSAa0KgNeXAgkq9+41milcLXxek1fS5dGfPep2PSS5ixKjXbYxuKRGUGVzP33I6f7Z5EdeX5RXX0TbWV0a6hZ5kFHONVv0LWL97v5wyLbRaljfqPwbyX9yZPIFmKx6kmMJfaHoLJuNoLUFa+a8n8Bqy+ZCvZ4ff6/VlRbPRd2CNZlgKDAidknfWajiYuD70cG2Wx70pC18bmRYKkzQuEG01nZGc4sOBU4J23NWjnTs0eEvic14mGdxfyNg99540o8C8c9bxx+vPcf6svPudRBazuqUWgy/mDKrTfFqIqyZarvPjbUPj7w34o8QziR5wN3vvPY0zTSphSXNtKFI6muksyXLbhvq6ycV872nwv+qrO+1gMfuLxLWpXj97H0lelp0bJ+3Ay1kgLMFK8tJCzMuSwzlYPpDyPb/AAvmQ3hK9mvLCl2R8gpvyga/1U6X4XGB4mtaRJQt6q1hahn8h6vqKGqP99oO/61NI78Gs9kYmU2HRC/BGMHh3NAY3TrzWujNGj/fAQ==

Not 100% sure if this is intended or not, just want to put it out here.

@Dorus
Copy link
Collaborator

Dorus commented Sep 4, 2024

And another scenario. This is now unsolvable. (left is this branch, right is current master.) The weird part is that current master also cannot solve it when i remove the arquad non-turd egg recipe, so the not-solving part might be unrelated? At least it also doesn't give an error for not being able to solve it.

afbeelding

inR0c+YKKMrPSk0uCUhMT+Uy0LPQM9Qz4OLiAgAAAP//7VZRa9swEP4rRc+2cRp3y/LYskBhhWz0ZYw+nKWzIyJLriylCSH/fWfL6bLijYZ2jIbmSb77vpPu0ydFW8aNdqjd7aZGNmXfCZrcGIEqIYLw3EmjbyFXyCJWeikIk01gJMbppwlenGdpDpBmH4uMfxiPs4ssFZyQkqoS8tphlYC9BxHfe0RNGQ1VOw8Fz76eaRM7bwWF+1Ww6ZbhugYtkGZy1mPElNTLhk1/bFlpjGiGy0JlfEtPk3REX6o0VrpFRYFdNEjEsow1Nu6Q+yxiBWVpjqfRfMdyjsBjVbtNzE2VP480U7SVyQOs/wK/i5hFLmsM4ocxcb91g0P94xGVod2TbkP5z92gzy/kCuNqmbaIwqOi/Nw8oE1QkRmt5C2HUnKN4tJLJaQum24tXWjWUQpQDfaRa11aFLLzivZK9eHerPtYTqXcQb0QRd0a+dFYDspupoqoqu1yy3KEzrkBryT5o+09IJ5uTjfxVS9eK1dgfwmsu13EGp+X1vh6X3AFVoJ2TUj/SdO9NZt3XV9V13Byh1VdWfDLJq5BCMOXp+bY0N0/tuyI7h8lBrS1/M3oGVAv0OBtt//fL0BOrxMRBz3f/1ReR9Nfb4P4fMCeHBpHHom9lu7Ez2l47zwVYFEXJ9IyueXgKBdSqRup0dJXv0jwzswoPIdNDnzZt6IUWnrydwe8X+Fvt0D4GkI0c7T7Rth0EjGzQmulwCsLhaNcQNFqdvT7CQ==

@Dorus
Copy link
Collaborator

Dorus commented Sep 4, 2024

Some general thoughts:
Recipes selected in a production sheet should always be considered active. Only when not present, the YaFC cost calc should exclude them. I think currently cost calc is always assuming all milestones to be unlocked, so i'm not even sure what i'm asking here. Possibly there should be a cost calc for unlocked recipes and one for all recipes. Or even better, one for recipes that can be unlocked with milestones, but not those that cannot (so no turd-recipes, but do include science recipes).

Another problem with the turd implementation is that when you enable a turd, all sub-recipes are enabled regardless of the accessibility of the replaced recipe.

project.pages.ForEach(p => p.SetToRecalculate());
Task primary = (activePage?.RunSolveJob() ?? Task.CompletedTask);
await (secondaryPage?.RunSolveJob() ?? Task.CompletedTask);
await primary;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could go with Task.WaitAll here to wait for both jobs. That way if the second one fails, you get thrown out of it instantly.

@veger
Copy link
Collaborator

veger commented Sep 4, 2024

I think currently cost calc is always assuming all milestones to be unlocked, so i'm not even sure what i'm asking here.

I think it is good to calc when assuming all milestones are unlocked, otherwise it will be very hard to make plans without unlocking milestones before you researched them in game? But without a plan you won't fill your research queue to unlock them... So it will become some chicken-egg problem. This is something we should avoid.

But inaccessible recipes vs unlocked recipes is different. In accessible recipes are never unlocked by milestones (e.g. your turd choices). So we should be able to detect this distinction between inaccessible and unlocked?

@DaleStan
Copy link
Collaborator Author

DaleStan commented Sep 4, 2024

Recipes selected in a production sheet should always be considered active. Only when not present, the YaFC cost calc should exclude them.

I'm not sure what you mean by active here. I was expecting to see one of "(in)accessible"1, "enabled/disabled"2, "(un)locked"3, or possibly "(not )automatable"4. That said, cost and accessibility calculations cannot depend on the contents of a production sheet; there's one set of cost/accessibility values for all sheets in a project.
If you mean "enabled", that's the current behavior. This would disqualify this PR, which sometimes disables rows with inaccessible recipes. (Unlike normal disabling, child rows are still enabled.) In the other PR, I updated the cost calculation so all recipes get a cost of some sort, and inaccessible recipes get a cost penalty which discourages the solver from using them, but doesn't flat-out prevent that.

This project demonstrates another difference between the two PRs.
This PR sees the seaweed-dry row that uses an accessible recipe, and ignores the row with the inaccessible seaweed-5 recipe. However, if you add a link for any of the seaweed-5 ingredients, it will switch back to seaweed-5 until you add alternate consumption recipes for all linked ingredients.
The other PR looks at both rows, applies the specified x35 penalty for seaweed-5, and decides that seaweed-5 is still a better choice than seaweed-dry. Increasing the penalty in the Preferences window to x40, or to the default of x100, will cause the other PR to make the "correct" choice, and adding unused links for the seaweed-5 ingredients will not cause it to switch back.

I think currently cost calc is always assuming all milestones to be unlocked,

Cost calculation is done both at the current milestone setting and with all milestones unlocked. You sometimes see text like "YAFC cost per item: ¥1.37 (Currently ¥5.08)". I suspect Yafc uses the current cost when solving a table and deciding which recipes to use, but I haven't confirmed that.

I haven't had a chance to look at the tables that aren't being solved yet.

Footnotes

  1. Accessibility is auto-detected by Yafc and can be overridden by the "Mark as inaccessible" and "Manually mark as accessible" links in the Dependency explorer.

  2. An enabled recipe row is one where the "enabled" checkbox, that appears when clicking on the recipe icon, is checked. This is the only one that applies to the recipe row; the rest apply to the recipe, not the row.

  3. An unlocked recipe/item/fluid/entity is an accessible object that is available to the player without creating/using/researching any locked milestones. This can be adjusted by adding, removing, locking, or unlocking milestones. Any object where the "Mark as accessible without milestones" or "Manually mark as accessible" links have been clicked is also unlocked.

  4. An automatable recipe is an accessible recipe where (1) all ingredients are automatable and (2) a non-character crafter with an automatable fuel is accessible. This calculation cannot be overridden except by changing the accessibility of the recipe, ingredients, or crafter.
    An automatable ingredient or fuel is one that is produced by an automatable recipe, including the various recipes generated by Yafc.

@Dorus
Copy link
Collaborator

Dorus commented Sep 4, 2024

Ah thanks for the write up on the different terminology. With active i meant it's eligible to be used in the current production sheet, independent of inaccessibility, locked or automatable state.

Couple points to organize my thoughts:

  1. YaFC cost should not depend on the current production sheet.
  2. If i add some recipe to a production sheet, this recipe should be eligible to be used for the current sheet.
  3. Beside YaFC cost there is also the weight of a recipe in the current production sheet. This is not explicitly shown somewhere in numbers, but still decide if and how much a recipe is used for a certain item. For example YaFC might use recipe a 90% and recipe b 10%, because it prefers a as long as it can cheaply get rid of the side products up to a certain point, or there is some loop that only allows 90% to go around and it needs b to escape some over/under production.

For (2), the big question is what cost inaccessible recipes should have, currently YaFC makes it free and prefers to use it, that's nonsense. The current Pr tries to disable those recipes unless it has to use them, but then allows them to be used freely, while the other Pr tries to just add some arbitrary cost to avoid the use of said inaccessible recipes (?).

One important query i often want to throw at YaFC: Given the recipes i'm already using, which of these 2 potential recipes should i prefer? I could compare that by making 2 sheets with each recipe, but i much more prefer to simply give YaFC both recipes on the same sheet, and see what it starts using. Hopefully the cheapest one.

@shpaass
Copy link
Owner

shpaass commented Sep 6, 2024

The weird part is that current master also cannot solve it

This particular production-chain starts to break on very small numbers. I've encountered this issue before: #80
In general, I believe that Yafc fails to solve anything that requires more than 10^-6 precision for any item on the page -- it gets rounded down to zero.

@DaleStan
Copy link
Collaborator Author

DaleStan commented Sep 6, 2024

Both tables solve fine for me on all three branches (master, apply-penalty, and treat-as-disabled). Will you attach the project file(s), so I have your project settings too, please?

It looks to me like the second table can't be solved without "arqad-egg-1" because "arqad-egg-1" consumes .001 queens per ten eggs, while "arqad-egg-1-cold" consumes .005 queens per ten eggs. The table can't make up that difference with the selected recipes.

For (2), the big question is what cost inaccessible recipes should have, currently YaFC makes it free and prefers to use it, that's nonsense. The current Pr tries to disable those recipes unless it has to use them, but then allows them to be used freely, while the other Pr tries to just add some arbitrary cost to avoid the use of said inaccessible recipes (?).

That's correct. It sounds like the other PR may have the behavior you want if you set the penalty to x1?

@Dorus
Copy link
Collaborator

Dorus commented Sep 7, 2024

Both tables solve fine for me on all three branches (master, apply-penalty, and treat-as-disabled). Will you attach the project file(s), so I have your project settings too, please?

Might be these are non-scenario's, checking back one that couldn't solve it on master gives red errors when i increased the numbers, so that's probably a low-number rounding problem. The other one i dont quite spot what i thought was wrong at that time anymore.

That's correct. It sounds like the other PR may have the behavior you want if you set the penalty to x1?

Yes that sounds very promising. Sorry i haven't taken the time yet to dive into that one fully, but if i understand it right that might indeed be very helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants