SKY refactoring

In my first post I've wrote about refactoring of SKY engine. Here I want to describe it in more details what did I mean.



Let's look to the main engine's loop. It's huge, so I'll process it part by part

First thing we're seeing - nested pause loop. Are we really need it? Or may remove without any regrets? Obviously it's paused state of engine. When engine in this state, it's only make delay and check keyboard for 'P' button pressed. (Also there is one moment. In this state user can't close game cause shouldQuit() procedure doesn't processed. I don't know actually is it bug or feature, but seems like bug)

1. while (!shouldQuit()) {
2.         _debugger->onFrame();
3.         if (shouldPerformAutoSave(_lastSaveTime)) {
4.                 if (_skyControl->loadSaveAllowed()) {
5.                         _lastSaveTime = _system->getMillis();
6.                         _skyControl->doAutoSave();
7.                 } else
8.                         _lastSaveTime += 30 * 1000; // try again in 30 secs
9.         }
10.         _skySound->checkFxQueue();
11.         _skyMouse->mouseEngine();
12.         handleKey();
13.         if (_systemVars.paused) {
14.                 do {
15.                         _system->updateScreen();
16.                         delay(50);
17.                         handleKey();
18.                 } while (_systemVars.paused);
19.                 delayCount = _system->getMillis();
20.         }

After I removed nested loop, code begin to show that the pause is a state of loop. Also removing of totally useless loop helped us to make code more readable

1. while (!shouldQuit()) {
2.         if (_systemVars.paused) {
3.                 _system->updateScreen();
4.                 delay(50);
5.                 handleKey();
6.                 delayCount = _system->getMillis();
7.         }
8.         else {
9.                 _debugger->onFrame();
10.                 if (shouldPerformAutoSave(_lastSaveTime)) {
11.                         if (_skyControl->loadSaveAllowed()) {
12.                                 _lastSaveTime = _system->getMillis();
13.                                 _skyControl->doAutoSave();
14.                         } else
15.                                 _lastSaveTime += 30 * 1000; // try again in 30 secs
16.                 }

Now let's pay our attention to the bottom of go() function. There is calculation of delay

1. if (_fastMode & 2)
2.         delay(0);
3. else if (_fastMode & 1)
4.         delay(10);
5. else {
6.         delayCount += _systemVars.gameSpeed;
7.         int needDelay = delayCount - (int)_system->getMillis();
8.         if ((needDelay < 0) || (needDelay > _systemVars.gameSpeed)) {
9.                 needDelay = 0;
10.                 delayCount = _system->getMillis();
11.         }
12.         delay(needDelay);
13. }

I want to move it to separate function because it make reading of main loop much harder than it can be.

1. int SkyEngine::calculateDelay( uint32 &delayCount ) {
2.         if (_fastMode & 2) {
3.                 return 0;
4.         }
5.         else if (_fastMode & 1) {
6.                 return 10;
7.         }
8.         else {
9.                 delayCount += _systemVars.gameSpeed;
10.                 int needDelay = delayCount - (int)_system->getMillis();
11.                 if ((needDelay < 0) || (needDelay > _systemVars.gameSpeed)) {
12.                         needDelay = 0;
13.                         delayCount = _system->getMillis();
14.                 }
15.                 return needDelay;
16.         }
17. }

In main loop I've left only delay (calculateDelay(delayCount)) string. I don't actually know, why delayCount uses in pause state, so I did't touch it. 

Next code I want to extract from main loop is 

1. if (shouldPerformAutoSave(_lastSaveTime)) {
2.         if (_skyControl->loadSaveAllowed()) {
3.                 _lastSaveTime = _system->getMillis();
4.                 _skyControl->doAutoSave();
5.         } else
6.                 _lastSaveTime += 30 * 1000; // try again in 30 secs
7. }

It's absolutely independent and we may extract it into separate method

1. void SkyEngine::performAutosave() {
2.         if (shouldPerformAutoSave(_lastSaveTime)) {
3.                 if (_skyControl->loadSaveAllowed()) {
4.                         _lastSaveTime = _system->getMillis();
5.                         _skyControl->doAutoSave();
6.                 } else
7.                         _lastSaveTime += 30 * 1000; // try again in 30 secs
8.         }
9. }

At the same time i've noticed that acutosaving doesn't perform in pause state (bug?) 

For the same reasons, I've extracted this code

1. if (_debugger->showGrid()) {
2.         uint8 *grid = _skyLogic->_skyGrid->giveGrid(Logic::_scriptVariables[SCREEN]);
3.         if (grid) {
4.                 _skyScreen->showGrid(grid);
5.                 _skyScreen->forceRefresh();
6.         }
7. }

to separate method 

1. void SkyEngine::showGrid() {
2.         if (_debugger->showGrid()) {
3.                 uint8 *grid = _skyLogic->_skyGrid->giveGrid(Logic::_scriptVariables[SCREEN]);
4.                 if (grid) {
5.                         _skyScreen->showGrid(grid);
6.                         _skyScreen->forceRefresh();
7.                 }
8.         }
9. }

in result of all these actions I got clean and readable main loop. 

1. while (!shouldQuit()) {
2.         if (_systemVars.paused) {
3.                 _system->updateScreen();
4.                 delay(50);
5.                 handleKey();
6.                 delayCount = _system->getMillis();
7.         }
8.         else {
9.                 _debugger->onFrame();
10.                 performAutosave();
11.                 _skySound->checkFxQueue();
12.                 _skyMouse->mouseEngine();
13.                 handleKey();
14.                 _skyLogic->engine();
15.                 _skyScreen->processSequence();
16.                 _skyScreen->recreate();
17.                 _skyScreen->spriteEngine();
18.                 showGrid();
19.                 _skyScreen->flip();                            
20.                 delay(calculateDelay(delayCount));                     
21.         }
22. }

But it's not the end. I still can make few improvements here. Each state call delay and handleKey functions. I think, I can move them out of condition. But to do this, I need to change calculateDelay function and add those strings to it:

1. if (_systemVars.paused) {
2.         delayCount = _system->getMillis();
3.         return 50;
4. }

Paused state calls _system->updateScreen() method. After few ticks function delay calls the same method. I not sure, but it seems like we can remove this call from paused state.  Aad we get situation where engine doesnt perform any actions in paused state. I've changed condition and main loop got following look:

1. while (!shouldQuit()) {
2.         if (!_systemVars.paused) {
3.                 _debugger->onFrame();
4.                 _skySound->checkFxQueue();
5.                 _skyMouse->mouseEngine();
6.                 _skyLogic->engine();
7.                 _skyScreen->processSequence();
8.                 _skyScreen->recreate();
9.                 _skyScreen->spriteEngine();
10.                 showGrid();
11.                 _skyScreen->flip();                                                   
12.         }
13.         performAutosave();
14.         delay(calculateDelay(delayCount));
15.         handleKey();
16. }

In my opinion, it much more readable and clear than it was. We can move forward and make it FSM-like and remove delay function and one more loop, but I don't think we realy need to do this. 

Now, let's look to the delay() function.

1. void SkyEngine::delay(int32 amount) {
2.         Common::Event event;
4.         uint32 start = _system->getMillis();
5.         _keyPressed.reset();
7.         if (amount < 0)
8.                 amount = 0;
9.        
10.         do {
11.                 while (_eventMan->pollEvent(event)) {
12.                         switch (event.type) {
13.                         case Common::EVENT_KEYDOWN:
14.                                 _keyPressed = event.kbd;
15.                                 break;
16.                         case Common::EVENT_MOUSEMOVE:
17.                                 if (!(_systemVars.systemFlags & SF_MOUSE_LOCKED))
18.                                         _skyMouse->mouseMoved(event.mouse.x, event.mouse.y);
19.                                 break;
20.                         case Common::EVENT_LBUTTONDOWN:
21.                                 if (!(_systemVars.systemFlags & SF_MOUSE_LOCKED))
22.                                         _skyMouse->mouseMoved(event.mouse.x, event.mouse.y);
23.                                 _skyMouse->buttonPressed(2);
24.                                 break;
25.                         case Common::EVENT_RBUTTONDOWN:
26.                                 if (!(_systemVars.systemFlags & SF_MOUSE_LOCKED))
27.                                         _skyMouse->mouseMoved(event.mouse.x, event.mouse.y);
28.                                 _skyMouse->buttonPressed(1);
29.                                 break;
30.                         default:
31.                                 break;
32.                         }
33.                 }
35.                 _system->updateScreen();
37.                 if (amount > 0)
38.                         _system->delayMillis((amount > 10) ? 10 : amount);
40.         } while (_system->getMillis() < start + amount);
41. }

There are 2 loops in delay() function. They make logicaly unrelated to each other actions. Outer loop makes delay, inner loop processes events. I think we need extract events processing to separate function (processEvents). This will make code more redable and willn't change logic. Delay funtion became more understandable, cause programmer don't need to filter event processing by his mind, he's just seeing "processEvents" word and understands what's going on. So, after extraction there are 2 functions:

1. void SkyEngine::delay(int32 amount) {
2.       uint32 start = _system->getMillis();
3.       _keyPressed.reset();
5.       if (amount < 0)
6.             amount = 0;
8.       do {
9.             processEvents();
10.             _system->updateScreen();
11.             if (amount > 0)
12.                   _system->delayMillis((amount > 10) ? 10 : amount);
14.       } while (_system->getMillis() < start + amount);
15. }
17. void SkyEngine::processEvents() {
18.       Common::Event event;
19.       while (_eventMan->pollEvent(event)) {
20.             switch (event.type) {
21.             case Common::EVENT_KEYDOWN:
22.                   _keyPressed = event.kbd;
23.             break;
24.             case Common::EVENT_MOUSEMOVE:
25.                   if (!(_systemVars.systemFlags & SF_MOUSE_LOCKED))
26.                         _skyMouse->mouseMoved(event.mouse.x, event.mouse.y);
27.                   break;
28.             case Common::EVENT_LBUTTONDOWN:
29.                   if (!(_systemVars.systemFlags & SF_MOUSE_LOCKED))
30.                         _skyMouse->mouseMoved(event.mouse.x, event.mouse.y);
31.                   _skyMouse->buttonPressed(2);
32.                   break;
33.             case Common::EVENT_RBUTTONDOWN:
34.                   if (!(_systemVars.systemFlags & SF_MOUSE_LOCKED))
35.                         _skyMouse->mouseMoved(event.mouse.x, event.mouse.y);
36.                   _skyMouse->buttonPressed(1);
37.                   break;
38.             default:
39.                   break;
40.             }
41.       }
42. }