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

New game start dialog #5561

Merged
merged 11 commits into from
Aug 25, 2023
Merged

Conversation

Gliese852
Copy link
Contributor

изображение
изображение
изображение
изображение
изображение

Visually, the dialog consists of widgets grouped into 4 tabs. The internal state is represented by a set of parameters sufficient to run the game. Widgets do not exactly match parameters - there may be widgets not represented by parameters, and parameters not shown as widgets, but most parameters correspond to some widget.

Each parameter is an "instance" of some class, it can pull its data from multiple sources (built-in start variant, savegame document), display an interface for viewing/editing, validate it, and export the data to the game's start routine.

Widgets have the property of blocking, by default editing most of the parameters is not available. In order to unlock all parameters, there is a 'Custom start' option, but it is disabled by default. To enable it, you need to activate the 'Debug start dialog' checkbox in the debug window.

First of all, the purpose of this menu is the basis for restoring obsolete saves. Although in this PR, such an feature is not yet available. As the next step, it is planned to add the ability to import the state from the savegame. And if some parameters cannot be imported, allow them to be edited manually.

In addition, skins for starting ships were fixed:

изображение
изображение
изображение

Specific skins are, of course, a matter of debate.

Also added a simple random name generator for the ship, lists are taken from here: https://github.com/thomasleveil/name-generator/tree/master/data

@impaktor
Copy link
Member

What's the license of https://github.com/thomasleveil/name-generator ? I don't find any License file.

@craigo-
Copy link
Contributor

craigo- commented Feb 28, 2023

Restoring obsolete saves? This is fabulous news - thank you, looking forward to it.

If you are keeping a list of objectives, may I please suggest that the feature is able to import previous flight logs? That is one element that cannot be 'cheated' via the Lua Console.

@sturnclaw
Copy link
Member

I can already tell this will be a fun review process - from a quick skim (and previous looks at e.g. sector map separation) I'm liking the code I see so far. There may be a few patterns employed in the substantial amounts of new code in this PR which I may ask for a refactor/improvement on, but I won't have any concrete examples until I start going through the detailed review process.

I'll start by mentioning the biggest and most onerous task here: the list of names used for ships needs to be pruned to remove any 'handles' (TwoWord style names, e.g. BarryNelson) and any immediately recognizable character names from works of fiction outside the Pioneer universe or more generally covered under intellectual property laws (e.g. C3PO, Daenerys, Galadriel, etc.). This is something that will have to be done manually, as there are a lot of offending entries in that file.

I know we're not a commercial entity and the chance of getting into any actual trouble related to IP law is slim-to-none, but these names set the tone for Pioneer's universe as one of the first "in-universe" elements the player sees (on the start screen, even!) and thus a significant amount of care needs to be taken here. 'Handle' style names can be split into their component "real world" names and added as two separate entries at your discretion, (e.g. "Barry", "Nelson",).

Finally, ship names are internationally considered Proper Nouns, and thus the left and right components of a ship name should both have the initial letters capitalized.

