SDL: Fix shaking with SDL1 and negative offsets #4754
Conversation
|
Note that the shake function is documented as taking only positive offsets: So passing negative offsets would be an error in the engine. That being said, we should indeed avoid a crash. I am unsure about supporting this in some backends though. This may give the impression that negative offsets are supported, but the engine will then break on some other platform. Maybe it would be better to add a sanity check in the implementation of setShakePos to ignore negative offsets and print a warning? |
This happens in the scumm engine on the day of tentacle on the scene with loud music which is unavoidable for walkthrough. Reading the code I see that scumm uses negative offsets: see method ScummEngine::updateScreenShakeEffect. On the other hand only 2 engines use negative shakes: SCUMM and KYRA |
This appears to be a fairly recent change from commit 33d1804. However it is not clear from the commit message and comment added in the code if changing from positive to negative offsets was intended. @athrxx can probably comment. We could also decide to change the documentation to accept negative offsets if that is needed by some engines. But then we need to also modify all porters as the SDL1 backend may not be the only one not supporting it. |
|
That change to negative offsets was actually intended, since that matches the shaking style of the original code (these offsets that get written into the crt controller registers of the VGA hardware will move the screen up, not down). (Just to make it more clear: the original code does write positive offsets, but they do have the opposite effect) I only ever tested SDL2. I wasn't even aware that SDL1 was still much of a thing for us. With this fix we should probably update the documentation for that method to accept negative offsets. |
Current code results in a crash
| SDL_Rect blackrect = {0, 0, (Uint16)(_videoMode.screenWidth * _videoMode.scaleFactor), (Uint16)(_videoMode.screenHeight * _videoMode.scaleFactor)}; | ||
|
|
||
| if (_videoMode.aspectRatioCorrection && !_overlayInGUI) | ||
| if (_gameScreenShakeXOffset >= 0) | ||
| blackrect.w = (Uint16)(_gameScreenShakeXOffset * _videoMode.scaleFactor); | ||
| else { | ||
| blackrect.w = ((-_gameScreenShakeXOffset) * _videoMode.scaleFactor); | ||
| blackrect.x = ((_videoMode.screenWidth + _gameScreenShakeXOffset) * _videoMode.scaleFactor); | ||
| } |
There was a problem hiding this comment.
A bit of nitpicking, but this whole block could be simplified as follow:
SDL_Rect blackrect = {0, 0, (Uint16)(ABS(_gameScreenShakeXOffset) * _videoMode.scaleFactor), (Uint16)(_videoMode.screenHeight * _videoMode.scaleFactor)};
if (_gameScreenShakeXOffset < 0)
blackrect.x = ((_videoMode.screenWidth + _gameScreenShakeXOffset) * _videoMode.scaleFactor);| SDL_Rect blackrect = {0, 0, (Uint16)(_videoMode.screenWidth * _videoMode.scaleFactor), (Uint16)(_videoMode.screenHeight * _videoMode.scaleFactor)}; | ||
|
|
||
| if (_videoMode.aspectRatioCorrection && !_overlayInGUI) | ||
| blackrect.h = real2Aspect(blackrect.h - 1) + 1; | ||
| if (_gameScreenShakeYOffset >= 0) | ||
| blackrect.h = (Uint16)(_gameScreenShakeYOffset * _videoMode.scaleFactor); | ||
| else { | ||
| blackrect.h = ((-_gameScreenShakeYOffset) * _videoMode.scaleFactor); | ||
| blackrect.y = ((_videoMode.screenHeight + _gameScreenShakeYOffset) * _videoMode.scaleFactor); | ||
| } |
There was a problem hiding this comment.
A similar simplification to what suggested above for the vertical black rect could also be applied here.
| else | ||
| src_y += -_currentShakeYOffset; | ||
|
|
||
| if (dst_x < width && dst_y < height) { |
There was a problem hiding this comment.
| if (dst_x < width && dst_y < height) { | |
| if (dst_x < width && dst_y < height && src_x < width && src_y < height) { |
4 participants
Current code results in a crash