-
Notifications
You must be signed in to change notification settings - Fork 23
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
base: master
Are you sure you want to change the base?
Should not prefer inaccessible recipes: Treat as disabled #258
Conversation
…'s only source or sink.
@@ -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) { |
There was a problem hiding this comment.
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.
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.
|
Some general thoughts: 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; |
There was a problem hiding this comment.
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.
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? |
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. This project demonstrates another difference between the two PRs.
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
|
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:
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. |
This particular production-chain starts to break on very small numbers. I've encountered this issue before: #80 |
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.
That's correct. It sounds like the other PR may have the behavior you want if you set the penalty to x1? |
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.
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. |
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