ULTIMA8: Merge RenderSurface and SoftRenderSurface

[OMGPizzaGuy]

Hi all! I'm doing these changes through a pull request to get feedback on them.
My aim is to simplify custom rendering in the engine and possibly eliminate methods that could be handled by ManagedSurface.
Should I flatten the render_surface.inl into the paint shape methods?
ShapeFrame owns a height, width, and pixels: should these be put in a Graphics::Surface before the custom blit or stored as a surface in ShapeFrame directly?
Should I consider forcing the bpp on RenderSurface rather than letting it be configured by Ultima8Engine::GraphicSysInit?

Any other suggestions?

[sev-]

ShapeFrame owns a height, width, and pixels: should these be put in a Graphics::Surface before the custom blit or stored as a surface in ShapeFrame directly?
I'd store Surface internally.

Also, these BlendedLogic functions are suspiciously similar to the newly introduced ManagedSurface::blendBlitTo(), the former TransparentSurface code.

Should I consider forcing the bpp on RenderSurface rather than letting it be configured by Ultima8Engine::GraphicSysInit?

Yes, this is always the best, as it simplifies the code significantly. Especially in your case, when there is only one game/family of games based on the engine, e.g. you know that some alien format will not arrive anyway.

[OMGPizzaGuy]

ShapeFrame owns a height, width, and pixels: should these be put in a Graphics::Surface before the custom blit or stored as a surface in ShapeFrame directly?
I'd store Surface internally.

Sounds good I'll rebase this branch and start down that path.

Also, these BlendedLogic functions are suspiciously similar to the newly introduced ManagedSurface::blendBlitTo(), the former TransparentSurface code.

Indeed! Watching work on that inspired me to cycle back around to this as I wasn't sure how to tackle this previously.

Should I consider forcing the bpp on RenderSurface rather than letting it be configured by Ultima8Engine::GraphicSysInit?

Yes, this is always the best, as it simplifies the code significantly. Especially in your case, when there is only one game/family of games based on the engine, e.g. you know that some alien format will not arrive anyway.

Alright, I think I embrace the BlendBlit PixelFormat for this engine so I might be able to use the new code there.

Thanks!

[ccawley2011]

Should I consider forcing the bpp on RenderSurface rather than letting it be configured by Ultima8Engine::GraphicSysInit?

Yes, this is always the best, as it simplifies the code significantly. Especially in your case, when there is only one game/family of games based on the engine, e.g. you know that some alien format will not arrive anyway.

I should point out that there's usually a trade-off between simplicity and compatibility here since not all backends support the pixel format required by the common blending routines, and some of the ones that do perform an additional conversion pass in software that may introduce additional overhead on lower end machines.

I'd be interested to know what the Ultima8 engine is using alpha blending for - I wouldn't have expected a game from 1994-1996 to need it.

[OMGPizzaGuy]

I should point out that there's usually a trade-off between simplicity and compatibility here since not all backends support the pixel format required by the common blending routines, and some of the ones that do perform an additional conversion pass in software that may introduce additional overhead on lower end machines.

Understood. I'm curious to know how well this engine performs on lower end as well, but have yet to check. There's a config key "bpp" currently to set 16 or 32 to see how much that pass would effect those machines.

I'd be interested to know what the Ultima8 engine is using alpha blending for - I wouldn't have expected a game from 1994-1996 to need it.

Dragging items did a mask and blend in the original game to produce a light grey outline of the item. Street lights on ground , smoke effects from chimneys, and black potions for invisibility have similar blends

Pentagram enhanced these as a full blending, which I think is a bit nicer. Also, there's a few extras for "Show Highlight Items" and "Show Touching Items" that blend colors - although those could be potentially handled by a modified pallete for those items.

[ccawley2011]

Understood. I'm curious to know how well this engine performs on lower end as well, but have yet to check. There's a config key "bpp" currently to set 16 or 32 to see how much that pass would effect those machines.

What I'd suggest if possible is to select a format based on what the backend reports as supported. If the engine specifically requires RGB565 or RGBA8888, you can pass an array with those formats as the third parameter to initGraphics, while if it'll work with any 16bpp or 32bpp format, you can pass a null pointer instead and you'll get whatever true colour format the backend reports as being available and fast (with palette mode as a last resort).

Dragging items did a mask and blend in the original game to produce a light grey outline of the item. Street lights on ground , smoke effects from chimneys, and black potions for invisibility have similar blends

Pentagram enhanced these as a full blending, which I think is a bit nicer. Also, there's a few extras for "Show Highlight Items" and "Show Touching Items" that blend colors - although those could be potentially handled by a modified pallete for those items.

That sounds like a neat enhancement. Does support for the original mask/blend effects exist in the current code, or is it just the enhanced version?

[ccawley2011]

Am I correct in thinking that this is essentially the same as Graphics::Surface::setAlpha() except with an additional Rect parameter?

[OMGPizzaGuy]

I believe you are correct. It was used in Pentagram for a two pass rendering of TextWidget for the normal scaled paint and then the composited unscaled paint to render a higher res font.

I've been maintaining this method and testing with CHECK_ALPHA_FILLS, although it's currently not used as the composited painting was abandoned although not completely removed when ported to ScummVM.

[OMGPizzaGuy]

What I'd suggest if possible is to select a format based on what the backend reports as supported. If the engine specifically requires RGB565 or RGBA8888, you can pass an array with those formats as the third parameter to initGraphics, while if it'll work with any 16bpp or 32bpp format, you can pass a null pointer instead and you'll get whatever true colour format the backend reports as being available and fast (with palette mode as a last resort).

I'll keep this in mind.

That sounds like a neat enhancement. Does support for the original mask/blend effects exist in the current code, or is it just the enhanced version?

Nope, never implemented an original version option.

[OMGPizzaGuy]

I think additional changes I'd like to make are starting to fall out of scope for this pull request. I'll hold off on those for the moment. Any additional review is appreciated.

Currently the benchmarks show a small improvement on my machine:
RenderSurface::benchmark 1 4 1000000
branch: master - 6c74678
[2023-08-28 18:15:28] Paint: 4444
[2023-08-28 18:15:32] PaintNoClip: 3737
[2023-08-28 18:15:37] PaintTranslucent: 4884
[2023-08-28 18:15:42] PaintMirrored: 4702
[2023-08-28 18:16:58] PaintInvisible: 76257
[2023-08-28 18:17:47] PaintHighlight: 49131
[2023-08-28 18:19:02] PaintHighlightInvis: 75299

PR #5288
[2023-09-02 11:46:32] Paint: 3843
[2023-09-02 11:46:32] PaintNoClip: N/A
[2023-09-02 11:46:36] PaintTranslucent: 4384
[2023-09-02 11:46:41] PaintMirrored: 4113
[2023-09-02 11:47:57] PaintInvisible: 76145
[2023-09-02 11:48:44] PaintHighlight: 47362
[2023-09-02 11:49:59] PaintHighlightInvis: 75079

[mduggan]

Thanks for doing the cleanup! overall it LGTM - there are further improvements to be made but this should make them a lot easier!

[OMGPizzaGuy]

I'll go ahead and complete this PR as I have a number of follow-up changes ready at this point.