reyandme / kam_remake

"KaM Remake" is an RTS game remake written in Delphi from scratch.
http://www.kamremake.com
GNU Affero General Public License v3.0
122 stars 29 forks source link

make KM_WorkerThread and its tasks more flexible #183

Open dpronin opened 8 months ago

dpronin commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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