search

tonihele / OpenKeeper

Dungeon Keeper II remake
GNU General Public License v3.0
437 stars 43 forks source link

Multithreading objects access #307

Open ArchDemons opened 6 years ago

ArchDemons commented 6 years ago

I noticed strange behavior of objects and began to understand why this happens. It turned out that objects are not passed by reference, but copied (cached) in different threads. We do not have enough synchronization blocks and use the keyword volatile.

Even in the code it would be nice to mark somehow not thread-safe methods. Or annotation or something else. We suffer from errors in rendering.

tonihele commented 6 years ago

New or old? Meaning master or feature-263? And welcome back :)

ArchDemons commented 6 years ago

I tested only the master branch. ;-)

tonihele commented 6 years ago

Ok, that makes sense then. feature-263 uses ES design on the entities and a library that should allow this [concurrent access]. Also everything related to rendering is completely isolated from the game logic. This long overdue separation should remedy this. At least completely eliminate the "already rendered but state changed" fatal errors. Of course it is entirely possible to introduce other concurrency issues on the game logic side, but I have (almost) kept the logic in one thread.

tonihele commented 6 years ago

On networked game though... The player actions are all over the threads. But I've set locks on building and gold management stuff. At least tried to remember this.

ArchDemons commented 6 years ago

And then it became funny when the button was assigned a handler with the sound "FE BUTTON BIG 5.mp2", and at the moment when the sound should play, there is already a handler with the sound "FE BUTTON SMALL.mp2". This is our main menu

tonihele commented 6 years ago

Hmm :) Is this a bug in Nifty or our code?

tonihele commented 6 years ago

We also nowadays have a Discord channel! ;)

ArchDemons commented 6 years ago

I think we shoud not initialize states in other threads. Loading shoud push new states in manager and wait when states are initialized. Or volatility and synchronization, if it helps us

tonihele commented 6 years ago

Yeah, I think I broke it some more when I optimized it a bit, or rather fixed another random bug that happened due to, yep, race condition. Should really think that whole thing over at some point.