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

Auto path #74644

Closed
wants to merge 37 commits into from
Closed

Auto path #74644

wants to merge 37 commits into from

Conversation

Brambor
Copy link
Contributor

@Brambor Brambor commented Jun 17, 2024

Summary

Features "Player can record paths and automatically walk them again"

Purpose of change

Resolve #74433

Describe the solution

Listen with u.get_path_manager()->record_step to when the avatar moves in

void avatar_moves( const tripoint &old_abs_pos, avatar &u, const map &m )

Add the new_pos to std::vector<tripoint_abs_ms> recorded_path. If a cycle is detected, remove it.

Add an interface, where the player can manage their paths:
image

If they select Walk path from middle:
image

Known issues to fix / features to implement:

  • Recording path across ramp doesn't work (stairs do work).
  • When a problem occurs, it continues recording. I want to change this to fail fast.
    • Example of detectable unsolved problem: lift going more than 1 z-level.
    • image
  • The latest version crashed on me, so I need to fix at least one crash.
  • (i) hover next to Walk the path button: Walk the first path from the list the player is standing on either end.

Will not fix Depends on:

  • Migrate uilist to ImGui #74341 - this uses uilist and without its migration, the uilist shows under the Path Manager menu
  • When naming start/end the string_popup is under the window (barely visible) but it does work.
    • Will be solved by Imgui input popups #77863
    • Will not fix, unless somebody tells me how to easily do it without migrating string_input_popup to ImGui.
      • Migrating string_input_popup to ImGui needs migrating string_input_popup::*history which uses uilist so it needs its migration which is being done in Migrate uilist to ImGui #74341

Further possible improvements

  • I have a nice optimizations in my back pocket, described in the issue Record path then take it (again and again). #74433
    • Look at one point at index i, count the distance from the new point dist = abs(z1-z2) + max(abs(x1-x2), abs(y1-y2)). The new point cannot be at indexes i-dist+1, ..., i+dist-1. So skip those in-loop checking.

dist optimization results

For the optimization, I ended up doing max( tripoint_diff.abs().xyz() ) . The results are pretty amazing: 4 checks instead of 800 on this test path:

Cataclysm_.Dark.Days.Ahead.-.0.G-10291-g221e3ee59b-dirty.2024-06-24.21-04-11.mp4

Works reasonably in a spiral. I especially like how the number of checks decreases despite the number of tiles increasing when the character walks away from the spiral at the end:

Cataclysm_.Dark.Days.Ahead.-.0.G-10291-g221e3ee59b-dirty.2024-06-24.21-18-35.mp4

  • Also, add QoL optimization to cut corners: going north and then west can be replaced by going north-west. In general: iterate over recorded_path from begin() to end() with tile, if the player is next to the tile, delete everything from tile to recorded_path.end(). This is similar to loop optimization but more aggressive.
  • A checkbox to turn off cut-corner optimization.
  • New button: Take an already recorded path the player is standing at either end of. Start recording (again) to edit this path as if recorded for the first time.
    • (i) hover next to the button: Continue recording the first path the player is standing at either end of. Temporarily swaps start and end until the recording is finished.
  • New button: fix path. Something interrupted the walking (even if you did). You can snip the next tile of the path. This leaves two separate paths: from_start & till_end. Start recording until you reach any tile from till_end. Then glue them together (from_start ends at your feet, remove tiles from till_end from the snipped tile until the tile under your feet. Merge paths and that is your result.) - Not sure whether this works when the path has loops.
    • "Abort fix path" button. It stops everything and restores the original path before snipping.
  • Remember the path and the direction the player was walking. The player can press "continue walking". They continue if they are on the same path (even at different tile of the path). Otherwise popup with info to the closest tile and the tile you left the path: popup( "You are no longer standing on the path you were walking. You left the path 3SW, closest tile is 1N." )
    • This is close to a special case of the next one. So don't do this.
  • Walk from middle:
    1. Find paths under the player's feet (not start/end but any tile).
      • With the optimizations, this could be decently fast.
    2. The player selects a path.
    3. The player selects whether to go to the path start or the path end.
  • Edit paths without the need to walk them: Instead of walking, record the cursor position in look around. Such paths might need some fixing on first use.

Describe alternatives you've considered

Record keystrokes #74433 (comment)

Testing

I tried whatever I could think of. Should work on flat ground, with stairs, and ladders. Doesn't work with ramps, I will look into Travel To code when I have the time.

Additional context

@github-actions github-actions bot added <Bugfix> This is a fix for a bug (or closes open issue) <Enhancement / Feature> New features, or enhancements on existing [JSON] Changes (can be) made in JSON [C++] Changes (can be) made in C++. Previously named `Code` astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jun 17, 2024
@Brambor
Copy link
Contributor Author

Brambor commented Jun 17, 2024

It is not ready for merge, but it is ready for review.

@anothersimulacrum
Copy link
Member

I'm not sure this is a benefit. Like macros, this risks making it a player responsibility to make up for deficiencies in game systems and UX.

@Brambor
Copy link
Contributor Author

Brambor commented Jun 18, 2024

@anothersimulacrum You are not the only one. @ GuardianDll pushed back too and warned me. I am totally fine with this not being merged. As stated on Discord, I made it for myself. Merging is just more convenient to me.

@Brambor
Copy link
Contributor Author

Brambor commented Jun 18, 2024

TL;DR: I see the alternatives as either suboptimal or more work (for the player).

I disagree though, that Travel to, and such, could be improved to the level this would be completely obsolete. For me, Travel to would have to find the optimal* path between A and B and that is next to impossible in all cases. Small improvements just don't do it for me. I need to believe the path is optimal.

Just not being able to mark tiles as dangerous per-tile makes Travel to sub optimal. If that was implemented, then marking all dangerous tiles would be too much work. So let's consider only tiles I walked on already, I could have walked into a puddle I don't want to walk into again. Then I need to mark it off again. I could mark furniture_type = puddle to be blacklisted. That wouldn't work, since it sometimes is better or possible to only walk through puddle. So we would define cost for each tile ... ... ...

@Brambor

This comment was marked as resolved.

Copy link
Contributor

Spell checker encountered unrecognized words in the in-game text added in this pull request. See below for details.

Click to expand
  • Recorded path has no lenght. Path erased.

This alert is automatically generated. You can simply disregard if this is inaccurate, or (optionally) you can also add the new words to tools/spell_checker/dictionary.txt so they will not trigger an alert next time.

Hints for adding a new word to the dictionary
  • If the word is normally in all lowercase, such as the noun word or the verb does, add it in its lower-case form; if the word is a proper noun, such as the surname George, add it in its initial-caps form; if the word is an acronym or has special letter case, such as the acronym CDDA or the unit mW, add it by preserving the case of all the letters. A word in the dictionary will also match its initial-caps form (if the word is in all lowercase) and all-uppercase form, so a word should be added to the dictionary in its normal letter case even if used in a different letter case in a sentence.
  • For a word to be added to the dictionary, it should either be a real, properly-spelled modern American English word, a foreign loan word (including romanized foreign names), or a foreign or made-up word that is used consistently and commonly enough in the game. Intentional misspelling (including eye dialect) of a word should not be added unless it has become a common terminology in the game, because while someone may have a legitimate use for it, another person may spell it that way accidentally.

@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed astyled astyled PR, label is assigned by github actions labels Jun 24, 2024
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Jun 27, 2024
@db48x
Copy link
Contributor

db48x commented Jul 1, 2024

I really like the UI!

@Brambor
Copy link
Contributor Author

Brambor commented Jul 1, 2024

I will continue with this on 2024-07-22. I just figured this could be improved:

I want (i) to show in detail what some buttons do. (i) doesn't show in ImGui demo with my version of ImGui. I should mark that regression separately.

I would like it if a TAB would always switch focus from buttons to paths list. I will see if I can make the buttons unselectable. Like in the keybindings menu.

I dislike that when you browse paths with UP & DOWN, the bright blue is "hovered over" and light blue is "selected". This is confusing. I accidentally deleted the wrong path.

Add a popup on deleting a path. With all the path's info.

The keybindings are what I figured could work. The menu could be opened with } by default.

@Brambor
Copy link
Contributor Author

Brambor commented Aug 4, 2024

Added "Walk path from middle", see the Original Post.

@kevingranade
Copy link
Member

Just to repeat what guardiandll and anothersimulacrum already told you, this isn't getting merged, but if you want to work on it in this PR that's fine.

@Brambor
Copy link
Contributor Author

Brambor commented Aug 4, 2024

Just to repeat what guardiandll and anothersimulacrum already told you, this isn't getting merged, but if you want to work on it in this PR that's fine.

I thought my argument was accepted:

TL;DR: I see the alternatives as either suboptimal or more work (for the player).

I disagree though, that Travel to, and such, could be improved to the level this would be completely obsolete. For me, Travel to would have to find the optimal* path between A and B and that is next to impossible in all cases. Small improvements just don't do it for me. I need to believe the path is optimal.

Just not being able to mark tiles as dangerous per-tile makes Travel to sub optimal. If that was implemented, then marking all dangerous tiles would be too much work. So let's consider only tiles I walked on already, I could have walked into a puddle I don't want to walk into again. Then I need to mark it off again. I could mark furniture_type = puddle to be blacklisted. That wouldn't work, since it sometimes is better or possible to only walk through puddle. So we would define cost for each tile ... ... ...

But ok, thanks for letting me know. I guess I don't need to polish it any further then.

@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Aug 6, 2024
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Aug 6, 2024
@Brambor
Copy link
Contributor Author

Brambor commented Aug 6, 2024

It is ready. I am leaving it in draft since it will not get merged. Then reviewers don't have to look at an open PR.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` <Enhancement / Feature> New features, or enhancements on existing [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Record path then take it (again and again).
5 participants