Add alpha support to Framebuffer by benjhar · Pull Request #1 · boppofun/rust_boppo_core

benjhar · 2026-05-08T15:05:48Z

This PR adds support for alpha channels to our Framebuffer. It does so by making Framebuffer generic over C: Color, and adding an implementation of Color for RGBA.

There are some elements of this PR that warrant further discussion:

Implementing Color for f32 or u8, and/or other scalar types would enable use of grayscale Framebuffers. These could be useful for keeping track of alpha or brightness separately, or for use as a "mask".
The implementation, as it is, would allow us to vectorise most operations but would make it difficult to vectorise Framebuffer<RGBA>::alpha_blend and any operations that might seek to target only one channel, although we don't implement any such operations currently.
Color defines a blend function, which takes a rhs_percent parameter. The use of this parameter (or rather, how to use it properly) for blending RGBA values is not clear - RGBA values can be blended entirely using their alpha values. Do we just ignore those and have blend work consistently for both RGB and RGBA?

CC: @benhansen-io @tiesselune

benhansen-io

This will be nice to have. Still some high level questions but looking good overall.

benjhar · 2026-05-11T11:58:15Z

Updated the PR comment with another question that came up.

benhansen-io · 2026-05-11T13:31:30Z

Implementing Color for f32 or u8, and/or other scalar types would enable use of grayscale Framebuffers. These could be useful for keeping track of alpha or brightness separately, or for use as a "mask"

This is nice we can do it but I don't have any use case right now and can't think of any coming up so I would lean toward holding off for now. If we do end up wanting it, I think I would like to wrap a u8 in a Grayscale struct and impl Color on that instead of on u8 directly since it won't be common.

The implementation, as it is, would allow us to vectorise most operations but would make it difficult to vectorise Framebuffer::alpha_blend and any operations that might seek to target only one channel, although we don't implement any such operations currently.

Thats fine for now

Color defines a blend function, which takes a rhs_percent parameter. The use of this parameter (or rather, how to use it properly) for blending RGBA values is not clear - RGBA values can be blended entirely using their alpha values. Do we just ignore those and have blend work consistently for both RGB and RGBA?

Good point. I agree its confusing so I would prefer to not have it. How about removing it from Color and only implementing a free function blend(a: RGB, b: RGB) then we don't have to answer the ambiguous question for RGBA. That or maybe we rename it average and we take the component average of all components including alpha but I don't know when that would be useful. I am inclined to say lets not implement it for RGBA.

benhansen-io · 2026-05-11T13:33:35Z

Does dim_to also feel weird for RGBA. I feel like my natural instinct would be that it would adjust the alpha channel but thats also weird.

benjhar · 2026-05-11T13:58:37Z

I think I'd prefer renaming blend to average, over removing it as a method. At the moment it is just a component average, and so is dim_to with OFF. I think this is fine if we document it - for me the problem was that it is unclear, and for proper alpha blending we can have a separate alpha_blend(a: RGBA, b: RGBA) function.
I think it probably depends on which we think will be more common, RGB component-average, or RGBA alpha blending.

benhansen-io · 2026-05-11T14:00:02Z

                1. - ((relative_dist - FALL_OFF_DISTANCE) / (1.0 - FALL_OFF_DISTANCE))
            };
-            let this_color = color::dim_to(c, percent_colored);
+            let this_color = c.dim_to(percent_colored);


So in my mind ideally we should be able to have a Framebuffer and write multiple draw calls to it and the falloff should use alpha and blend with the existing values. Otherwise it feels weird that we accept a writeable Framebuffer instead of just returning one.

Oh and this dim_to definitely feels like it should be alpha based for an RGBA buffer.

I don't have graphics API experience. I spent a bit of time asking Claude trying to figure out what is common here. Claude seems to indicate that they have configurable blend states that you can set globally or per pipeline depending on the API. Thats probably overkill for us, but we could pass in an optional blend fn pointer at some point so the user gets full control of how the existing pixel and drawed pictures get blended.

I really want to move these drawing functions to be a builder pattern and have fall_off be an option on the builder but I think that should be a separate PR and doesn't have to be you implementing it. So maybe we get these draw_* functions compatible with the current situation and then come back and improve them.

So in my mind ideally we should be able to have a Framebuffer and write multiple draw calls to it and the falloff
should use alpha and blend with the existing values.

Oh and this dim_to definitely feels like it should be alpha based for an RGBA buffer.

Yeah this does sound much nicer than what we have at the moment. I'll work on making it so, hopefully without breaking anything. Agreed about dim_to.

I don't have graphics API experience.

I'm in the same boat. I agree that the approach Claude mentioned seems like a bit much. The optional function pointer would work but I also sort of don't like it. I can't think of a better option though.

Adding @tiesselune to this review since he has game dev experience in case he has any thoughts

The optional function pointer would work but I also sort of don't like it

I've come around on this - I think this should be fine, and it means we don't need two separate functions for scalar and "vector" blends and dims. Although, I don't think it needs to be optional - we can just pass it always and users can do |_| 0.5 if they want to just to scalar weighting

So in my mind ideally we should be able to have a Framebuffer and write multiple draw calls to it and the falloff should use alpha and blend with the existing values. Otherwise it feels weird that we accept a writeable Framebuffer instead of just returning one.

@benhansen-io In your mind is it worth breaking these lights_plane functions for RGB to be able to implement this? I'm really struggling for a neat way to do this generically. The best way I can think of, so far, is to return to_rgb_u8 to Color and also add an alpha(&self) -> Option<u8> to Color. I can't think of a way that doesn't require adding extra functionality to Color.

I've come around on this - I think this should be fine, and it means we don't need two separate functions for scalar and "vector" blends and dims. Although, I don't think it needs to be optional - we can just pass it always and users can do |_| 0.5 if they want to just to scalar weighting

What does |_| 0.5 represent. I thought the blending function would take two colors but I haven't throught through it all.

In your mind is it worth breaking these lights_plane functions for RGB to be able to implement this? I'm really struggling for a neat way to do this generically. The best way I can think of, so far, is to return to_rgb_u8 to Color and also add an alpha(&self) -> Option to Color. I can't think of a way that doesn't require adding extra functionality to Color.

The dim_to you have right now seems pretty close. The only thing wrong is it doesn't seem to blend with the current values in the framebuffer. For RGB it seems like instead of assuming we dim to black we should blend with the current color (which allows painting a black circle on a colored background for example) and for RGBA we should alpha blend the existing color and the new color (otherwise drawing red circle with falloff onto a green blackground would produce some black around the edges which doesn't make sense)

benhansen-io · 2026-05-13T09:23:52Z

Let me know when this is ready for another review.

benhansen-io

Seems like we are getting pretty close. This will be a nice change to have.

Marking request changes so its clear when you want me to look at it again.

benhansen-io · 2026-05-21T04:33:55Z

                1. - ((relative_dist - FALL_OFF_DISTANCE) / (1.0 - FALL_OFF_DISTANCE))
            };
-            let this_color = color::dim_to(c, percent_colored);
+            let this_color = c.dim_to(percent_colored);


I've come around on this - I think this should be fine, and it means we don't need two separate functions for scalar and "vector" blends and dims. Although, I don't think it needs to be optional - we can just pass it always and users can do |_| 0.5 if they want to just to scalar weighting

What does |_| 0.5 represent. I thought the blending function would take two colors but I haven't throught through it all.

In your mind is it worth breaking these lights_plane functions for RGB to be able to implement this? I'm really struggling for a neat way to do this generically. The best way I can think of, so far, is to return to_rgb_u8 to Color and also add an alpha(&self) -> Option to Color. I can't think of a way that doesn't require adding extra functionality to Color.

The dim_to you have right now seems pretty close. The only thing wrong is it doesn't seem to blend with the current values in the framebuffer. For RGB it seems like instead of assuming we dim to black we should blend with the current color (which allows painting a black circle on a colored background for example) and for RGBA we should alpha blend the existing color and the new color (otherwise drawing red circle with falloff onto a green blackground would produce some black around the edges which doesn't make sense)

benhansen-io · 2026-05-21T04:36:31Z

-pub struct Framebuffer {
+#[derive(Clone, PartialEq)]
+#[must_use]
+pub struct Framebuffer<C: Color = RGB> {


What do you think about a FramebufferRGBA type alias for Framebuffer::<RGBA>?

Yeah that sounds useful

benjhar · 2026-05-25T16:56:13Z

What does |_| 0.5 represent. I thought the blending function would take two colors but I haven't throught through it all.

|_| 0.5 would be the component average of both colours, with the function argument being the light index. I think taking both colours is better although it would make it more verbose.

For RGB it seems like instead of assuming we dim to black we should blend with the current color

I think this might be unintuitive, since dim_to only acts on self and just takes the percent as a parameter. This does sound useful though, what do you think about a separate function that does this more explicitly, like blend_with_current or light_blend. I think this would, internally, just be a call to weighted_average using the current lights as the rhs argument.

…pecialised impls

Leaving the others in framebuffer.rs as those make code clearer

…confusion.

benjhar · 2026-05-31T20:49:35Z

@benhansen-io I thought a bit more about this:

For RGB it seems like instead of assuming we dim to black we should blend with the current color

This would be fairly trivial if we used RGBA in MainFramebuffer instead of RGB. The memory overhead would be fairly small and we could use defaults for RGB flush. What do you think?

benhansen-io

I am a little lost on this PR if its for feedback or for merging. When requesting a review its helpful if you tell me if its ready to merge or if you are looking for feedback and if your looking for feedback what items you want feedback on. If its ready to merge, please include all breaking changes and how we know they don't break activities and what tests were done to ensure that.

I like the direction this is going but I feel like this is too big with breaking changes to merge. I think we should try to merge the a minimal subset of this that makes no breaking changes and add in commits from there. Also sometimes I find it really helpful to make cleanup commits before making a change commit.

I also think handling RGB and RGBA in a generic context is going to be more confusing than its worth. I think Framebuffer being generic is fine for all of the set functions (its an assigmment). I think most of the current flush functions should only be implemented for Framebuffer.

benhansen-io · 2026-06-11T10:22:00Z

-    }
-
+#[allow(
+    clippy::cast_possible_truncation,


Going forward lets not put allow statements for pedantic clippy lints. They clutter up the code and are not run be default. For example there are more lines of code for this allow statement than the function definition and body.

benhansen-io · 2026-06-11T10:30:13Z

+
+    /// Blend two colors together, according to their brightness or alpha.
+    #[must_use]
+    fn blend(self, rhs: Self) -> Self;


We renamed blend to weighted_average and then put a new definition of blend. Do you think this definition will work and will be better for all of our current usage sites or were you planning on renaming the current usage sites to weighted average?

Is blending based on brightness a common operation? I don't have any intuition for it.When I asked Gemini how to mix two colors from 0 to 100% it only provided the weighted average option.

benhansen-io · 2026-06-11T10:35:15Z

+            r: self.r,
+            g: self.g,
+            b: self.b,
+            a: mix_component(self.a, 0, percent),


This doesn't feel like a "dim". Its a dim if the background is black but who is to say that the background is black. Now that the flush for rgba is flushing with the current colors the background could be any color.

I am inclined to say we should define dim_to only in terms of RGB. In fact maybe we should only have this operation on RGB.

benhansen-io · 2026-06-11T10:36:04Z


    fn luminance(self) -> f32 {
-        brightness(self)
+        const MAX_MAGNITUDE_SQUARED: f32 = 255.0 * 255.0 * 3.0;


Lets add tests for this and other functions that are easily testable in this PR.

benhansen-io · 2026-06-11T10:39:30Z

+    /// Flush this buffer to lights, blending with the current state according to `blend_fn`.
+    /// `blend_fn` is passed the buffer's light color, the current light color, and the light index,
+    /// in that order.
+    pub fn flush_blend<W: Fn(RGB, RGB, usize) -> RGB>(&self, blend_fn: W) {


Do we plan to use this function anywhere for RGB? It feel more like an RGBA function so I am inclined to leave it off until we need it.

benhansen-io · 2026-06-11T10:40:53Z

+
+    /// Blends this framebuffer with the current [`MainFramebuffer`], and writes the result back to
+    /// this buffer.
+    pub fn blend_with_current(&mut self) {


I don't think we need this function unless you know of a use case. If we find we need it, I would rather have a function to blend two Framebuffers and let the users call get_currently_set

benhansen-io · 2026-06-11T10:42:15Z

+    }
+
+    /// Flush the buffer's contents to lights, ignoring alpha level.
+    pub fn flush_no_alpha(&self) {


I think we can remove this function and let them. Its pretty easy to call .to_rgb().flush()

benhansen-io reviewed May 11, 2026

View reviewed changes

benjhar requested a review from benhansen-io May 11, 2026 11:57

benhansen-io reviewed May 11, 2026

View reviewed changes

benhansen-io requested a review from tiesselune May 11, 2026 15:12

benjhar force-pushed the feature/composable_framebuffers branch from 5d4e04e to 43412b4 Compare May 14, 2026 14:08

benhansen-io requested changes May 21, 2026

View reviewed changes

benjhar added 15 commits May 29, 2026 11:45

Make Framebuffer generic over colour

96e68af

Simplify and rename ColorExt, add new Framebuffer functions and s…

9652b18

…pecialised impls

Remove unnecessary type specification

cc7216a

Leaving the others in framebuffer.rs as those make code clearer

Simplify alpha_blend

e44c543

Remove unnecessary type constraints

1653ce0

Force RGB for circles (for now), fix from_bytes bug.

781ef20

Make lights_plane functions generic, change RGBA::dim_to to use alpha

f3b0492

Change blend to weighted_average, remove free functions to avoid …

15acb97

…confusion.

Tidy impls

0a6dbe0

Change Framebuffer weighted_average and dim to take function pointers

f06da0b

Add flush_blend API that allows users to pass blend fn

119fb40

Add alpha_blend fn for blending RGBA

1920c1c

Add FramebufferRGBA type alias.

e18669b

WIP

87eee0c

Fix RGB blend

ea5ea15

benjhar force-pushed the feature/composable_framebuffers branch from 0bb96e1 to ea5ea15 Compare May 31, 2026 20:27

benjhar added 2 commits May 31, 2026 21:47

Add blend_with_current fns to Framebuffers

7c87a96

Consistent ordering for blend functions.

d04ea63

benjhar requested a review from benhansen-io May 31, 2026 20:49

benhansen-io requested changes Jun 12, 2026

View reviewed changes

Conversation

benjhar commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benhansen-io left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benjhar commented May 11, 2026

Uh oh!

benhansen-io commented May 11, 2026

Uh oh!

benhansen-io commented May 11, 2026

Uh oh!

benjhar commented May 11, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjhar May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benhansen-io commented May 13, 2026

Uh oh!

benhansen-io left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjhar commented May 25, 2026

Uh oh!

benjhar commented May 31, 2026

Uh oh!

benhansen-io left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

benjhar commented May 8, 2026 •

edited

Loading

benjhar May 14, 2026 •

edited

Loading