Overhaul faction mission travel.

[sparr]

Summary

Balance "Rework faction camp mission travel calculations"

Purpose of change

Faction mission travel is broken in many ways. Distances are wrong. Travel times don't make sense. Food consumption isn't consistent. There are numerous irritating prompts and back-to-the-start interface quirks.

Describe the solution

Here are the functional changes in this PR.

• Game mechanical changes
  • Missions use pathfinding instead of straight lines for NPC travel.
    • NPC paths across water are now forbidden, and the swim skill checks are commented out until that's resolved.
  • Food cost calculation functions for missions take an extra optional "travel time" parameter and use MODERATE_EXERCISE for that time, instead of assuming that the time spent traveling uses the same exertion level specified for the work part of the mission.
  • Exertion level and travel time are remembered for non-fixed-time missions. Extra time is assumed to be work time, not travel time. Extra time uses the work time exertion level, previously this was a constant minimal exertion level.
  • companion_travel_time_calc no longer includes work time or any handling of "haulage". NPCs are just going to make multiple round trips to carry extra stuff. There are a few edge cases where hauling would make more sense, like small numbers of large items (maybe logs, definitely fridges), but I'm not planning to tackle that right now.
  • Wood cutting haul estimation uses weight and volume of logs and sticks, not previous much more rough approximation.
  • Hide sites are now allowed on all the terrain where logging can happen instead of just a few forest types, as well as swamp and field as before.
  • Fortifications are now allowed in all terrain where logging can happen instead of just a few forest types, as well as field as before.
  • Digging trenches no longer requires traveling back to base after each tile. Spiking trenches still does.
  • Combat Patrol and Scouting Mission now make a single trip instead of a round trip if the path you select is a loop back to the starting point.
  • Combat/Scouting mission paths can now be set to the full range, not ending early when reaching range-3.
• Content changes
  • Updated some message wording and mission descriptions.
• Code structure changes
  • Most of the companion pathfinding functions now pass around a pf::simple_path<tripoint_abs_omt>, which contains the total traveled distance and the total pathfinder cost for the path in addition to the vector<tripoint_abs_omt> that was passed previously.
  • Some path selection and calculation overloads were eliminated when they became unused after all call sites were updated to use the pathfinder functionality.
  • Pathfinder costs have been adjusted mostly proportionally so that they measure seconds of travel (assuming auto drive speed for cars, planes, and boats), with weights and times kept mostly in line with the previous costs and npc mission travel times.
  • Various new and updated comments on and in the relevant and related functions.
• Interface changes
  • New om_path_mark() adds/removes map notes along a pathfinder path, similar to the old om_line_mark().
  • om_companion_path now allows undoing selections. You have to press escape multiple times to undo then exit to cancel.
• Bug fixes
  • Fixed some bugs in the calculation of pathfinder cost around the start and end of a path.

Describe alternatives you've considered

I went through multiple variations of half of the changes here. I did not keep a list of what they were and why I switched away from them. I could probably work to remember it for any particular change if asked.

Testing

I've been playing with this since before I created the PR. I expect a few automated tests are in order as well, but I'm not sure where to start on that.

Additional context

This PR is shrinking as smaller PRs get split off from it, 8 so far. I've run out of low hanging fruit. I'll make another round of merge-conflict-heavy PRs if this is still too busy.

[PatrikLundell]

Please list the various changes in the "Describe the solution" section rather than providing something that says that a lot of stuff has been changed. You don't need to specify each individual change, but make each type its own point (there are a number of cases where strings have been added to queries, and they could all be a covered by a single point).

If you're separating work duration from travel duration (which is a good thing), you ought to rename the "duration" parameter in various operations to "work_duration" or "work_time" to make it clear (I assume it's no longer total duration, so travel time would have to be deducted from it).

I don't know if it matters, but it looks like you've separated work time from travel time in the food store reduction logic, but not in the companion time/exertion logic. If the companions end up being fed the same from the ether as being consumed by them it doesn't matter that the separation isn't repeated there, though.

I would say "from %d to %d" in the changed logging query, as "between" is ambiguous whether it is inclusive or exclusive.

Wait a minute: In faction_camp.cpp you feed "time_to_food" with total time, which includes travel time. If you're going to separate out travel time you really should only feed work time into the first parameter.

I think it makes more sense to warn about forest roads changing into field roads without any visual cue (which you removed) than mentioning that forest might turn into fields (which you added, and which is visible).

Why is the tree chopping call to "start_mission" different from the one for clear cutting (the latter uses an additional "travel_time" parameter on top of the total_time)?

Corresponding comments for the hide site.

I don't understand what "lesser direction" is in the comment "We get lesser direction for free". The "greater trips" in the original comment only made some kind of sense through the mentioning of the return trips, which I think means the "greater trips" are the ones where equipment is carried.

And the same for hide site hauling.

Does the changed permitted fortification location list really include forest trails and thick forest, or it it otherwise intended to exclude them?

You don't need to check for an illegal "start" value in "start_fortifications", since you've already bailed out if that was the case (and is bailing out the correct action in the first place?).

Fortification call to start_mission is the one without the travel time.

recipe_batch_max: Why have you ditched work_days conversion? That would result in the use of less food?

Replacing "base" with "previous point" is probably wrong. There doesn't seem to be any previous point, but rather a start point or origin.

craft_description: workdays ditched again...

OK, it seems the parameters are actually intended to be total time and travel time after all. It would still be helpful to name all the total_duration parameters accordingly, rather than retaining the generic duration one in the various operations that have changed parameters.

recipe_group.cpp get_recipes_by_id: It's legal in C, but it's still misleading to check "limit" when you really should say "limit == 0". Don't know if the style orders demands that, though.

savegame_json.cpp: You're not setting a default value for comp_miss_travel_time. Is time_duration one of these ugly things that has a hidden default value?

simple_pathfinding.cpp: A bit odd to extend 3D cardinal directions with horizontal diagonals, but not diagonals with Z components. You'd eventually need these to deal with ramps.
Haven't looked at the logic of the operations, or tried to look for where all the magic numbers come from.

[sparr]

I'll be back soon with a better PR description, PR comments, and more replies. But to get started...

In faction_camp.cpp you feed "time_to_food" with total time, which includes travel time. If you're going to separate out travel time you really should only feed work time into the first parameter.

The original version of that function takes total time (often including travel time), then reverse engineers the "work days" math to figure out how much of the time was spent working vs idle vs sleeping. It took me a bit to wrap my head around that. Given that logic was already there, adding the travel time as an extra exceptional option (which isn't always used) seemed like the least intrusive fix.

[PatrikLundell]

I think it's OK to retain the total time parameter, but it should be renamed to avoid confusion as to what it represents now when the travel time parameter has been introduced. That's mentioned further down in my comment, but I guess going through that isn't much more fun than going through the PR, so it won't be surprising if it's split up.

[sparr]

Please list the various changes in the "Describe the solution" section rather than providing something that says that a lot of stuff has been changed. You don't need to specify each individual change, but make each type its own point (there are a number of cases where strings have been added to queries, and they could all be a covered by a single point).

Will do! My plan was to use the draft to add PR comments, then use the PR comments to re-draft the summary before submitting it for review.

If you're separating work duration from travel duration (which is a good thing), you ought to rename the "duration" parameter in various operations to "work_duration" or "work_time" to make it clear (I assume it's no longer total duration, so travel time would have to be deducted from it).

I think it's OK to retain the total time parameter, but it should be renamed to avoid confusion as to what it represents now when the travel time parameter has been introduced.

OK, it seems the parameters are actually intended to be total time and travel time after all. It would still be helpful to name all the total_duration parameters accordingly, rather than retaining the generic duration one in the various operations that have changed parameters.

Good call. I already made a few passes of making the naming and variable handling more consistent, and I'll aim for this in a next pass. My eventual goal would be deduplicate the logic in question, so there's just one travel_time = ...; work_time = ...; total_time = travel_time + work_time; ...food(total_time, ..., travel_time)l ... that they can all use, and all the called functions would use the same argument naming scheme.

I don't know if it matters, but it looks like you've separated work time from travel time in the food store reduction logic, but not in the companion time/exertion logic. If the companions end up being fed the same from the ether as being consumed by them it doesn't matter that the separation isn't repeated there, though.

I tried to hit all the relevant spots. Can you point out the one that I overlooked?

I would say "from %d to %d" in the changed logging query, as "between" is ambiguous whether it is inclusive or exclusive.

I took the "between" language from the previous code. Happy to change it; I agree "from" is more clear. Also, to be clear, the min and max range are also displayed visually on the map with notes.

Wait a minute: In faction_camp.cpp you feed "time_to_food" with total time, which includes travel time. If you're going to separate out travel time you really should only feed work time into the first parameter.

A further reply to this in addition to the above... I can't "only feed work time" because the first parameter already isn't just work or work+travel time time. It also includes idle and sleep time. I don't like this solution, and I tried to fix it, but when I saw how many other calculations mirror the same logic I decided against it.

I think it makes more sense to warn about forest roads changing into field roads without any visual cue (which you removed) than mentioning that forest might turn into fields (which you added, and which is visible).

