Feat/reset view on hyperspace exit #5922
Conversation
mwerle
force-pushed
the
feat/reset_selected_system
branch
from
2155638 to
c388417
Compare
I suspect this is an edge use case. I think we had a crash bug when doing this, and took a while before it was discovered. |
|
This PR can also of course be extended to tackle some of the issues with the proposed change and I'll defer merge until that's been accomplished at your discretion. I similarly had the idea that it might be good to have an explicit "view system" button in the Sector Map which opened the System View on that specific system and otherwise decouple the System View from the Sector Map selection. |
|
So I did some testing and it seems that moving the code to LUA breaks it - calling I've also added code to force-change the view back to the game view when exiting Hyperspace, and I think that might not be a bad option to add to the game. Not sure how to add options though... (offtopic) (offtopic 2) I got as far as |
NVM, figured it out. |
mwerle
force-pushed
the
feat/reset_selected_system
branch
from
c388417 to
fb745c4
Compare
|
Ok, so I've added an option "Reset View on Hyperspace Exit". This option is on a new tab "Options", and I've also moved the "Enable Autosave" option to this tab as it's got nothing to do with the various graphics settings. This is done in 2 commits to logically separate the work. [EDIT: just saw that a game-settings tab was also implemented in #5655] If "Reset View on Hyperspace Exit" is unchecked, then the game stays on whichever view the player is currently in. If the current view is the "System Map" or the "System Overview", this will change to the current system when the hyperspace jump is completed. If "Reset View on Hyperspace Exit" is checked, the player is ejected back to the Game View from whichever screen they are currently in. Brief testing appears to show no issues. If people like these changes, I will squash the various commits and convert the PR from draft to a real PR. Otherwise open to improvement suggestions. |
I'm not entirely sure I follow this thought. Which system should be opened from this explicit "View System" button? The only one that makes sense is the currently selected system.. which is what happens already. Unless you add a right-click menu for a star.. |
|
@mwerle I think you might want to try the debug tool we have in-game: Ctrl+i to give yourself hydrogen, ship, enemy, etc. |
Thank you; that is very useful! I had actually stumbled on this before, but didn't see the section where you can buy commodities through it.
Well, yes, I can open it.. but the example given on the wiki page to add cargo doesn't work (I didn't try the others) and I couldn't figure out a way to make it work. No longer required due to the debug tool, but we might want to update the wiki.. |
|
Ok, big changes to the proposal. No longer change the currently selected system on hyperspace exit. -> Reverted in commit 7f7f701 Instead, add a new button to the System Map and System Overview screens to switch the display between "Current" system and "Selected" system (defaults to "Selected" to keep current mode of operation). If the display has been switched to "Current" system, then the map is updated on hyperspace exit to show the arrival system. -> Implemented in Commit 6c8d551 Also add a new game setting to "Reset View on Hyperspace Exit". When checked, the view is reset to the game view when exiting hyperspace. The view itself is not changed (ie, internal/external/direction). There's an argument to reset the view to internal-forward.. -> Implemented in commits c666827, c666827, c666827, and c666827 Also found and fixed an edge-case bug where the next hyperspace system was not being properly updated on hyperspace exit if the player did not enter the "Sector Map" following starting and loading the game, and immediately starting a hyperspace jump. Fixed in d740e42 Also refactored the "edge buttons" of all map screens - they are now aligned to the top-right, and the first set of buttons is the same between all screens. -> Changed in commit c666827 Feedback welcome; especially choice of icons. Have done some testing, but not comprehensive yet (will do so after dinner). Test plan:Game View:
No-Game and Non-Map View (eg, Personal Details):
Sector Map View:
System Map or System Overview:
|
Test Run : Micha 2024-09-30Game View:
No-Game and Non-Map View (eg, Personal Details):
Sector Map View:
System Map or System Overview:
Test savefile(Remove the ".zip" ending) |
|
regarding pain of lua/c++ development:
|
It's mostly not being able to trace through to see the logic in the Lua executing with variable views. It took me well over 3 hours to track down a rather trivial bug, which should have taken less than 10 minutes. And I only cracked it because I went back to the "poor man's debugger" and added But yes, the hot-reload is very nice when fiddling with UI layouts. And, I suspect, tweaking of AI routines for the NPC ships. Personally I'd still leave the vast majority of the game logic in the C++ though.. just add relevant Lua hooks everywhere for modders. |
This is definitely a pain-point to be sure. We have a function
The wiki is quite outdated and the developer-facing portions are in the process of being moved to https://dev.pioneerspacesim.net/. We have a debug tool for cargo as you've found, so updating that wiki has been historically low-priority. If you want to programmatically add cargo you can do so via: player:GetComponent('CargoManager'):AddCommodity(require 'CommodityType'.GetCommodity("hydrogen"), 2)Note that we also have a Lua codedoc providing documentation for the various Lua utilities and classes. If you're using VSCode or another language-server-protocol compatible IDE, the LuaLS tool is incredibly helpful and provides type information and autocomplete when working with type-annotated Lua files. We're incrementally working on expanding the type information / annotation coverage of the Lua codebase as time goes by. |
This sounds good. I'd name the tab "Game Options" specifically, as it has to do with in-game behaviors.
I'd recommend renaming this to "Close Map on Hyperspace Exit" - "Reset View" is semantically overloaded and can refer to changing the camera mode, changing the camera orientation, or changing to the WorldView.
I like this change, thank you!
The idea here was to completely remove the automatic system selection of the system map, and instead add an explicit button in the sector map to open the system view for the selected system, which system would remain open in the system map even if the sector map cursor was moved. Because the default behavior would be for the system view to not change the system viewed unless the user explicitly commanded the viewing of a different system, we'd most likely need a separate button accessible from the WorldView to open the current system in the system map. This is a quite involved change however, so I'm merely proposing it for discussion / evaluation rather than implementation. Note that you can collapse long lists on Github inside a <details>
The empty lines are important
Some long list
</details> |
Note that these edge buttons are loosely intended to be converted to use the
There is a lot of opinionated game logic in Pioneer that is neither a performance concern nor a "universal constant" but is still hard-coded in C++ and cannot be altered without significant work. My personal approach is that logic in C++ should be "composable" - it's a set of building blocks that the "default mod" arranges to produce the intended balance and behavior of the game. Other mods can then override and alter the arrangement of those building blocks to produce a different result. What we have now is the "sprinkle hooks everywhere" method, and it's produced a very brittle and inflexible codebase. For example, fuel scooping is "game logic" that is (and was even more so) completely hardcoded in C++. As a result, mods cannot e.g. allow scooping a star, change the minimum atmospheric density to scoop, or allow scooping any other material other than thruster fuel. ...that's a bit of an off-topic rant though. I do agree that a lot of game logic can (and often should) live in C++, but there has to be a great amount of attention paid to how that logic interconnects with other parts of the game. All that out of the way, I'll try to give this branch a playtest soon and provide more actionable feedback about the concerns you raised above. My mental queue is currently full, so it may be a few days however. |
Question Do you mean the internal name of the tab, or the tooltip text (or both)? I simply chose "Options" for the tooltip to re-use an existing translation but trivial to update.
Well, it does the last - returns to the Game View (is that "WorldView"? I am meaning the in-flight view, either internal or external) irrespective of what the current view is. It doesn't just return to the Game View from the various map screens, but also from the various informations screens which can be accessed in-flight. There's an argument for not only returning to the Game View but also resetting the view to "internal-front" so the player can instantly react to hostile actions etc. For now I did not change the actual view configuration as the player might prefer to play in external-camera mode.
So the user can already toggle the automatic system selection in the Sector Map; if the user doesn't like this, they can always disable that, which means the System Map / System Overview remain on the currently selected system. And with my addition of toggling between the currently selected system and the current system there's no need for the user to change the currently selected system unless they want to view a different system. So while it doesn't quite work the way you envision with explicit buttons and a second "selected map system" state, the user-story that you envision can, I think, be achieved with the currently proposed changes.
I can revert the edge-button changes; it just made a little bit more sense to me to have them in a more fixed location and with a more consistent ordering. If no coding has actually been starting on the proposed UI update, then I don't think my changes would interfere with that work and, until that is done, it is a slightly improvement IMHO. EDIT Also it would require opening the side-view before being able to use the various action buttons. Some of these buttons only toggle additional information views, but some perform actual actions. I think it makes sense to condense the various information views into a sidebar, but not so sure about the action buttons. Question I can leave the edge-button refactor as-is for now or revert, please let me know your preference. Thank you for your time in replying and evaluating this PR, as well as your additional information about the game design and debugging/investigating the Lua components while developing. I will try your suggestions. |
Yes, that's the WorldView I must say, it's really great to see some more contributors playing with our source code. Also @Web-eWorks is currently swamped with #5734 (feel free to help playtest and/or review, although the latter would be daunting for any mortal man), so replies & review of your PRs might be a while.
I'll let @bszlrd answer ☝️ |
The tooltip text specifically shown to the user, as well as an appropriate icon. Internal name can be whatever is semantically connected to the text that is shown to the user - either is fine.
I saw that it closes e.g. the Personal Info screen as well on exiting hyperspace after writing that comment. Leaving the name of the option as-is is fine in light of that information - a clearer name would be too verbose.
This is good, thanks! I like what you're describing. I expect this to be ergonomic enough that there's no need for my proposed solution.
Yes - it's a mockup to show general layout and hierarchy, not final layout and design. 😄 My intention when implementing that mockup would be to split functionality between left and right sidebars as needed, moving commonly-used context-free buttons (i.e. rotate map, reset view) either to a central button dock at the top/bottom center of the screen, or simply as a "regular button" in the row of sidebar toggle buttons.
I've not yet gotten a chance to play with it or review the code, but I'll say leave as-is. This was mostly to inform you about future plans for the screen so you don't feel like your work is accepted and then (eventually) immediately discarded in a redesign.
And thank you for your time developing it! I'm happy to spend the time I have available to accelerate other contributors - many hands make light work, after all. I don't know if I can promise a timeline for "final review" but as soon as I clear some other IRL responsibilities off my plate I'll try to give this a formal review + playtest. |
mwerle
force-pushed
the
feat/reset_selected_system
branch
from
7f7f701 to
34adbe5
Compare
Fixed. The tooltip is now "Game Options". I left the icon as the "cog" as that is a very commonly used icon for settings/options. If there is a better icon to use, please suggest one.
Of course not; this is a hobby and I see you have a major feature implementation which has just entered review as well. There is no expectation for this PR to be reviewed within a specific timeframe. Rebased/squashed commits and reworded the PR. Based on the initial feedback, I think this PR is now ready for proper review. |
data/lang/ui-core/de.json
Outdated
| @@ -715,6 +715,10 @@ | |||
| "description": "", | |||
| "message": "Vollbildmodus" | |||
| }, | |||
| "GAME_OPTIONS": { | |||
There was a problem hiding this comment.
You only need to update the en.json file, it's the "master", this will then propagate new strings to all other language files (might be temporarily not working though, but that's on me).
There was a problem hiding this comment.
As I speak German, I added the German translation as well..
There was a problem hiding this comment.
Your edit will probably get over-written by transifex. I can never quite remember what happens when editing the non-en.json files.
If you want to maintain the german translation, that should be done through transifex.
There was a problem hiding this comment.
Removed the german translations.. will add them again when they become available on transifex.
That said, I think it would be better if people could supply translations however they wished, with transifex being one option. transifex should populate itself from the existing data.. but no idea if that actually is possible. First time hearing about that system.
There was a problem hiding this comment.
I'm quite happy with this! Two minor nitpicks in terms of consistency and maintainability, but overall I like the effect of the changes.
Something to consider for a future PR - the player might want a "shortcut" to open the system view and switch to current system view mode from the WorldView (and similarly for selected system mode from the Sector Map) - it's extra cognitive load to check that the map is in the correct view mode after opening it.
That's not something I expect you to implement at current however; it's a better topic for later down the road when the Sector Map UI gets looked at for a rework.
src/SystemView.h
Outdated
| enum class SystemSelectionMode { | ||
| CurrentSystem = 0, | ||
| SelectedSystem = 1, | ||
| }; |
There was a problem hiding this comment.
This enum should be annotated for scan_enums.py to generate enum_table entries.
| enum class SystemSelectionMode { | |
| CurrentSystem = 0, | |
| SelectedSystem = 1, | |
| }; | |
| enum class SystemSelectionMode { // <enum name=SystemSelectionMode scope='SystemView::SystemSelectionMode' public> | |
| CurrentSystem = 0, | |
| SelectedSystem = 1, | |
| }; |
There was a problem hiding this comment.
Fixed in 62aad68
Generated code should (IMHO) never be checked into source control; instead, the relevant generator should be invoked during configuration and/or build time..(*) However, requiring this would mean developers would need a version of Python2 installed.. which is currently not the case as long as a developer doesn't add a new enum which should be usable by Lua.
That said, if I'd read the top of the file I guess I would have figured it out too ;) I literally only searched for the other enum and inserted all the required bits for my new one.
(*) Which, it appears, does happen on system which have a version of Python 2 in the path as python - on modern Debian systems, this is not the case. I fixed this in 5bcbfd6
There was a problem hiding this comment.
So there's one specific problem with this approach - using the Ninja generator, every time a rebuild or incremental compile is triggered every file which includes enum_tables.h is recompiled as the file has "changed" on disk. Worse yet, it looks like those changes to enum_tables.h aren't actually picked up the first time the project is built following a valid change to an annotated enum - it seems CMake (or in this case Ninja) scans dependencies before invoking the scan_enums.py script, so it's actually the second rebuild that produces a valid program state.
I'd ask that you remove the dependency between pioneer-core and the scan_enums invocation - instead, leave it defined as a CMake command and we can document it as something that should be run manually in the (very low-frequency) case of modifying an annotated enum value.
There was a problem hiding this comment.
While I do agree with you in the general case that autogenerated code should likely not be checked into the build system at all, this is a sort of "gray area" where the file typically changes only once every few months, does not have easily-tracked dependencies to determine when it should be re-generated, and touches several distinct areas of the codebase (for example, the script also generates data/meta/Constants.lua). As a result it's (historically, in this codebase) "easier" for it to be checked into source control and manually re-generated when actually necessary.
There was a problem hiding this comment.
I couldn't figure out a way of running it manually and assumed that it just ran at some point during the build process, hence adding the explicit dependency to ensure it ran early in the build.
ninja enums gave me an invalid target warning. I'll investigate why.
Good catch that it doesn't build properly the first time around.
Perhaps instead of having it in the CMake just document that it needs to be invoked? Or run it as part of the build configuration phase?
There was a problem hiding this comment.
...and trying again now it works as expected, with both ninja and make invoking the command.. so removing the target dependency, but leaving in the change to properly find Python 2.
There was a problem hiding this comment.
Thanks! Having it in CMake is mostly so it can be quickly invoked, e.g. via cmake --build build/ --target enums or make -C build enums, as well as serving as "living documentation" for the correct invocation of the script.
mwerle
force-pushed
the
feat/reset_selected_system
branch
from
34adbe5 to
62aad68
Compare
|
Rebased on |
The "scan_enums.py" script requires a Python v2 interpreter to be available. The "CMakeLists.txt" simply searched for a Python interpreter, which failed on modern Debian systems. Instead of simply searching for "python", the CMake script was adjusted to specifically search for a Python 2 interpreter, and then to explicitly invoke that to run the "scan_enums.py" scripts.
Add a new Options tab to the settings dialog for "Game Options" in preparation for adding additional settings to the game. Move the "Autosave" setting to this new tab. This option is currently in the "Graphics Options" tab where it does not belong.
Add an option to reset the view to the WorldView on hyperspace exit to the "Game Options" tab of the settings window. When this option is enabled, the view is reset to the WorldView after a hyperspace jump.
Prepare for additional buttons to be added to the "edge buttons" area of the map views. Move the edge buttons to the top-right of the display, so all icons remain in a fixed location irrespective of how many buttons there are. This makes it easier to learn muscle-memory. Also, rearrange the icons so that all 3 map views have the same list of buttons from the top down, with any map-specific buttons following the "Info" button. Finally, remove the superfluous edge button to switch between System Map and System Overview - they already exist on the top button bar.
Add a button to switch between the Current System and the Selected System in the System Map and the System Overview screens. With this change, if the player has selected to view the current system and is viewing one of the maps during a hyperspace jump, a situation can occur in which SystemView::CalculateStarportHeight() is called while the game state is in hyperspace. Hence a check is required that the game is in normal space prior to calculating the real height of a starport above a planet surface.
Minor improvement to prevent unnecessary "onHyperspaceTargetChanged" events from being generated if the hyperspace system has not actually been changed to a new system.
The call to ResetHyperspaceTarget in Player::OnEnterSystem has been superfluous since probably commit 1e2e471. As there was a comment that the SectorView should not be invoked from this callback, and recent testing showed that it did not impact the game logic, lets remove it and resolve the comment.
mwerle
force-pushed
the
feat/reset_selected_system
branch
from
62aad68 to
7c5a654
Compare
|
Remove target dependency for the Thank you for reviewing and the various comments, suggestions, and assistance with implementing these changes. |
If there's anything missing from COMPILING.txt that can be helpful for package maintainers, or anyone wanting to compile & run pioneer, please add. |
Fixes #5691
Background
The current game has a concept of a "selected system" in the
Sector Map. Whenever opening theSystem Overviewor theSystem Map, this is the system whose details will be displayed.The only way to change the "selected system" is by the user changing it in the
Sector Map.This is somewhat unintuitive from a players' perspective. When the player is interacting with the various maps, then yes, this is the expected behaviour, however, when the player is jumping into a new system, upon arrival, it is more intuitive for the
System MapandSystem Overviewto show the current system.Proposed changes
This PR proposes to fix this with 2 partially-related changes:
When the option to reset the view to the WorldView is unchecked (default), and the Map Display is set to the selected system (default), there is no change to the game behaviour from the current behaviour.
Reset View after hyperspace exit
This was originally added to avoid certain issues with the selected system changing, but is kept as some players may prefer this . This could be split into a separate PR.
A benefit of enabling this option is that players can immediately react to hostile actions following a hyperspace exit.
An improvement to this option may be to also reset the view to "internal" and "front" - the current implementation maintains the last camera setting.
Add Map View mode to selected or current
When the Map View is set to the current system, the System Map and the System Overview will show the current system instead of the selected system. This can be especially useful if the player is exploring and wishes to quickly examine the current system while following a hyperspace route of multiple jumps.
One side-effect of this change is that if the player is currently viewing one of the maps while performing a hyperspace jump, then the view is updated as soon as the new system is reached.
On the one hand this could be seen as a benefit as the player is immediately notified of arrival in a new system, on the other it could be disruptive if the player is currently examining some aspect of the previous system.
It would require additional work, potentially complex and fragile, to keep the view on the currently opened system.
Issues with these changes
Apart from those already mentioned, there are no known issues or new bugs introduced by these changes.
Original Draft-PR Description
The only way to change the "selected system" is by the user changing it in the
Sector Map.This is somewhat unintuitive from a players' perspective. When the player is interacting with the various maps, then yes, this is the expected behaviour, however, when the player is jumping into a new system, upon arrival, it is more intuitive for the
System MapandSystem Overviewto show the current system.Proposed changes
This PR proposes to fix this by resetting the "selected system" to the "current system" when entering a new system.
Issues with the fix
The current implementation has the downside that if the player is viewing the
System MaporSystem Overviewwhile performing a hyperspace jump, the map is changed as soon as the jump is complete, even if the player is interacting with the map at the time.Discussion
Hence this PR is still a "draft" while ways to prevent the map from changing are investigated. While some players might like the current approach (ie, it's obvious when the jump is complete), others might prefer to keep viewing the old map.
One solution could be that the player is forcibly returned to the flight screen when the hyperspace jump is completed. This has the benefit that the player is immediately able to react to, for example, hostile actions by other ships.
This should be fairly easy to implement.
Another suggestion has been that the "selected system" is not changed, but rather that the
System MapandSystem Overviewshow the "selected system" when opened from theSector Map(and possibly some other screens such as the BBS), but the "current system" when opened from the spaceflight screen. This might lead to confusing behaviour depending on the exact sequence of opening maps and the starting screen compared to what the player expects. This is also likely to be somewhat more complex to implement.