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

Increase precision of craft progress to better support long/slow crafts #29633

Merged
merged 3 commits into from
Apr 17, 2019

Conversation

esotericist
Copy link
Contributor

@esotericist esotericist commented Apr 16, 2019

Summary

Summary: Bugfixes "Increase precision of craft progress to better support long/slow crafts"

Purpose of change

With sufficiently long crafts, or with a sufficiently slow crafting rate, the math for craft progress could end up truncating to 0, preventing any progress in crafting without actually stopping the craft task, causing time to go into nowhere.

Even without it being long enough to make no progress, the truncation would artificially increase the time needed to craft versus what it should be.

Example case: a batch of 20 anvils. Expected to take 3 days and 8 hours. Zero progress no matter how much time spent on it.

Describe the solution

Increased the precision of the item_counter used by the crafting activity by three digits. We should be good to go for crafting job durations measuring in months to years now.

Describe alternatives you've considered

It was a simple precision error, what else was there to do?

Additional context

Shoutout to @OrenAudeles for helping me make the math check out. I was so past wanting to think about numbers.

@esotericist
Copy link
Contributor Author

Grr. Working on the tests.

@Rail-Runner
Copy link
Contributor

Rail-Runner commented Apr 16, 2019

Might want to make sure that it works for even longer crafts than 3 days 8 hours. According to #27132 (comment), "safe" nitrocellulose recipe would take a bit more than 16 days, in case someone does add that recipe at some point.

@esotericist
Copy link
Contributor Author

esotericist commented Apr 16, 2019

Might want to make sure that it works for even longer crafts than 3 days 8 hours. According to #27132 (comment), "safe" nitrocellulose recipe would take a bit more than 16 days, in case someone does add that recipe at some point.

I'm currently running a test on that now. I made lockpicks take 16 days to craft, and I set up a batch of 20 of them. That's a 320 day craft task. I gave myself the appropriate debug items to not care about food, sleep, or weather, set up a floodlight on a vehicle powered by a minireactor (with a bunch of storage batteries)...

Then I set 'turn' to 0, and started the craft.

A little over a day in I checked my progress using the debugger, and the math came out to 0.33% of the way through, which is approximately 1.056 days of progress.

at 00:00 of day 6, I paused it again to check the progress.
item_counter: 144043
With 10000000 as the target, that means it is 1.44043% complete. That's... approximately 4.61 days of progress for 5 days (straight) of effort. This reveals the bounds of the precision here.

The actual progress being made each step is 100 (which is the number of moves in a turn when there's no other modifiers around), with an approximate craft time (in moves) of 460,800,000.

100 / 460,800,000 * 10,000,000 = 2.2

Rounding to nearest means a progress of 2: or 90.9% of the actual progress for the time spent.
so 0.909 * 5 = 4.545. Pretty close to the measurement taken via IDE.

If this holds, it means a 320 day craft job will take somewhere around 350 days to complete. (again, that's actual time spent crafting)

Still not "ideal", but I don't know how much the team cares about precision on those timescales. We have the option of bumping the precision up again (now that I know why the tests failed), but I honestly don't know that it matters.

Shorter crafting jobs will by nature of being smaller numbers will be more accurate, so this is very much an edge case.

@esotericist
Copy link
Contributor Author

btw, I forgot to comment on the tests, but they were failing due to the binary precision issues involving representation of 0.1 in floating point math. I'm pretty certain what was happening with the test was that it was coming out to 99999.999999_ internally instead of the expected 100000.0, which is normally not an issue, but an implicit cast to int truncates.

Adding a round() should guarantee this isn't a problem in the future.

@mlangsdorf mlangsdorf added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Crafting / Construction / Recipes Includes: Uncrafting / Disassembling labels Apr 16, 2019
Copy link
Contributor

@ifreund ifreund left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. I should probably refactor how this calculation is done at some point and store number of moves instead of percent progress at some point, but this is way better than what we had.

@ZhilkinSerg ZhilkinSerg self-assigned this Apr 17, 2019
@ZhilkinSerg ZhilkinSerg merged commit 540e41f into CleverRaven:master Apr 17, 2019
@ZhilkinSerg ZhilkinSerg removed their assignment Apr 17, 2019
@esotericist esotericist deleted the craft-progress-fix branch August 3, 2019 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Crafting / Construction / Recipes Includes: Uncrafting / Disassembling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants