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 4: support alpha-blending DrawingSurface operations #2661

Merged

Conversation

ivan-mogilko
Copy link
Contributor

@ivan-mogilko ivan-mogilko commented Jan 17, 2025

Resolve #1514

This lets DrawingSurface to make use of a alpha component in DrawingColor.

The implementation utilizes Allegro's drawing_mode that lets run a custom blender when drawing graphical primitives. DrawingSurface would check if the surface is 32-bit and assigned drawing color has non-opaque alpha component, in which case it sets "alpha blending mode". In this mode it will enable blending drawing before making a draw and disable after (so to not conflict with anything else drawn afterwards). Added internal StartDrawingWithBrush / EndDrawingWithBrush methods that handle that.

DrawPixel has its own way, where it switches between Bitmap::PutPixel and Bitmap::BlendPixel.
Bitmap::BlendPixel is the new Bitmap method I wrote, that basically runs a active blender func over a single pixel, and then writes result into bitmap.

For string drawing, I added alpha handling directly into each of the Font Renderers, because they might have their own ways of drawing (and if we set anything in engine, that won't affect plugin font renderers anyway).

Following is the current state of alpha blending support for DrawingSurface methods:

  • DrawingSurface.Clear - already works, because it plain copies color, nothing else necessary;
  • DrawingSurface.DrawCircle - works in this PR;
  • DrawingSurface.DrawLine - works in this PR;
  • DrawingSurface.DrawPixel - works in this PR;
  • DrawingSurface.DrawRectangle - works in this PR;
  • DrawingSurface.DrawTriangle - works in this PR;
  • DrawingSurface.DrawString - works in this PR;
  • DrawingSurface.DrawStringWrapped - works in this PR;

On a side note, I suppose we may want to add a separate DrawingSurface function for writing pixels, which ignores blending, and always directly sets color value.

@ivan-mogilko ivan-mogilko added this to the 4.0.0 (preliminary) milestone Jan 17, 2025
@ivan-mogilko ivan-mogilko marked this pull request as ready for review January 17, 2025 19:37
@ivan-mogilko ivan-mogilko force-pushed the ags4--drawingalphacol branch 2 times, most recently from 6b8ea3f to ab95107 Compare January 17, 2025 20:59
@ericoporto
Copy link
Member

This looks alright, and it appears it works. I also made a fix for the CI that should prevent the unrelated CI failure you had here.

I noticed that the blend mode settings is global, and not a set of that particular drawing surface, which is why I understood it requires the start and end brush calls wrapping things.

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Jan 18, 2025

I noticed that the blend mode settings is global, and not a set of that particular drawing surface, which is why I understood it requires the start and end brush calls wrapping things.

That is true, and that's the most annoying thing in Allegro 4. IMO the proper API would have a kind of a "drawing context" object, which holds all the drawing settings. Strangely, they still don't have that in Allegro 5 either, although, judging by the docs, they at least have separate blending settings per thread. In our case we cannot have any drawing operations on separate threads even if they are done on separate bitmaps, or we'd have to lock mutex everywhere any drawing mode is set. I believe that's the biggest reason to replace allegro 4 code with another drawing lib. (Of course there's also an option to adjust it ourselves, but idk how well worth that would be)

@ivan-mogilko ivan-mogilko force-pushed the ags4--drawingalphacol branch from ab95107 to 0565ff8 Compare January 18, 2025 18:51
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Jan 18, 2025

Rebased with some minor fixes.

I also tried to add BlendMode to drawing operations today, but it turned out that the existing software implementation of blend modes completely ignores destination alpha, and produces zero final alpha; so it sort of works in 24-bit mode. I cannot tell why it was done so, or how does it work with sprites... Perhaps @AlanDrake could shed some light on this?

In order to use blendmode when raw drawing, we'd need a proper 32-bit variants of all blenders, that combine both alphas (from src and dst), and add final alpha too. Original AGSBlend plugin was doing that (judging by its code).
I just want to be careful to not break sprite blending in software mode.

@AlanDrake
Copy link
Contributor

IIRC the alpha blender variants for blend modes did not exists in allegro and were not needed at the time, so I went ahead with the simplest case.

@ivan-mogilko
Copy link
Contributor Author

Hmm, I shall look into this separately then.

@ivan-mogilko ivan-mogilko force-pushed the ags4--drawingalphacol branch from 0565ff8 to bf19431 Compare January 18, 2025 21:09
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Jan 20, 2025

Alright, I will merge this, since this appears working.

@ivan-mogilko ivan-mogilko merged commit 4bbd138 into adventuregamestudio:ags4 Jan 20, 2025
21 checks passed
@ivan-mogilko ivan-mogilko deleted the ags4--drawingalphacol branch January 20, 2025 18:24
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Jan 23, 2025

@AlanDrake I'm investigating these blenders now, and it looks like they got something wrong.

Here where the final color is being combined:

ags/Common/gfx/blender.cpp

Lines 201 to 206 in c9367d9

const uint32_t src_alpha = geta32(x);
const uint32_t alphainv = 255 - src_alpha;
const uint32_t r = (getr24(blender_result)*src_alpha + alphainv*getr24(y)) / 255;
const uint32_t g = (getg24(blender_result)*src_alpha + alphainv*getg24(y)) / 255;
const uint32_t b = (getb24(blender_result)*src_alpha + alphainv*getb24(y)) / 255;
return r | g << 8 | b << 16;

The color shifts are opposite to the ones that AGS actually uses:

// RGB shifts for Allegro's pixel data
_rgb_r_shift_32 = 16;
_rgb_g_shift_32 = 8;
_rgb_b_shift_32 = 0;

Did anyone notice any wrong colors in blend results previously? I am not sure if these were properly tested.
Or perhaps nobody paid attention to how software blending works...

Hmmm, this might explain why my previous tests gave such weird results.

@AlanDrake
Copy link
Contributor

Allegro had these defs being assigned to int _rgb_b_shift_24 and similar

#define DEFAULT_RGB_R_SHIFT_24  0
#define DEFAULT_RGB_G_SHIFT_24  8
#define DEFAULT_RGB_B_SHIFT_24  16

But they would change through engine_setup_color_conversions() and become the opposite.

I suppose I should've paid more attention and used the makecol facilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants