Devilutionx: mouse cursor flickering in caves

[Chance4us]

The mouse cursor is flickering permanently in the lava levels and the "Poisoned Water Supply" quest.

[ghost]

Yes , Source Code deleted reported this to me.

I will look into it. I think the upstream might have a fix for this.

[ghost]

@AJenbo do we know where the code that might concern this might be located? I wonder if there is some rendering issue. Ironically, this is only an issue in DevilutionX .

[AJenbo]

I believe this issue might have been resolved, possibly by cleaned up render.
But also dx.cpp is still heavily stubbed so cleaning it might solve the problem.
If that doesn't do it then probably by the time we add hardware acceleration it should go away.

[MaxDesiatov]

I don't think this is fixed upstream, even after render.cpp cleanup. I've done a clean rebuild from 82adedb which has the most fresh upstream changes and this is still reproducible for me on a 32-bit build on macOS 10.14.4.

[AJenbo]

Your right and not in the https://github.com/diasurgical/devilutionX/tree/render branch either :(

That leaves cleaning up dx.cpp and rewriting the render as possibly "free" fixes.

[AJenbo]

I have identified the issue as being related to color cycling. If disabled the mouse will stop flickering, but then you won't have animated lava.

[Chance4us]

Issue is being directly or indirectly affected by following source file / functions:
devilutionX/Source/palette.cpp
function1: void __cdecl palette_update_caves() -> color cycling for lava
function2: void __fastcall palette_update_quest_palette(int n) -> Poisoned Water Supply Quest -> change the color of poisoned water to clean blue water
I tried to comment out the content of both functions with the result as color cycling would be disabled and the flickering is gone.

[Chance4us]

I think this issue is almost solved.
Please try to replace "sdl_update_entire_surface();" by "memcpy(logical_palette, orig_palette, 1024);" in the file devilutionX/SourceX/storm/storm_dx.cpp
By only this change the flickering is gone completely. Unfortunately the main menu and the cutscenes are no more visible at all.

[Chance4us]

There is no need for "sdl_update_entire_surface();" or "memcpy(logical_palette, orig_palette, 1024);" at all. I'll try to workaround the SDL_BlitSurface(pal_surface, &src_rect, surface, NULL) causing flickering in devilutionX/SourceX/dx.cpp

[Chance4us]

Please check failed pull request #86. Maybe the code has to be a little bit modified.

[AJenbo]

Thanks for the PR, although it fails on the build servers, it defiantly points us in the direction of the issue. I strongly belive that this issue will be dealt with once we have dx.cpp unstubbed. I have already started this work but I'm currently away on holiday, I expect to resume (and probably finish) that work next week and if this dosen't solve it I will target the issue directly based on your findings. Once it is fixed I think it would be fitting with a new release, especially once all the upstream work is synced.

[AJenbo]

After cleaning up dx.cpp this is the conclusion I have come to.

Diablo originally rendered to a back buffer that did not contain any color information, whenever it had completed the rendering of a frame it would blit the updated parts to the framebuffer in the graphics card. The framebuffer would also have the color pallet injected, at this could be updated regardless of where in the rendering process the next frame where since this was happening in the off-screen back buffer.

The way DevilutionX currently handles things is so having the color information associated with the back buffer, the blitting is then done to a 32bit buffer (converting the colors in the process) this is done so that it is more convenient to display on modern systems via SDL/OpenGL. This is also fine, the problem occurs when the palette is changed during the rendering of a frame; since we cannot change the palette of the 32bit image (because it is full color) we apply the new palette to the back buffer and copy the new result, this can then result in an unfinished frame being presented on screen.

The solution I'm planning is to add an additional 8bit buffer where the color change can safely be performed on the latest completed image, and then do the conversion from there.

An alternate solution would be to make the pipeline 8bit, but it's not where we want to head in the future (full 32bit pipeline). The extra 500kb copy pr frame also shouldn't be much of an issue for modern systems. An alternative solution would be to make sure that all palette updates happens at the end of a frame rendering, but I would prefer not changing any parts of the engine for now.

[ghost]

For what it's worth, Diablo II handled Direct Draw by using two different buffers. Essentially one is just temporary and like Diablo 1, 8-bit with the palettes. This then gets copied to the front buffer which is 32-bit and is what is shown on screen.

[AJenbo]

Testing @Chance4us's workaround it appears to solve the issue satisfactorily with no noticeable side effects so going to go with it for now. Basically, it defers the updating of the pallet until the next frame is rendered (would probably not show until then anyway), except for when doing fade in/out in the menus.