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

Toggle to Only Draw Overmap During Autotravel #76017

Merged
merged 2 commits into from
Aug 31, 2024

Conversation

ShnitzelX2
Copy link
Contributor

Summary

Interface "toggle to only draw overmap during autotravel"

Purpose of change

Alternative to #75600, see that issue

Describe the solution

Separates the overmap UI from its static function by making a singleton overmap_data held in the game object. By doing this, we can draw the overmap UI e.g. over multiple game turns. This was bound to a toggle key on the overmap, so we can do this while autotraveling:

Recording 2024-08-27 at 12 23 21

The time save is significant. For roughly 110 OMTs' uninterrupted drive in a 4x4, the following data was gathered:

Drive Type Time
TILES 40 sec
TILES with fast travel 24 sec
NO TILES 34 sec
NO TILES with fast travel 22 sec

...and then on foot following the same path,

Walk Type Time
TILES 20 sec
TILES with fast travel 14 sec
NO TILES 20 sec
NO TILES with fast travel 14 sec

Other Notes:

  • This works for both tiled and ASCII overmaps.
  • Any UI windows still draw over the overmap, including distractions, "Driving... press 5 or .", etc.
  • When travel is finished or interrupted, you are booted out of the overmap.
  • It's called "fast travel" in-game for lack of a better name. The toggle is currently bound to 'x', let me know if we want it changed.
  • This is meant for traveling a safe or mostly-safe route and is less safe than just autodriving/moving; you only have yourself to blame if you ignore "mi-go spotted!" and end up next to a mi-go.
  • This makes it easier to find wandering NPCs and hordes on the overmap.

Describe alternatives you've considered

See original issue

Testing

In addition to the basic tests in the tables above:

  • fast travel into a monster
  • ..ignoring that monster, then about to crash in vehicle
  • ..ignoring that monster on foot
  • fast travel while path blocked
  • fast travel while grabbing furniture
  • fast travel with allies
  • fast travel on foot next to projectile shooter (no less safe than without fast travel)

Additional context

Drafted until clang runs and tests run locally

@github-actions github-actions bot added Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON Map / Mapgen Overmap, Mapgen, Map extras, Map display [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Aug 28, 2024
@CLIDragon
Copy link
Contributor

To confirm, the only changes are that the map is not drawn during fast travel? The simulation is still fully running?

And could you provide flame graphs / call stacks of before / after? When I've profiled overmap travel before, most of the time was spent in map::load and map::shift (along with the usual suspects of character::update_body, map::regenerate_visibility_cache, and so forth), rather than anything ui-related.

-moves overmap UI parameters to data struct (kept in game object for now) or to uistate
-importantly, of those parameters: the ui adaptor, which can be preserved over do_turn()
- cursor+PC follows along omt path
@ShnitzelX2
Copy link
Contributor Author

To confirm, the only changes are that the map is not drawn during fast travel? The simulation is still fully running?

Yes, this change was a suggestion by kevin for improving autotravel speed in #75600. It isn't actually fast travel, it is solely to cut back on draw time. If we want a different name or keybind, feel free to post ideas.

For the same path traveled by car:
Without fast travel (ui_adaptor::redraw_invalidated at 52% usage, 4.2 sec travel)
image

With fast travel (redraw at 19% usage, 2.3 sec travel)
image

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 29, 2024
@CLIDragon
Copy link
Contributor

I don't know if it would be possible but what about a message when you auto travel? Something like:

Don't show terrain while travelling?
Yes/No/Always/Never

That way it doesn't need a new keybind.

@ShnitzelX2
Copy link
Contributor Author

I considered a dialogue option upon autotraveling, but figured it would be annoying if you didn't set it permanently. And if you did, there'd have to be an option somewhere else to reset Always/Never. I think the keybind's fine, just the key specifically ('x') was what I was unsure about.

Anyways, this PR is ready for review.

@ShnitzelX2 ShnitzelX2 marked this pull request as ready for review August 30, 2024 04:09
@CLIDragon
Copy link
Contributor

I considered a dialogue option upon autotraveling, but figured it would be annoying if you didn't set it permanently. And if you did, there'd have to be an option somewhere else to reset Always/Never. I think the keybind's fine, just the key specifically ('x') was what I was unsure about.

Ah, the blame's on me for not looking at the screenshot close enough. I thought it was a separate button that replaced the auto travel button.

@Maleclypse Maleclypse merged commit 88ecaf6 into CleverRaven:master Aug 31, 2024
38 of 44 checks passed
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 [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants