Skip to content

make KM_WorkerThread and its tasks more flexible#183

Open
dpronin wants to merge 1 commit into
reyandme:masterfrom
dpronin:thread_worker/polymorphic-tasks
Open

make KM_WorkerThread and its tasks more flexible#183
dpronin wants to merge 1 commit into
reyandme:masterfrom
dpronin:thread_worker/polymorphic-tasks

Conversation

@dpronin
Copy link
Copy Markdown

@dpronin dpronin commented Jan 6, 2024

It is more convenient to use polymorphism when wanting to execute some task within a worker thread without having to specify a concrete type of a task to be scheduled
This approach unties hands and makes a caller (a consumer of a worker thread) be responsible for defining a concrete type with merely one requirement of exec() virtual function to be implemented

Please, consider this approach and merge it in if there are no questions

PS: this approach also will help me port this code over FPC easier

@dpronin dpronin changed the title make KM_WorkerThread and their tasks more flexible make KM_WorkerThread and its tasks more flexible Jan 6, 2024
@dpronin dpronin force-pushed the thread_worker/polymorphic-tasks branch 2 times, most recently from cd9da71 to 389a729 Compare January 7, 2024 21:45
@dpronin
Copy link
Copy Markdown
Author

dpronin commented Jan 8, 2024

How could it be verified in the game? I think if worker thread unit is a heart of the system we will definitely see some issues if any right upon starting the game, won't we?
I presume that there will be no issues at all

@reyandme
Copy link
Copy Markdown
Owner

reyandme commented Jan 8, 2024

Are you able to verify it now?
Extensive testing is required before adding these types of changes. I am not able to do that atm

@dpronin
Copy link
Copy Markdown
Author

dpronin commented Jan 8, 2024

Are you able to verify it now? Extensive testing is required before adding these types of changes. I am not able to do that atm

If I had Windows OS I would test it

I do not think it requires extensive testing, however the more the better

Actually, this project demands automated testing

@sado1
Copy link
Copy Markdown

sado1 commented Jan 9, 2024

Testing under Wine is not an option, since Delphi IDE does not work there (at least last time I checked)
But I am able check such changes on my Windows PC.

I do not understand the MR well (again, sorry, not a programmer), what is the likely risk here from game logic point of view? I understand that this touches worker threads (used only in few places in Remake like save handling, loading the assets when a game is started, was there anything more?), and makes them use a polymorphic function instead of a couple statically typed ones.

If this is a change, that effectively needs good testing from the players themselves at some point, and does not introduce big benefits, I would recommend to introduce it only after next release. But I lack enough details to understand the impact.
I'll try in coming days to create an issue describing current project state in details, and the way forward, so you (and anyone else interested to contribute) are aware of current project status.
In short, we lack a programmer to help fixing a few last bugs, before current beta could be released officially, so we're sort-of in code freeze state for bigger changes.

I'm not sure if Rey told you that already, but in the past, a big risk to introduce hard to debug bugs, was anything that touched multiplayer game behavior, in a way that would desynchronize the game's state. With such bugs, game for different players would run game logic commands in different order, which would change entropy for random functions, and eventually lead up to a desync.

To give you an example, I remember one bug was something like: when one player makes autosaves every X minutes, but another player makes autosaves every Y minutes, then the game state became desynchronized in savefiles.

@dpronin
Copy link
Copy Markdown
Author

dpronin commented Jan 9, 2024

@sado1 there are two aspects of these changes that might or might not affect the game. The first one is business logic that is preserved, so it means that the game is hopefully not affected. The second one is architecture, which is about programming, not business logic of the game, and the architecture is improved so that tasks scheduled within a worker thread might be arbitrary and it is decided by a call site, not the worker thread itself, and this approach is the only one true in OOP according to SOLID (I am talking here about D meaning Dependency Inversion)

If we like to test these changes we need to test features related to autosaving and saving games, also exporting different kind of resources

As a programmer I naturally would want to see them in the master branch because it will definitely ease my programming process and clean the code

Signed-off-by: Denis Pronin <dannftk@yandex.ru>
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.

3 participants