game: auto-save pc-settings to user's home directory as well as memcard files #1233

xTVaser · 2022-03-14T06:22:41Z

This will automatically load the user's pc-settings if it is available from $HOME/OpenGOAL/jak1/settings/pc-settings.gc. If it's not there or it cannot be loaded, the defaults will be saved and used instead.

This also saves memcard files to $HOME/OpenGOAL/jak1/saves/*

Fixed this issue, leaving the write up here

There's one remaining issue here that im aware of:

If i set the game's aspect-ratio during boot as part of the pc-setting's load routine, i get a crash. If i run the same code later when the game is fully running, it's fine and works as expected. The current code sets it as part of the setting-control defaults, but these two files are tightly coupled when trying to load the pc-settings at boot as well, one has to come before the other:

pc-settings needs *setting-control* to be initialized so it can fix the hud layout (atleast for now)
setting-control needs pc-settings to be initialized to know which aspect-ratio setting to boot with
- this is why settings can't come last

This is how the original game changes the aspect ratio in the progress menu -

jak-project/goal_src/engine/ui/progress/progress.gc

Lines 1643 to 1645 in b2131d4

    
           (set! (-> *setting-control* default aspect-ratio) 
        
                 (the-as symbol (-> (the-as (pointer uint32) (-> s5-0 (-> obj option-index) value-to-modify)))) 
        
                 )

For clarification, im talking about running just this line at boot time, you can even add them to the bottom of settings.gc to reproduce:

(set! (-> *setting-control* default aspect-ratio) 'aspect16x9)

I can think of hack workarounds to skip things during the setup of the pc-settings global, but it feels like the better solution is to figure out why its crashing only at boot time.

The current stacktrace isnt very good since this is all triggered from top-level code

Read symbol table (411136 bytes, 2189 reads, 2188 symbols, 8.42 ms)      
rax: 0x000000000014f14c rcx: 0x3726de336f6e0000 rdx: 0x0000000000000009 rbx: 0x0000000000f63914
rsp: 0x0000018f1a254450 rbp: 0x0000018f1a254670 rsi: 0x000000003fe38e39 rdi: 0x0000000000f63914
 r8: 0x0000000000000000  r9: 0x000000000014f12c r10: 0x0000000000f63914 r11: 0x0001000000020001
r12: 0x0000000000174474 r13: 0x0000018f1db97ea4 r14: 0x0000000000147d24 r15: 0x0000018f1a0e0000
rip: 0x0000018f1b03a13d
In unknown code
  [0x18f1b03a11d] INVALID (0x1f)
  [0x18f1b03a11d] cld
  [0x18f1b03a11e] mov r9d, [r15+r9*1+0x78]
  [0x18f1b03a123] mov rdi, rbx
  [0x18f1b03a126] add r9, r15
  [0x18f1b03a129] call r9
  [0x18f1b03a12c] lea r9, [r14+0x7408]
  [0x18f1b03a134] mov r8d, [r15+r14*1+0x5DD8]
- [0x18f1b03a13c] mov [r15+r8*1+0x1F4], r9d
  [0x18f1b03a144] mov r9d, [r15+r14*1+0x5B60]
  [0x18f1b03a14c] mov esi, [r15+r9*1+0x0C]
  [0x18f1b03a151] mov r9d, [r15+rbx*1-0x04]
  [0x18f1b03a156] mov r9d, [r15+r9*1+0x70]
  [0x18f1b03a15b] mov rdi, rbx
  [0x18f1b03a15e] add r9, r15
  [0x18f1b03a161] call r9
  [0x18f1b03a164] jmp 0x0000018F1B03A212
  [0x18f1b03a169] lea r9, [r14+0x7430]
  [0x18f1b03a171] cmp rsi, r9
  [0x18f1b03a174] jnz 0x0000018F1B03A1BC
  [0x18f1b03a17a] INVALID (0xbe)
  [0x18f1b03a17a] INVALID (0x15)

Backtrace:
   rsp: 0x18f1a254450 (#x174450) rip: 0x18f1b03a13d (#xf5a13d)
Unknown Function at rip
...
gs> (:sym-name #x5dd8)
symbol name for symbol 5DD8h is *setting-control*
gs> (:sym-name #x5B60)
symbol name for symbol 5B60h is *pc-graphics-16x9-valid-resolutions*
gs> (:sym-name #x7408)
symbol name for symbol 7408h is aspect16x9

ManDude · 2022-03-14T06:43:07Z

The current stacktrace isnt very good since this is all triggered from top-level code

In the stacktrace, we can see R8 is zero. R8 is loaded with the value of *setting-control*, so *setting-control* here is zero (e.g. not loaded).

xTVaser · 2022-03-14T06:57:23Z

So that explains the problem, but how does it make sense that *setting-control* is zero when pc-kernel.gc builds after settings.gc now?

https://github.com/xTVaser/jak-project/blob/d98cf1ff2c5fe465c93d7157be5c8e2e644269b1/goal_src/game.gp#L1879

And inside settings.gc it initializes it if it's zero

https://github.com/xTVaser/jak-project/blob/d98cf1ff2c5fe465c93d7157be5c8e2e644269b1/goal_src/engine/game/settings.gc#L410-L411

I am fairly certain this is the entrypoint of the crash, but ill have to triple check something else isn't happening first... Is there something else im misunderstanding?

https://github.com/xTVaser/jak-project/blob/d98cf1ff2c5fe465c93d7157be5c8e2e644269b1/goal_src/pc/pckernel.gc#L836

ManDude · 2022-03-14T06:59:10Z

The build order is not the same as the load order. The DGO description files have the list of files in the order they are loaded.

xTVaser · 2022-03-14T07:29:45Z

ah! kk ill look into that, didnt know about those. Thanks

…pect ratio

water111

Looks good to me, just a few questions (nothing that needs changing if you don't want). It's probably a good idea to have @ManDude look over some of the pckernel/memory card saving stuff, but it all looks good to me.

water111 · 2022-03-18T01:49:14Z

common/util/FileUtil.cpp

+  // the project path should be explicitly provided by whatever if needed
+  // TEMP HACK
+  // - if the provided path is absolute, don't add the project path
+  if (input.size() == 1 && std::filesystem::path(input.at(0)).is_absolute()) {


should we make this a different function, instead of having it do different things with absolute and relative paths?

I'm not opposed to this (it makes sense to let absolute paths be absolute paths), but I'm just wondering.

So the main reason I had to make this change is this method is used by the file-stream stuff:

jak-project/game/sce/sif_ee.cpp

Lines 100 to 102 in da00b1f

s32 sceOpen(const char* filename, s32 flag) {

FILE* fp = nullptr;

auto name = file_util::get_file_path({filename});

I think long-term yeah we probably want a more explicit function like get_project_file_path and that function linked above should not be assuming everything is within the project directory. I think we can do a lot of cleanup / simplification, but im not sure how much we want to do immediately.

common/util/FileUtil.cpp

game/kernel/kmachine.cpp

ManDude · 2022-03-18T22:04:58Z

game/kernel/kmemcard.cpp

-    "/BASCUS-97124AYBABTU!",           "/BASCUS-97124AYBABTU!/icon.sys",
-    "/BASCUS-97124AYBABTU!/icon.ico",  "/BASCUS-97124AYBABTU!/BASCUS-97124AYBABTU!",
-    "/BASCUS-97124AYBABTU!/bank0.bin", "/BASCUS-97124AYBABTU!/bank1.bin",
-    "/BASCUS-97124AYBABTU!/bank2.bin", "/BASCUS-97124AYBABTU!/bank3.bin",
-    "/BASCUS-97124AYBABTU!/bank4.bin", "/BASCUS-97124AYBABTU!/bank5.bin",
-    "/BASCUS-97124AYBABTU!/bank6.bin", "/BASCUS-97124AYBABTU!/bank7.bin"};
+    "BASCUS-97124AYBABTU!",           "BASCUS-97124AYBABTU!/icon.sys",
+    "BASCUS-97124AYBABTU!/icon.ico",  "BASCUS-97124AYBABTU!/BASCUS-97124AYBABTU!",
+    "BASCUS-97124AYBABTU!/bank0.bin", "BASCUS-97124AYBABTU!/bank1.bin",
+    "BASCUS-97124AYBABTU!/bank2.bin", "BASCUS-97124AYBABTU!/bank3.bin",
+    "BASCUS-97124AYBABTU!/bank4.bin", "BASCUS-97124AYBABTU!/bank5.bin",
+    "BASCUS-97124AYBABTU!/bank6.bin", "BASCUS-97124AYBABTU!/bank7.bin"};


Does the leading backslash cause issues? Don't really think it's a big deal to change these strings, but it would be nice if our filepath stuff could handle redundant slashes properly.

When being combined with std::filesystem it's a problem because you end up with a path with two slashes. String concatenation of course avoids this (i think that's how the old code worked). We could add extra code and steps to handle duplicate slashes but, we should probably just be constructing paths correctly and not have to go through another abstraction layer.

goal_src/build/game_dgos.gc

ManDude · 2022-03-18T22:08:50Z

goal_src/pc/pckernel-h.gc

+(define *pc-temp-string-1* (new 'global 'string 2048 (the-as string #f)))
+


Can't use the other *pc-temp-string*?

No, I use it to construct a string that is passed to a function that then uses *pc-temp-string* (i make the filepath that is passed to the read/write function). I could be wrong and there is a better way? but, one of the problems with depending on these global variables to do things :(

ok, it's fine then

ManDude · 2022-03-18T22:09:55Z

goal_src/pc/pckernel.gc

+  (clear *pc-temp-string-1*)
+  (format *pc-temp-string-1* "~S/pc-settings.gc" *pc-settings-folder*)
+  (write-to-file obj *pc-temp-string-1*)
+  (clear *pc-temp-string-1*)


Look at the string-format macro in gstring.gc, it simplifies doing this. You can make an alternate version of it that uses that string instead of the usual *temp-string*.

From what I can tell it only really saves on the calls to clear? I'd rather just keeps things explicit, it's not obvious that macro uses a particular global (from the caller) and the pattern seems unsustainable

You would have to just write (write-to-file obj (pc-string-format "~S/pc-settings.gc" *pc-settings-folder*)) instead of those 4 lines.

I understand that it saves lines, that isn't what I'm opinionated against.

Well, the global only exists to be used a temporary string in the first place, so I don't think there's an issue.

Someone could get into the same issue I did and it wouldn't be obvious -- they call a function using the output of pc-string-format and within the function it also uses pc-string-format. It's not obvious what's going on unless you expand the macro. I just prefer to be more explicit.

ManDude · 2022-03-18T22:12:16Z

goal_src/pc/pckernel.gc

+          (return obj)
+          (begin
+            (format 0 "[PC] PC Settings found at '~S' but could not be loaded, re-creating with defaults!~%" *pc-temp-string-1*)
+            (reset obj))))


hmm yes that function should probably be altered to not overwrite the settings in case an error occurred, I'll have to fix that later.

I think resetting the settings back to defaults when they are not valid is sensible -- we aren't wiping save-file data or something important. It's better imo than crashing.

goal_src/pc/engine/ui/progress/progress-draw.gc

goal_src/game.gp

goal_src/pc/pckernel.gc

ManDude · 2022-03-18T22:27:28Z

goal_src/pc/pckernel.gc

+    ;; persist reset / new settings if we made it this far
+    (pc-mkdir-path *pc-temp-string-1*)
+    (commit-to-file obj)
+    (clear *pc-temp-string-1*)


And this - is it just writing the settings after they've been reset? Probably not super useful (reading the default file and not reading a file at all is the exact same thing), and personally I'd rather it not write things at start-up in case things go wrong.

water111

thanks!

xTVaser added 6 commits March 14, 2022 00:20

cmake: reduce warning spam especially from libs

e65d27d

runtime: add FS helper functions

06a2b52

game: save/restore pc-settings | add original aspect option

df9b95f

game: overwrite unloadable settings with defaults

f2bc872

temp: unable to set the games aspect-ratio in the boot else crash?

2178916

runtime: save memcard files to user directory as well

d98cf1f

xTVaser added 3 commits March 14, 2022 19:54

runtime: fix pckernel load order which resolves setting the orig as…

14ed38c

…pect ratio

lint: format

8ebcb36

cmake: revert warning suppression, it's just causing problems it seems

2c244da

xTVaser marked this pull request as ready for review March 15, 2022 00:43

fix the order of the rest of pckernel and creation of obj file paths

0cdb0b0

xTVaser force-pushed the v/autosave-settings branch from 4aecd77 to 0cdb0b0 Compare March 15, 2022 01:08

xTVaser added 2 commits March 17, 2022 00:21

Merge branch 'master' into v/autosave-settings

ca68ec0

lint: formatting

44660c3

water111 reviewed Mar 18, 2022

View reviewed changes

ManDude reviewed Mar 18, 2022

View reviewed changes

game: don't save settings on startup even if they are corrupted

4190cf5

water111 approved these changes Mar 21, 2022

View reviewed changes

water111 merged commit 014da6f into open-goal:master Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

game: auto-save pc-settings to user's home directory as well as memcard files #1233

game: auto-save pc-settings to user's home directory as well as memcard files #1233

xTVaser commented Mar 14, 2022 •

edited

Loading

ManDude commented Mar 14, 2022

xTVaser commented Mar 14, 2022 •

edited

Loading

ManDude commented Mar 14, 2022

xTVaser commented Mar 14, 2022

water111 left a comment

water111 Mar 18, 2022

xTVaser Mar 18, 2022

ManDude Mar 18, 2022

xTVaser Mar 19, 2022 •

edited

Loading

ManDude Mar 18, 2022

xTVaser Mar 19, 2022 •

edited

Loading

ManDude Mar 20, 2022

ManDude Mar 18, 2022

xTVaser Mar 20, 2022 •

edited

Loading

ManDude Mar 20, 2022

xTVaser Mar 20, 2022

ManDude Mar 20, 2022

xTVaser Mar 20, 2022

ManDude Mar 18, 2022

xTVaser Mar 20, 2022

ManDude Mar 18, 2022

water111 left a comment

	(set! (-> setting-control default aspect-ratio)
	(the-as symbol (-> (the-as (pointer uint32) (-> s5-0 (-> obj option-index) value-to-modify))))
	)

	s32 sceOpen(const char* filename, s32 flag) {
	FILE* fp = nullptr;
	auto name = file_util::get_file_path({filename});

		(define pc-temp-string-1 (new 'global 'string 2048 (the-as string #f)))

game: auto-save pc-settings to user's home directory as well as memcard files #1233

game: auto-save pc-settings to user's home directory as well as memcard files #1233

Conversation

xTVaser commented Mar 14, 2022 • edited Loading

ManDude commented Mar 14, 2022

xTVaser commented Mar 14, 2022 • edited Loading

ManDude commented Mar 14, 2022

xTVaser commented Mar 14, 2022

water111 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xTVaser Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xTVaser Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xTVaser Mar 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

water111 left a comment

Choose a reason for hiding this comment

xTVaser commented Mar 14, 2022 •

edited

Loading

xTVaser commented Mar 14, 2022 •

edited

Loading

xTVaser Mar 19, 2022 •

edited

Loading

xTVaser Mar 19, 2022 •

edited

Loading

xTVaser Mar 20, 2022 •

edited

Loading