(...Hopefully I've bought myself enough time here to make headway on the detailed review 😅)

@Gliese852
Copy link
Contributor Author

Made some fixes - just comments, and some unused code

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed some low-hanging fruit in several areas, but will defer review of the SectorMap and main game start dialog to the next batch of review comments. I need to spend some time reading that code and see if there are some prior comments/concerns discussed on IRC that still need addressing.

Well done on what I've reviewed so far!

src/pigui/ModelSpinner.cpp Show resolved Hide resolved
data/pigui/libs/wrappers.lua Outdated Show resolved Hide resolved
src/lua/LuaUtils.cpp Show resolved Hide resolved
data/libs/utils.lua Outdated Show resolved Hide resolved
src/Pi.cpp Outdated Show resolved Hide resolved
@Gliese852
Copy link
Contributor Author

@craigo- thanks for support! I think this can be implemented (import previous flight logs)

@Gliese852
Copy link
Contributor Author

@Web-eWorks about the list of words - I think for a while to wait a bit, because it is not clear about the license. I asked the author a question, but he is still silent. Maybe I'll have to clean up the whole list and look for another.

@impaktor
Copy link
Member

impaktor commented Mar 1, 2023

If you are keeping a list of objectives, may I please suggest that the feature is able to import previous flight logs? That is one element that cannot be 'cheated' via the Lua Console.

Alternative, there was a request on SSC, to export flight log to txt file. It's a feature I'd like to see.

@sturnclaw
Copy link
Member

A few issues I'm noting here:

image

  • You cannot change your name via the name input field, other than to randomize it.
  • You can click into and select text in the Rating / Reputation fields, but cannot type anything
  • The colors used for the forward/backward buttons give the indication that they are disabled, even though they can be used. Ideally a "regular" white color would be used here (perhaps with a thinner line-weight icon if the intent is to avoid distracting from the category icons).

Similarly on the ship page:

image

  • You can select the ship type and registration number field but cannot change any text inside it.
  • You can amend text in the Ship Name field but it will not save and reverts back to the randomized name.
  • The Propulsion, Front weapon, and Rear weapon fields are all selectable text input fields but you cannot change any text inside.
  • Same for the system name input field on the Location screen.

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few (11) change requests for the Lua code aspect of things. I've taken a fairly high-level look at the Lua code and tried not to nitpick the architectural design too closely; at this point I think you write high-quality code and I don't need to "police" it.

The design of GameParam is acceptable, though the "locking mode" may need a second thought as I'm not seeing any uses for lock mode other than on or off (and future special "locked" modes should probably be initialized from the start preset and the locked boolean used only to determine whether the lock mode should apply or not).

Regarding the C++ side of things, I want to say there may be a slightly more performant way to handle SectorMapContext::Callbacks::GetDisplayMode() than a virtual function dispatch, but at the current juncture I have no issues with it. Otherwise the code looks fairly good and I'm quite happy with the cleanup you've done in various files!

data/pigui/modules/misc-debug.lua Outdated Show resolved Hide resolved
src/lua/LuaGame.cpp Outdated Show resolved Hide resolved
data/pigui/modules/new-game-window/widgets.lua Outdated Show resolved Hide resolved
data/pigui/modules/new-game-window/layout.lua Outdated Show resolved Hide resolved
data/pigui/modules/new-game-window/layout.lua Outdated Show resolved Hide resolved
data/pigui/modules/new-game-window/start-variants.lua Outdated Show resolved Hide resolved
data/pigui/modules/new-game-window/summary.lua Outdated Show resolved Hide resolved
end
end

local function startGame(gameParams)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this function should provide a way for modules which provide additional start variants etc. to perform initial setup on the player's ship / universe state, perhaps by calling an event handler and passing the gameParams value.

src/lua/LuaSectorMap.cpp Outdated Show resolved Hide resolved
@Gliese852
Copy link
Contributor Author

@Web-eWorks

You can click into and select text in the Rating / Reputation fields, but cannot type anything

This was done, one might say, intentionally, on the one hand, this is a parameter, it’s just that its editing is blocked, on the other hand, I was too lazy to insert plain text there and add a background to it, calculate all this, etc. If you think it's important, I'll change it.

@Gliese852
Copy link
Contributor Author

@Web-eWorks I noticed that the game may crash when starting in a multiple system, apparently this was already fixed in the c58d3bb, this branch just budged earlier.

@Gliese852
Copy link
Contributor Author

Tried to filter (to the extent of knowledge) the list of ship names.

Gliese852 and others added 10 commits August 23, 2023 23:28
There was a need to use the sector map in different contexts, not only
inside the SectorView. Therefore, the 3d star map viewer has been moved
to a separate SectorMap class. The SectorMap class has an interface for
adding custom objects to the map - lines, spheres and round indicators.
The SectorView uses this to overlay game logic objects (player location,
route, etc.) on the map. Some functions that SectorView used to perform
directly are passed to SectorMap, and are performed via
GetMap()->Function(). The SectorMapContext class has also been added,
which contains all the necessary external information for the SectorMap
to function.

Also added the ability to display SectorMap inside the ImGui window.

Some additional modifications:

  * completely get rid of Pi:: in the SectorMap class
    - move GetMoveSpeedShiftModifier() to Input::Manager
    - explicitly pass frame time to SectorMap::Update
  * input tweaking
    - receive wheel data directly instead of via a signal
  - draw arrow icons instead of '<' and '>' chars

  - scale the face generator buttons proportionally to the fonts so that
    it is more predictable in the game start menu

  - make a public renderFaceGenButtons method so that it can be
    controlled from outside
  - add the ability to draw with or without a progress bar

  - now it works well with complex formats like YYYY-MM-DD, will need it

  - add mouse wheel control

  - also apply small fix in ui.withButtonColors, to wrap the drag call
    in this function
  - pass any flags to the ColorEdit widget

  - simplify color passing to ModelSkin

  - fix the use cases
When setting the ModelSpinner projection, the FOV parameter means the
vertical viewing angle by default. This causes the zoom to be dependent
on the viewing height. Therefore, for example, in the ships market,
where the height of the widget with the ship is small, the ships are
drawn very small and there is a lot of free space on the sides.

Changed to make FOV stand for the horizontal field of view. Now, it
seems, the ships will be displayed more optimally.
  - fix that ui.withStyleColors couldn't return multiple values

  - return value from ui.withButtonColors

  - round with double in format_money function
Also implement some functions for the convenience of obtaining a rating
by its numerical value.
Visually, the dialog consists of widgets grouped into 4 tabs.  The
internal state is represented by a set of parameters sufficient to run
the game. Widgets do not exactly match parameters - there may be widgets
not represented by parameters, and parameters not shown as widgets, but
most parameters correspond to some widget.

Each parameter is an "instance" of some class, it can pull its data from
multiple sources (built-in start variant, savegame document), display an
interface for viewing/editing, validate it, and export the data to the
game's start routine.

Widgets have the property of blocking, by default editing most of the
parameters is not available. In order to unlock all parameters, there is
a 'Custom start' option, but it is disabled by default. When clicking on
the "New Game" button, if Ctrl is pressed, an additional option "Custom
start" will appear, unlocking all parameters.

In this commit, the dialog is only added, it is fully functional, but the
procedure for starting the game has not yet been implemented.
Add one argument to the game constructor so that it immediately creates
a ship of the correct type, otherwise there were some problems with
geoms. If not docked, the ship is spawned in a low circular orbit.

Remove the ability to edit the face and name of the player inside the
game. Slightly reduced the image in personal information. Do not create
a player character at game start if one has already been created.
Removed most "out of place" or trademarked names which readily associated with a fictional entity/character rather than a real name.
The focus is mostly on preserving the tone and context of the world of Pioneer rather than using 100% original names.
The criteria for remaining names is that they should not immediately stand out as belonging to a specific commercial work within the last ~50 years when combined with another name fragment.

Capitalized all name fragment lists, as the resulting generated word will be an english Proper Noun and should be capitalized.
@sturnclaw sturnclaw merged commit 6a904b6 into pioneerspacesim:master Aug 25, 2023
4 of 5 checks passed
@sturnclaw
Copy link
Member

Well done @Gliese852! Thank you for your patience and the large amount of work that went into this PR!

@Mc-Pain
Copy link
Contributor

Mc-Pain commented Aug 25, 2023

Saves after commit 032e80b are broken with following backtrace:

#0  0x00007ffff71f21d0 in ?? () from /lib64/libc.so.6
#1  0x000055555569727a in Space::UpdateBodies (this=this@entry=0x55555c66bab0) at /home/mcpain/pioneer/src/Space.cpp:1069
#2  0x0000555555698f8a in Space::~Space (this=this@entry=0x55555c66bab0, __in_chrg=<optimized out>) at /home/mcpain/pioneer/src/Space.cpp:314
#3  0x00005555555c8683 in std::default_delete<Space>::operator() (this=<optimized out>, __ptr=0x55555c66bab0) at /usr/include/c++/13/bits/unique_ptr.h:93
#4  std::default_delete<Space>::operator() (__ptr=0x55555c66bab0, this=<optimized out>) at /usr/include/c++/13/bits/unique_ptr.h:93
#5  std::unique_ptr<Space, std::default_delete<Space> >::~unique_ptr (this=0x55555c5a4d68, __in_chrg=<optimized out>) at /usr/include/c++/13/bits/unique_ptr.h:404
#6  Game::Game (this=<optimized out>, jsonObj=...) at /home/mcpain/pioneer/src/Game.cpp:187
#7  0x00005555557c5601 in Game::LoadGame (filename="test") at /home/mcpain/pioneer/src/Game.cpp:890
#8  0x00005555558d5210 in l_game_load_game (l=0x555555d66a10) at /home/mcpain/pioneer/src/lua/LuaGame.cpp:172
#9  0x0000555555943482 in luaD_precall (L=L@entry=0x555555d66a10, func=<optimized out>, func@entry=0x555556986b10, nresults=nresults@entry=0) at /home/mcpain/pioneer/contrib/lua/src/ldo.c:318
#10 0x00005555559520a3 in luaV_execute (L=L@entry=0x555555d66a10) at /home/mcpain/pioneer/contrib/lua/src/lvm.c:709
#11 0x00005555559437f8 in luaD_call (L=0x555555d66a10, func=<optimized out>, nResults=<optimized out>, allowyield=<optimized out>) at /home/mcpain/pioneer/contrib/lua/src/ldo.c:395
#12 0x0000555555942d6c in luaD_rawrunprotected (L=L@entry=0x555555d66a10, f=f@entry=0x555555939df0 <f_call>, ud=ud@entry=0x7fffffffd180) at /home/mcpain/pioneer/contrib/lua/src/ldo.c:131
#13 0x0000555555943a2f in luaD_pcall (L=L@entry=0x555555d66a10, func=func@entry=0x555555939df0 <f_call>, u=u@entry=0x7fffffffd180, old_top=352, ef=<optimized out>) at /home/mcpain/pioneer/contrib/lua/src/ldo.c:595
#14 0x000055555593c697 in lua_pcallk (L=0x555555d66a10, nargs=<optimized out>, nresults=1, errfunc=<optimized out>, ctx=<optimized out>, k=<optimized out>) at /home/mcpain/pioneer/contrib/lua/src/lapi.c:949
#15 0x0000555555938f0e in pi_lua_protected_call (L=L@entry=0x555555d66a10, nargs=nargs@entry=1, nresults=nresults@entry=1) at /home/mcpain/pioneer/src/lua/core/Sandbox.cpp:253
#16 0x0000555555717c1b in LuaTable::Call<bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, double> (key="mainMenu", this=0x7fffffffd230) at /home/mcpain/pioneer/src/lua/LuaTable.h:410
#17 PiGui::RunHandler (delta=0.016611980274319649, handler="mainMenu") at /home/mcpain/pioneer/src/pigui/LuaPiGui.cpp:85
#18 0x000055555564bd2f in MainMenu::Update (this=0x555555c6df20, deltaTime=0.0166119803) at /usr/include/c++/13/bits/basic_string.tcc:238
#19 0x00005555555fbe57 in Application::Run (this=0x555555c6dfd0) at /home/mcpain/pioneer/src/core/Application.cpp:192
#20 0x00005555555def72 in main (argc=<optimized out>, argv=<optimized out>) at /home/mcpain/pioneer/src/Pi.h:145

test.gz

@zonkmachine
Copy link
Member

@Gliese852 Brilliant new start screen!

I got this glitch. Sometimes the fields 'Name' and 'Cash' overlap. I haven't seen a pattern in this but it's happened twice.
overlap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants