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

reduce turn length to 1 second #30253

Merged
merged 19 commits into from
Jun 8, 2019
Merged

Conversation

ymber
Copy link
Member

@ymber ymber commented May 5, 2019

Summary

SUMMARY: Balance "Reduce turn time to 1 second"

Purpose of change

closes #30018

Describe the solution

Tick to time conversions were already encapsulated, constants just needed changing.

  • change constants in conversions
  • update json values and documentation
  • fix construction times
  • fix times in player_activity
  • check behavior of max_duration

@Vasyan2006
Copy link
Contributor

Crossbow reloading time is calculated in moves instead of second and must be updated for new turn duration. Same for many other actions.

@mlangsdorf
Copy link
Contributor

consume_fuel( idle_rate, 6 * to_turns<int>( 1_turns ), true );
needs to be changed to

-        consume_fuel( idle_rate, 6 * to_turns<int>( 1_turns ), true );
+        consume_fuel( idle_rate, to_turns<int>( 1_turns ), true );

There are other places in the vehicle code that do a similar thing, and there are other locations in the code that need to be audited for similar uses of to_turns.

@mlangsdorf
Copy link
Contributor

You'll need this to adjust the vehicle efficiency test for the change in idle consumption.

diff --git a/tests/vehicle_efficiency.cpp b/tests/vehicle_efficiency.cpp
index f71e151707..f38b9e9664 100644
--- a/tests/vehicle_efficiency.cpp
+++ b/tests/vehicle_efficiency.cpp
@@ -430,22 +430,22 @@ TEST_CASE( "vehicle_make_efficiency_case", "[.]" )
 // Fix test for electric vehicles
 TEST_CASE( "vehicle_efficiency", "[vehicle] [engine]" )
 {
-    test_vehicle( "beetle", 767373, 235000, 195500, 70760, 54070 );
-    test_vehicle( "car", 1072322, 363000, 245700, 44380, 26120 );
-    test_vehicle( "car_sports", 1098408, 208900, 168600, 35020, 20180 );
+    test_vehicle( "beetle", 767373, 294400, 235600, 77390, 58340 );
+    test_vehicle( "car", 1072322, 530700, 312700, 47890, 28180 );
+    test_vehicle( "car_sports", 1098408, 456500, 297800, 39400, 21740 );
     test_vehicle( "electric_car", 1070791, 212500, 121700, 17430, 9118 );
-    test_vehicle( "suv", 1271990, 696200, 405800, 68340, 32700 );
-    test_vehicle( "motorcycle", 162785, 64180, 54160, 41250, 34600 );
-    test_vehicle( "quad_bike", 264745, 63520, 63520, 31390, 31390 );
-    test_vehicle( "scooter", 62287, 140400, 138000, 108800, 107000 );
-    test_vehicle( "superbike", 241785, 58540, 40000, 30480, 19890 );
-    test_vehicle( "ambulance", 1783889, 331200, 281700, 61580, 46220 );
-    test_vehicle( "fire_engine", 2563241, 1018000, 863500, 236700, 208700 );
-    test_vehicle( "fire_truck", 6259233, 253000, 149700, 19010, 4523 );
-    test_vehicle( "truck_swat", 5939334, 409300, 248500, 27990, 7189 );
-    test_vehicle( "tractor_plow", 703658, 354200, 354200, 106700, 106700 );
-    test_vehicle( "apc", 5753619, 957100, 824200, 123600, 85540 );
-    test_vehicle( "humvee", 5475145, 465800, 278700, 24650, 8824 );
-    test_vehicle( "road_roller", 8779702, 315600, 295100, 21910, 6666 );
+    test_vehicle( "suv", 1271990, 998800, 492900, 71890, 34300 );
+    test_vehicle( "motorcycle", 162785, 76870, 62950, 47530, 38890 );
+    test_vehicle( "quad_bike", 264745, 75950, 75950, 35080, 35080 );
+    test_vehicle( "scooter", 62287, 266900, 258300, 195200, 189700 );
+    test_vehicle( "superbike", 241785, 72120, 46710, 34580, 21720 );
+    test_vehicle( "ambulance", 1783889, 433100, 351300, 65530, 48600 );
+    test_vehicle( "fire_engine", 2563241, 1171000, 970800, 248500, 219000 );
+    test_vehicle( "fire_truck", 6259233, 308500, 200800, 19950, 4747 );
+    test_vehicle( "truck_swat", 5939334, 505800, 349500, 30060, 7719 );
+    test_vehicle( "tractor_plow", 703658, 528000, 528000, 121900, 121900 );
+    test_vehicle( "apc", 5753619, 1100000, 942800, 132800, 91850 );
+    test_vehicle( "humvee", 5475145, 607600, 342700, 25880, 9263 );
+    test_vehicle( "road_roller", 8779702, 369800, 412000, 22990, 6996 );
     test_vehicle( "golf_cart", 446230, 52800, 104200, 26830, 13890 );
 };

@Hirmuolio
Copy link
Contributor

Hirmuolio commented May 5, 2019

_turns is used for duration of various things in many places. Almost all of them need to be changed.

Probably best to change many of them them to _seconds instead of 6x_turns unless they specifically are about turns instead of just time duration.

Same for to_turns.

Over 900 search hits for them combined in the src files.

@mlangsdorf
Copy link
Contributor

I played the current version for a bit - long enough to loot about 7 houses, drive about 100 overmap tiles back to my base, unload my loot, and do some basic maintenance on my ride. It wasn't a comprehensive test, but a decent start.

I didn't notice any glaring errors or problems. The looting portion of the game plays much faster, obviously, which may make the "instant expert" problem more severe.

I need to review the vehicle braking code. Hand-waving the rate of deceleration and skidding was probably acceptable when the turn length was 6 seconds, but now that the turn length is 1 second, it shouldn't be possible to brake from 60 mph in a single second. I don't think that should hold up this PR; it's just something to consider in the future.

@ifreund ifreund added <Enhancement / Feature> New features, or enhancements on existing [C++] Changes (can be) made in C++. Previously named `Code` Mechanics: Character / Player Character / Player mechanics Game: Balance Balancing of (existing) in-game features. Game: Mechanics Change Code that changes how major features work labels May 5, 2019
@ymber
Copy link
Member Author

ymber commented May 6, 2019

As far as i can tell whoever wrote item::calc_temp in the first place multiplied the conductivity constant of air by 6 so he could use 6 second turns as a time unit then used that constant as the heat transfer coefficient in the formula for conductivity_term. It sounds plausible and changing the constant accordingly made the tests pass so that's what I'm going with. conductivity_constant now works with actual watts and not six second watts.

src/addiction.cpp Outdated Show resolved Hide resolved
@Firestorm01X2
Copy link
Contributor

It is recommended to check aiming speed in ranged.cpp.

@ymber
Copy link
Member Author

ymber commented May 7, 2019

Aim speed should be ok, it's all measured in moves.

@mlangsdorf
Copy link
Contributor

I just realized that you're missing construction recipes. I thought I built that fireplace unreasonably quickly!

@kevingranade
Copy link
Member

Since most of this PR is changing crafting recipe time fields, what do you think about having "old style" time entries remain 6 seconds, and changing everything in the main repository to be like "time": "5 sec" or similar?

@mlangsdorf
Copy link
Contributor

player_activity is also broken - I'm chopping down trees in 15 minutes.

It looks like you need to go through iuse.cpp, iuse_actor.cpp, and iexamine.cpp and audit all moves - some things are meant to take X turns and can stay how they are, but other things should take Y seconds. Everything should use to_moves()

@ymber
Copy link
Member Author

ymber commented May 9, 2019

Changing the units of time fields is an option. I was thinking of making it like how volume fields are handled with different units.

@Rivet-the-Zombie
Copy link
Member

It would be pretty nice to not have to spend twenty seconds aiming at a target in order to actually stand a chance of hitting it.

@mlangsdorf
Copy link
Contributor

Chopping wood seems to happen too quickly, and you gain too much experience from doing construction tasks.

@Mcrone617
Copy link

Hey @ymber i'm working with @DracoGriffin to try to increase public participation in the development process, specifically by posting PR testing on reddit, and having people test out big changes that need a lot of testing to get merged. Would you be interested in having your PR as a public test? All you have to do is make a downloadable build and release it on GitHub, ( I can help you with that if you don't know how) and then i'll take it from there, and make a post describing your changes, and what people should do to test them and leave feedback. Of course you're welcome to post it yourself. Because this PR is such a big balance thing, I think it could really benefit from this, even 3-4 people saying that X is fine, or Y needs some fine tuning can really help in the merging process. Let me know if you're interested, i'm active here, and on the discord. I hope to hear back from you!

@kevingranade
Copy link
Member

We can pull all the arbitrary time units to real time units changes in a seperate PR so those can land without changing behavior while the last parts are hunted down.

@ZhilkinSerg ZhilkinSerg added the PUBLIC TEST Come and try this! label May 21, 2019
@ymber
Copy link
Member Author

ymber commented May 21, 2019

@Mcrone617 if you want to do testing builds from my feature branch go for it but I don't want to handle builds and releases because I work on linux and I don't want to have to handle cross compiling for windows players. Also this PR is getting split up so I can convert and standardize units for all the time fields before handling everything else, so you might not get much mileage out of testing this in its current state.

