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

Improvement - move calls to Game.SaveGameStats into Jobs #5929

Closed
mwerle opened this issue Oct 6, 2024 · 7 comments · Fixed by #5934
Closed

Improvement - move calls to Game.SaveGameStats into Jobs #5929

mwerle opened this issue Oct 6, 2024 · 7 comments · Fixed by #5934

Comments

@mwerle
Copy link
Contributor

mwerle commented Oct 6, 2024

Follow-up to #5927; details in this discussion.


Web-eWorks:

Something you might consider for a follow-on PR - data/pigui/modules/saveloadgame.lua calls Game.SaveGameStats repeatedly to load details about savefiles on disk. That function should* be able to be moved off-thread using the Job system (finalizing the job and delivering the result via LuaEvent on the main thread) so there isn't a perceptual hitch/stutter when enumerating details about saved games.

That's the equivalent of throwing you in the deep end to see if you can swim, so feel free to demur and I'll tackle it when I get time.

= I've not rigorously analyzed it to ensure it's free of dependencies against the main thread - that would be part of the work involved in moving it to an asynchronous job.
@mwerle
Copy link
Contributor Author

mwerle commented Oct 9, 2024

So I was going to yell for help, but I managed to work it out. Possibly more convolutedly than required..

My draft implementation can be found in #5934

In this implementation, the individual stutters are gone, instead there is one huge long stutter, and the interface updates all at once instead of entry by entry as currently. Perhaps I misunderstood and the Lua Events should be triggered from inside each job, but I have no idea how threadsafe the C++ / Lua interface is..

@mwerle
Copy link
Contributor Author

mwerle commented Oct 9, 2024

...and a very quick test shows that no, it is not threadsafe and blows up.

@syonfox
Copy link

syonfox commented Oct 9, 2024

Yeah current master branch crashes when I go to the system view.

@syonfox
Copy link

syonfox commented Oct 9, 2024

Info: Created shader geosphere_terrain (address=0x562de39d9cd0)
Info: Created shader rayleigh_fast (address=0x562de32d44a0)
Fatal: [T] pigui/modules/system-view-ui.lua:419: attempt to call method 'GetSystemSelectionMode' (a nil value)
stack traceback:
[T] pigui/modules/system-view-ui.lua:419: in function 'Show'
[T] pigui/libs/window-layout.lua:85: in function 'fun'
[T] pigui/libs/wrappers.lua:129: in function 'window'
[T] pigui/libs/window-layout.lua:84: in function 'fun'
[T] pigui/libs/wrappers.lua:662: in function 'withFont'
[T] pigui/libs/window-layout.lua:80: in function 'display'
[T] pigui/modules/system-view-ui.lua:909: in function 'fun'
[T] pigui/libs/wrappers.lua:129: in function 'window'
[T] pigui/baseui.lua:52: in function <[T] pigui/baseui.lua:49>

@mwerle
Copy link
Contributor Author

mwerle commented Oct 9, 2024

Yeah current master branch crashes when I go to the system view.

Info: Created shader geosphere_terrain (address=0x562de39d9cd0) Info: Created shader rayleigh_fast (address=0x562de32d44a0) Fatal: [T] pigui/modules/system-view-ui.lua:419: attempt to call method 'GetSystemSelectionMode' (a nil value)

... that's completely unrelated to this work (but the crash does look like it's related to my other PR #5922) ..

I can't repro this crash (master on commit 770ed87 - 20 or so hours ago)

@syonfox
Copy link

syonfox commented Oct 9, 2024

sorry it seems to work now maybe was a save game bug or something after recompiling it seems to be resolved on a new game.

@mwerle
Copy link
Contributor Author

mwerle commented Oct 9, 2024

sorry it seems to work now maybe was a save game bug or something after recompiling it seems to be resolved on a new game.

No worries; but please do not comment in completely unrelated issues...

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 a pull request may close this issue.

2 participants