I think I misunderstood what it was saying before. To be clear, they don't actually change terrain type, it's just saying they will be visually indistinguishable. One of my goals in this PR is to reduce the number of modal popup interruptions involved in all these interfaces. Maybe I can put the road-type-discernment advice in the mission description instead?

Why is the tree chopping call to "start_mission" different from the one for clear cutting (the latter uses an additional "travel_time" parameter on top of the total_time)?

Oversight on my part. Of all the things I want to deduplicate, those two are near the top of the list. They have so much in common, and it's so easy to miss a single detail changed between them. Adding the travel time parameter to start_mission will cause the food consumption for the mission to honor the distinction between work exertion and travel exertion.

Corresponding comments for the hide site.

I don't understand what "lesser direction" is in the comment "We get lesser direction for free". The "greater trips" in the original comment only made some kind of sense through the mentioning of the return trips, which I think means the "greater trips" are the ones where equipment is carried.

The trip calculation assumes you are making round trips with cargo one way and empty the other way. The mission sends items in both directions. So if the trip calculation says you need 5 round trips to take items to the hide site, and 3 round trips to bring items back from the hide side, you actually just need 5 round trips. I'm open to a more helpful concise comment to put there. I thought mine was an improvement, but maybe not.

And the same for hide site hauling.

Does the changed permitted fortification location list really include forest trails and thick forest, or it it otherwise intended to exclude them?

I did intentionally expand the list. I think the shorter list predates the introduction of [some of] the new terrain types.

You don't need to check for an illegal "start" value in "start_fortifications", since you've already bailed out if that was the case (and is bailing out the correct action in the first place?).

I don't think this is accurate. Could you comment within the PR changes where you see this?

Fortification call to start_mission is the one without the travel time.

Thanks. That's also one without the more recent "update the time based on the NPC chosen" code that a bunch of the others have.

recipe_batch_max: Why have you ditched work_days conversion? That would result in the use of less food?

craft_description: workdays ditched again...

You are correct. This is an error. I might not have fully grasped the work days conversions back and forth at that point.

Replacing "base" with "previous point" is probably wrong. There doesn't seem to be any previous point, but rather a start point or origin.

I think I need to remove that. Neither wording works everywhere. I reworded it around the mutli-point scout/combat missions, but now it doesn't make sense for the single-point missions.

recipe_group.cpp get_recipes_by_id: It's legal in C, but it's still misleading to check "limit" when you really should say "limit == 0". Don't know if the style orders demands that, though.

I, too, wasn't sure about style here. Happy to change it, though.

savegame_json.cpp: You're not setting a default value for comp_miss_travel_time. Is time_duration one of these ugly things that has a hidden default value?

That's an excellent question. I've seen some used uninitialized, which made me guess they default to 0, but I haven't confirmed. And there are far more places they do get initialized to 0, so I should probably just follow that pattern.

simple_pathfinding.cpp: A bit odd to extend 3D cardinal directions with horizontal diagonals, but not diagonals with Z components. You'd eventually need these to deal with ramps. Haven't looked at the logic of the operations, or tried to look for where all the magic numbers come from.

Ramps seem very exceptional, while diagonal horizontal travel is very common. I'm comfortable with the pathfinding time/distance/cost being off by a few seconds/meters when a ramp is traversed, at least for now.

[PatrikLundell]

I don't know if it matters, but it looks like you've separated work time from travel time in the food store reduction logic, but not in the companion time/exertion logic. If the companions end up being fed the same from the ether as being consumed by them it doesn't matter that the separation isn't repeated there, though.

I tried to hit all the relevant spots. Can you point out the one that I overlooked?

Having trouble finding the places (calling the total time "work_time" might have thrown me off), but:

• start_mission should probably use the time_to_food version that includes travel_time (twice in the beginning).

• feed_workers should probably be called twice: once for duration - travel_time, and once for travel_time, with the latter using the appropriate exertion_level (or is there a version with an extra parameter?: having trouble navigating my text copy of the code).

• start_cut_logs should use the time_to_food version taking travel_time as a parameter in its Trip Estimate query.

• start_setup_hide_side: ditto

• start_relay_hide_site: ditto

• start_fortifications: need_food should use 3 parameter call.
  Conclusion here: double check time_to_food usages to ensure the correct number of parameters is used.

  I think it makes more sense to warn about forest roads changing into field roads without any visual cue (which you removed) than mentioning that forest might turn into fields (which you added, and which is visible).

I think I misunderstood what it was saying before. To be clear, they don't actually change terrain type, it's just saying they will be visually indistinguishable. One of my goals in this PR is to reduce the number of modal popup interruptions involved in all these interfaces. Maybe I can put the road-type-discernment advice in the mission description instead?

• The logging/clear cutting operations MAY change terrain type if 95% of the trees are gone when the mission has been set in motion. A dirt road type change isn't visible in many (all?) tile sets, though, while forest->field probably is in all.
• The round trip comment: I think the comment need to explicitly state that it calculates round trips for hauling in both directions and selects the one that's greater.
• start in start_fortifications: 6 lines up:

    if( start == overmap::invalid_tripoint ) {
        return;
    }

so it has to be valid or you'd have bailed out already.

• Checking int values as boolean: The tests will demand changes if you don't adhere exactly to their rules. However, I don't think checking a non boolean value against 0 will be yelled at, in a rare display of flexibility.
• If path finding is capable of using ramps I don't have an issue with the cost being slightly off, but if it tries to separate horizontal travel and vertical in separate moves it won't work.

[sparr]

Skipping some replies that I think are obsolete since I was pushing a commit as you were typing, to address maybe 3/4 of the previous concerns.

* feed_workers should probably be called twice: once for duration - travel_time, and once for travel_time, with the latter using the appropriate exertion_level (or is there a version with an extra parameter?: having trouble navigating my text copy of the code).

The call to feed_workers in start_mission uses the camp_food_supply overload that takes travel_time and passes it through to time_to_food, so the two exertion levels are handled correctly.

Conclusion here: double check time_to_food usages to ensure the correct number of parameters is used.

Done!

• The logging/clear cutting operations MAY change terrain type if 95% of the trees are gone when the mission has been set in motion. A dirt road type change isn't visible in many (all?) tile sets, though, while forest->field probably is in all.

OK. I'm not sure how best to convey this concern. I still don't like the old wording. Do you have a suggestion, even just to go back to the old wording?

• The round trip comment: I think the comment need to explicitly state that it calculates round trips for hauling in both directions and selects the one that's greater.

Done!

• start in start_fortifications: 6 lines up:

    if( start == overmap::invalid_tripoint ) {
        return;
    }

so it has to be valid or you'd have bailed out already.

This "valid" check is determining whether the player pressed escape at the map tile selection interface. It doesn't assert anything about a tile that was chosen. The chosen tiles still need to be checked for type/etc. The check I modified already checks one of the two chosen tiles, and all the in between tiles, it just missed the other chosen tile due to an under-documented quirk of line_to. (which I'm about to rectify)

• If path finding is capable of using ramps I don't have an issue with the cost being slightly off, but if it tries to separate horizontal travel and vertical in separate moves it won't work.

I'll test this. I don't know if it worked before. I don't think I changed it.

[PatrikLundell]

Wording: I don't really have any good alternatives. What was there was my best attempt at it... (I'm not saying that can't be improved, just that I don't have any good ideas, and I'm not adverse to it being improved).

"Valid" check: I think I'm doing a poor job communicating what I mean...:

     if( start == overmap::invalid_tripoint ) {
        return;
    }
    tripoint_abs_omt stop = om_target_tile( omt_pos, 2, 90, terrains_field_swamp_forest,
                                            ot_match_type::type,
                                            true, start, _( "Select an end point from %d to %d tiles away." ) );
    if( start != overmap::invalid_tripoint &&
        stop != overmap::invalid_tripoint ) {

could be reduced to:

     if( start == overmap::invalid_tripoint ) {
        return;
    }
    tripoint_abs_omt stop = om_target_tile( omt_pos, 2, 90, terrains_field_swamp_forest,
                                            ot_match_type::type,
                                            true, start, _( "Select an end point from %d to %d tiles away." ) );
    if( stop != overmap::invalid_tripoint ) {

as the setting of "stop" won't change the value of the already tested "start". I didn't mean the whole check was redundant, but I think I failed to convey that.

[sparr]

Ahh, I thought you were talking about the terrain type check that I added farther down to account for line_to not returning its start position.

You're right about the redundancy where you pointed it out. I added the earlier check and forgot to remove the later check.

[sparr]

Updated the PR description with a list of changes.

[sparr]

I'd like to add some tests, but the current lack of tests in this area gives me pause. Any suggestions where I should start?

[sparr]

I'm moving this back to Draft while I break pieces out. The first four smaller PRs are linked above. When they get merged I'll rebase this one on them so I have a better base for the next round. I'll un-Draft this again when I think I've reached a point where everything left is too integrated to reasonably extract.

[sparr]

I've extracted all the low hanging fruit. Breaking things out is becoming increasingly conflict-prone, so I'm marking this for review again. I have half a dozen more improvements I want to make here, but I know adding them to this PR would make it even harder to get reviewed and merged.