@ZhilkinSerg ZhilkinSerg self-assigned this May 29, 2019
@ymber
Copy link
Member Author

ymber commented Jun 2, 2019

item_counter is and the item::process* functions look ok. Zombies reviving looks like it's intended to measure in turns irrespective of what that timeframe really is. item::process_tool is being updated - see #30656.

@Hirmuolio
Copy link
Contributor

Hirmuolio commented Jun 2, 2019

About the zombie reviving.

Lets assume we have a zombie corpse that has 1% chance to revive.
You stand next to the corpse for one minute.

With 6 s turns there are 10 turns in minute. There is 10% chance that the corpse revives during that minute ( 1-0.99^10 ).
With 1 s turns there are 60 turns in a minute. There is 45% chance that the corpse revives during that minute ( 1-0,99^60 ).

@kevingranade
Copy link
Member

This is so incredibly close.
@Hirmuolio is right about respawns, but I'm not terribly concerned about that, because it still doesn't rise for 6 hours, and then the rate of respawn climbs as expected as time since death approaches 48 hrs.
There's an overflow error in an item aging test, item ages wrap after a year now.
This definitely missed something in nutrition handling, vitamin handling is off by a factor of 6.

@Hirmuolio
Copy link
Contributor

Just dividing the probablitiy by 6 would probably be enough. It won't be exactly same as before but should be close enough.

@kevingranade
Copy link
Member

kevingranade commented Jun 7, 2019

Prospective fixes:

Respawn rate

drop a

if( !calendar::once_every( 6_seconds ) ) {
    return false;
}

around here

int age_in_hours = to_hours<int>( age() );

Overflow Error

I thought this was a symptom of the underlying data in time_point overflowing, but it claims it is an int (so 31 bits of precision) representing number of turns, so I don't know why it would have overflowed, maybe there's something weird happening in the test...

Vitamin discrepancy

The nutrition tick is normalized for time since last tick, so it is working as expected, need to drill down into the vitamin handling, it might be doing something strange.

@kevingranade
Copy link
Member

I've been trying to trace where the apparent overflow was coming from, and discovered it's actually an underflow. The item used was necco wafers, which spoil after 180 days, and then we modify their age by spoilage time - one year, which as it turns out resolves to a very negative number, so of course it doesn't work. I'm not sure now why this ever passed, but now I suspect this is just a red herring, if we swap in an item with a spoilage time longer than a year it should just work.

@kevingranade
Copy link
Member

Aha, the vitamin problem is pretty straightforward. Vitamin change rates are specified in turns:
https://github.com/CleverRaven/Cataclysm-DDA/blob/master/data/json/vitamin.json
Since the list is short and stable, I'd support either editing the json values directly or extending the parser to handle durations with units like elsewhere.

@Fukken-Saved
Copy link

The primary effect of reducing turn duration seems to be that now the taking forever to craft items problem is six times as bad, so as of now it's unplayable unless you can move it to machine language or something so it's six times as fast.
I like having all z-levels simulated so if I wanted to do that there's probably some version I can use before this update (not the awful one where the stamina was all messed up), it would be much better if turn duration could be player adjusted
Also, realistically as far as zombie sims go nobody is going to be able to run through a house and in 30 seconds grab a hundred pounds and 50 liters of very specific manually selected items that they want, the net effect of this is that it's going to make day-to-day survival wayyy easier as you effectively have days 6 times as long

@Fukken-Saved
Copy link

Also, everyone is going to be bored of it probably before even the first Summer

@kevingranade
Copy link
Member

realistically as far as zombie sims go nobody is going to be able to run through a house and in 30 seconds grab a hundred pounds and 50 liters of very specific manually selected items that they want,

This is a first coarse adjustment, all action durations can be tweaked to make them more reasonable, but it's now much closer than it was before.

@ZhilkinSerg
Copy link
Contributor

The primary effect of reducing turn duration seems to be that now the taking forever to craft items problem is six times as bad, so as of now it's unplayable

That is not true.

@Fukken-Saved
Copy link

I forgot where this was, but I just want to point out that build 9112 seems to be the last version before the new turn durations take effect. Chances are you'll be able to adjust turn duration eventually, but I've had very good experience with the build with most of the crashes on worldgen
And it is unplayable if you have to wait 10 minutes while it's crafting a stone knife.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` <Enhancement / Feature> New features, or enhancements on existing Game: Balance Balancing of (existing) in-game features. Game: Mechanics Change Code that changes how major features work Mechanics: Character / Player Character / Player mechanics PUBLIC TEST Come and try this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reduce turn length to 1 second