Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AGS: Commits from upstream up to version #4767

wants to merge 78 commits into from


Copy link

@tag2015 tag2015 commented Mar 4, 2023

This PR brings ScummVM's AGS implementation up to date with version 3.6.41,
It includes fixes for priority errors reported in Kathy Rain, Blackwell Convergence,
Resonance, Kill Yourself and probably others.


Engine: fixed TOUCH hints breaking compilation against older SDL2
We don't use touch-to-mouse emulation

Common: add bmp and pcx load and save from stream
Upstream changed the routine for loading/saving bmp/pcx files from in-game.
Skipped because it required significant changes.
If needed, this could be probably reimplemented better

Allegro4: removed bmp and pcx load/save code
related to 41eb660d599e94af31bfeb086392fce5cf1e1116

Allegro4: removed normal packfile implementation + unused datafile
related to 41eb660d599e94af31bfeb086392fce5cf1e1116

fix Makefile-defs.allegro
related to 41eb660d599e94af31bfeb086392fce5cf1e1116

Common: in bitmap loader replaced WRITE3BYTES with a Memory function
related to 41eb660d599e94af31bfeb086392fce5cf1e1116

Common: fixed Bitmap's SaveToFile not testing an opened stream
related to 41eb660d599e94af31bfeb086392fce5cf1e1116

Allegro: remove bitmap bank stuff
Not present

Direct3D: use glm instead of the local helper methods
Different implementation

Common: Fixed new[]/delete mistmatch
Already fixed

Common: Fix comment doesn't match macro name
Already fixed

Allegro: additional cblit cleanup
Not in scummvm

Direct3D, OpenGL: replace viewport hacks with proper transform matrix
This would require changes, and I'm not sure it's needed in ScummVM's implementation

Direct3D: support sprite batches rendering to a texture
Couldn't get the shared part to compile and it wouldn't do anything anyway

OGL: support sprite batches rendering to a texture
Related to 490522fdd412dcdb3c687f88143a62eb935de11a

Engine: render GUI & controls batch to a buffer texture
Related to 490522fdd412dcdb3c687f88143a62eb935de11a
(but may need to be reimplemented)

Direct3D: implement proper blend op settings for a custom render target
Different implementation, may need to be addressed later

OpenGL: implement proper blend op settings for a custom render target
Related to c91d3e70ceb9fdd0874dde80d806d8b927302bdd

Engine: release render targets before switching the gfx mode
Related to c91d3e70ceb9fdd0874dde80d806d8b927302bdd

OpenGL: fixed custom render targets in "screen resolution" mode
Not present

OpenGL: work with render targets of various sizes
Related to commit 3f086f1ef91d37b1d90208b798adf9a9b884a06e

Direct3D: work with render targets of various sizes
Related to commit 3f086f1ef91d37b1d90208b798adf9a9b884a06e

Direct3D: fixed rendering at screen resolution (when not stretched)
Different implementation

Engine: update render target if the GUI got resized
Related to commit 3f086f1ef91d37b1d90208b798adf9a9b884a06e

OpenGL: removed redundant error reporting
Not present in scummvm

Engine: create raw draw screens only on plugin's demand
Different implementation

Engine: fixed return value type in DoNullSpriteCallback()
Fixed in previous commit

Engine: use unique_ptr in VideoMemoryGraphicsDriver::ScreenFx
Couldn't get it to work with scummvm's implementation

OpenGL: removed incorrect redeclaration of _rendSpriteBatch
Not present

Direct3D, OpenGL: fixed a silly rounding mistake in _renderSprite
Not present

Direct3D, OpenGL: removed few redundant casts from float to float
Not present

Engine: csci gui messages uses intptr_t insted of long
Better solution already implemented

This supposedly should slightly improve bitmap to texture conversion performance.
From upstream ab213c0962df7bdec96ce38122f637f0febe4ef4
uses new char[] on Create method.
From upstream ecfc238401183e210a1773c27724516f74272c8e
This also fixes an allocation/deallocation mismatch found in the code.
From upstream b47a086cbd589cd271c5fe1b3497f9b8e4ae4abf
from upstream 4d9f1788059b2fdadd3e957a5a260a918458595d
- prefer empty instead of size() == 0
partially from upstream de97ab3e8c14074944c7087cc8e4bc5b15d6f4d6
from upstream 8d0e0cb3c20db0beddffc36c688ed33005a43d7c
- using braced initializers for returned invalid objects
partially from upstream 8aeda4b67bf24ce080b09255139e0f6b907a9a8f

from upstream 126a5e4c97581aabadbbd4410441b83f371e18ea
Completes commit 8e77b04062d865e35bbadedce2fb7ba4971b837c.
We don't support subdirs in saves, the previous change causes some
games to be unable to save/restore

from upstream 27306da4cd0c1fa193f984644aa1915c8a944364
… only)

Partially from upstream 1f867858c46c8a2dde841022aed8631f1deeabb1
From upstream bbf48a8deeaefb0c577426d3a745261831d67be7

Leave it with a new name as `update_polled_stuff()` only in functions
related to the game update.
From upstream 95dc139f51e704da19792e5480e69bbfe7ec27aa
from upstream a4a98adfcb039e0f8f213d60e8b0b639b0217f9b
This is forbidden in the latest editor, but seems to be a possibility in
older editors (2.*).
From upstream c1ba79d0cdb31b7704f889e953865fca4b175076
This complements / fixes previous fix attempt 08aef03
From upstream 95c99fc325f8550f220e7a0f67d6555bb23c8c5a
From upstream ca6620951ec013e3627d962701c3cc25c1ac8adb
Reimplemented from upstream 96d52b258dd226bc8c2243c1a5bc95a9ceeecd2a
From upstream c9ce4cfc457713abadc6a1a4cfb4b47aa0667382

Complements / reimplements f310a08
From upstream b0e2fe40c1d73674a7b868a87152be6029df6cac
From upstream cf31083808f19ae314212e7b451709cfd5d6f05d
From upstream 894ec8a85bd2c78438c8bfabee632b26f5323b07
From upstream 849a0f3153511087bd93c4d7d1ba898d4ab28cfa
From upstream c1afae64fe348c64ee4a8c8888349ae267241395
From upstream a06d97dc88887395f3f961ab8e3fbda277733795

I'm baffled how I did not notice this behavior earlier, but engine was
created at least 2 redundant intermediate bitmaps meant to cache
transformed sprites per each object and character on screen for
HW-accelerated renderers, - which they do not need, as they don't
use software transformations.

The resulting effect won't be too notable for low-res games with low
amount of simultaneous objects, but may be quite significant for the
high-res games with multiple large objects in the room.

From upstream cca997fe83ec5f7014677a69095a7c9ec1494cd9
This somewhat reverts 2b9f130 , while keeping tidier code and safer args.

Switching from lzwexpand_to_mem to using streams only caused speed
regression, which became notable for hi-res games with LZW sprite
compression (a 3.6.0 feature).
For this revert we instead load compressed data into a buffer in whole,
then pass two memory buffer pointers to lzw_expand, where it reads
and writes memory directly.
Testing a high-res (1920x1080) game with lots of sprite streaming
showed speed improvement of this function by around 200%.
The downside is the temporary buffer for reading compressed
data too.
In theory, same could be done for lzwcompress afterwards.

From upstream 6959004676916568c6fca210702d669f61741e13
The example when this could happen:
    Parser.Said("climb,get in bed");
where "climb", "get" and "bed" are dictionary words.

From upstream b633ca77076137e2402faa903f5b6fbcd3f4c3bc
From upstream c4dce7eb4a0ab4d2440a8b7e637ea8a7f9ed2c00
From upstream d60022c715f15d0454ffe04d4ce8d331c34e3045
From upstream 872598fea699341c7de82484b43678d91dcc425e
From upstream 416c456db0b20f5896506f86525faec0a353893f
From upstream 43c5560bd6d2f28a31ec402081c7fa7c5bbf78de
Was broken by fcc873e
From upstream ff210b2ce50cea0e903cd1d09fa7ad1734a862b0
…ed game

From upstream d5ded80f8219b1f04e484c369d647bbc9ed8edd8
From upstream b2dc19eef19f1498e4510de5672cb65c281d33d4

From upstream 662318d0dc1e877a334e91b1a964a41de2b46573

In ALSoftwareGraphicsDriver:
* Save index of a currently rendered sprite batch (or none, if not inside
a render pass).
* In SetMemoryBackBuffer() don't reset virtual screen subbitmaps
  unless we are outside render pass.
* In InitSpriteBatch() also test if batch's surface is subbitmap to the
  current virtual screen (in case one was replaced, but batch's surface

Partially from upstream 5dd078961020da6478347dac01dea729471333d1
…ole VS

When plugin calls SetVirtualScreen (for software renderer), don't
replace whole virtual screen, instead replace only the current
render stage buffer.
This is complementary to the older changes in software renderer,
made during development of AGS 3.5.0, which featured advanced
room viewports and cameras. Since that change, separate render
stages could draw on sub-bitmaps of smaller size.

While IAGSEngine::GetVirtualScreen() was adjusted to follow that
change, and return not the whole virtual screen, but only a "stage
buffer" (which may be a VS sub-bitmap, or an intermediate bitmap
created specifically for this render stage), SetVirtualScreen() was
NOT adjusted accordingly and kept replacing whole VS. As a
result, this discrepancy could cause logical errors, as well as
crashes with plugins that use this API (e.g. original SnowRain

The immediate reason of error is that plugin would remember a
pointer returned from GetVirtualScreen (which is a stage buffer),
and then try to set it back with SetVirtualScreen. As a result,
engine's own stage buffer is assigned as a full virtual screen.

From upstream 09143ea7fbf8474f78932116ae1c81ceff4de95a
From upstream dcec61ba9825be21a0d2009a73b2af24df6a912d
…d game

From upstream 0f19fc3d2b9efbc4eb809032bcb7d60b652e1150

From upstream 84417e8c73815bcd35f4a6812d6f118cfb3acacb

This allows to properly fixup (upgrade) RoomStatus structs
restored from a older format save in an upgraded game.
For example: game was upgraded from 3.4.0 to 3.6.0, but player
tries loading 3.4.0 save. In this situation the engine must know and
keep track of what version each RoomStatus was saved in.
This is necessary, because RoomStatus can only be updated upon
loading a corresponding room file (when entering a room). This
means that multiple RoomStatus objects will be staying in memory
not being upgraded yet, until player enters particular room.

From upstream 205232af5909e0a257494ff404f36003b004ebc4
From upstream b8de51fedf0318e8243e1fc3872c61f366a23144
NOTE: the code theoretically allows any object to be sorted this
way, but the problem is, the ID sequences are not shared, and
multiple object types may have same IDs.
This is something to consider in the future.
Partially from upstream 4404881c6a49e3a74f316998ee46368b00895fc7
From upstream 51a7e64efd628e62c73b1b1b8011d22ec83b0c38
From upstream 1cccdfea5c1009123de7ba25be72b2757f74d06f
Partially from upstream 81b5fbc917d83a02cebc152969fd769b2b7b9b15
@tag2015 tag2015 requested a review from criezy March 4, 2023 16:31
Copy link

digitall commented Mar 5, 2023

@tag2015 : Thanks for doing this. I was looking at it, but it is quite a task to review through the upstream commits and integrate, so kudos.

@criezy : Once this is integrated, we will need to review the open AGS bugs to see which are still present and which are fixed by the upstream patches. If you want help with that, let me know.

Copy link
Contributor Author

tag2015 commented Mar 5, 2023

@tag2015 : Thanks for doing this. I was looking at it, but it is quite a task to review through the upstream commits and integrate, so kudos.

@criezy : Once this is integrated, we will need to review the open AGS bugs to see which are still present and which are fixed by the upstream patches. If you want help with that, let me know.

Well, I can comment in advance on some of the bugs, from what I have tested, as I have most of the games (including the commercial ones except lancelot's hangover):
Issues #13724 #13725 #13758 13780 #14197 (except the "image editor" which is a recent upstream fix) should be fixed.
Priority issues are also fixed in Zniw's Adventure #13412
The font-related issues are still present (such as the "squashed" letters that get reported all the time)
The Excavation of Hob's Barrow still crashes.

Copy link

i30817 commented Mar 5, 2023

Incredible work. Thank you for keeping it up to date.

Copy link

digitall commented Mar 5, 2023

@tag2015 : Thanks for confirming those. I suspected we can probably close a fair few after confirming the fix... and I suspected that my current "favourite" bug i.e. Hob's Barrow at would still be broken. I think that is an issue in the allegro interface code so different to upstream. It may be present upstream, but a latent issue. However, still haven't got to the bottom of why that happens. Any help gratefully received.

Copy link

digitall commented Mar 7, 2023

@criezy : Do you have time to review this soon or are you happy as-is? Just that we have a bunch of AGS bugs which could probably get closed once this is merged :)

Copy link

criezy commented Mar 7, 2023

@criezy : Do you have time to review this soon or are you happy as-is? Just that we have a bunch of AGS bugs which could probably get closed once this is merged :)

Yes, I have some time, but there are a lot of commits to review, so it will take a few more days 😉

Copy link

digitall commented Mar 8, 2023

@criezy : No problem. I did suspect you were doing it, but just wanting to try to stop users reporting bugs which upstream have fixed ... and allow me to finish my backlog of AGS games in my game library cough :)

One note, this PR is now in the "This branch cannot be rebased due to too many changes" state. It will likely need you to merge it manually locally as I think the Github UI can't cope with the master changes which may overlap :|

Copy link

criezy commented Mar 12, 2023

Great job.
This has now been merged manually.

@criezy criezy closed this Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
4 participants