Composite border specifications work unintuitively with children that specify non-standard blending modes

[bdach]

Title is a doozy but I don't know how to describe it better, so here's a code sample: https://github.com/ppy/osu-framework/commit/775908aea0d69b5ae6abb7f00d3c46f74cedebef

The above is not a farfetched scenario, we already have a usage of this pattern game-side in ShearedButton. Here's what happens in current master:

https://user-images.githubusercontent.com/20418176/169663501-0dbdfe8d-86aa-44c4-8d9a-dc4d13a93c84.mp4

The border flashes bright green (#00ff00, basically). What I believe is happening here, is that because border drawing is deferred to the children via the masking shader, the additive-blended child actually draws the parent's border, and blends it with the same border drawn by the other child, causing it to flash bright green as a result. This generally seems like undesirable behaviour, but is not obvious to fix.

I have a proposal for a "fix", but it's very ugly. Basically what I did to semi-confirm the above is to push a temporary MaskingInfo with the border colour spec replaced with Colour4.Transparent. The code is here: https://github.com/ppy/osu-framework/commit/4d10ad0445e6a14421809bcb0551021ada1b4c0d, and the result looks like this:

https://user-images.githubusercontent.com/20418176/169663570-9c089b26-f1e3-4c50-b00f-33316f36df49.mp4

You could also keep the colour but turn down the thickness to 0 (https://github.com/ppy/osu-framework/commit/ebeb56a0fd9f9a59fcb2913e5626de06615a8b71), which looks like this:

https://user-images.githubusercontent.com/20418176/169663594-5b8c8ea3-d196-4172-9dba-5d1144034c61.mp4

I'm opening this as an issue for discussion for two reasons:

• That solution seems too ugly to live.
• I'm not sure which of the two resulting behaviours is expected (maybe none?)

[bdach]

Note that this is also fixable with a local workaround of nesting the border spec one level lower, i.e.:

            AddStep("create bordered box", () => Child = new Container
            {
                Anchor = Anchor.Centre,
                Origin = Anchor.Centre,
                Size = new Vector2(200),
                Children = new Drawable[]
                {
                    new Container
                    {
                        RelativeSizeAxes = Axes.Both,
                        Masking = true,
                        BorderThickness = 15,
                        BorderColour = Colour4.DarkGreen,
                        Child = new Box
                        {
                            RelativeSizeAxes = Axes.Both,
                            Colour = Colour4.Lime
                        },
                    },
                    additiveLayer = new Box
                    {
                        RelativeSizeAxes = Axes.Both,
                        Colour = Colour4.Red,
                        Blending = BlendingParameters.Additive,
                        Alpha = 0
                    }
                }
            });

Still doesn't change the fact that the original code works in a way that I think nobody expects, I think...?

[frenzibyte]

I personally think writing it this way clarifies whether the additive layer should flash the border of the container or not (reading the code/workaround above, it should). Should definitely mention this issue thread in case someone thinks the layer should be nested, regardless.

[peppy]

For the original code, I believe the first proposal makes sense visually. Also agree the actual usage that brought this issue to light should be refactored to work around the issue (since the way it's written right now implies the border wouldn't be affected by the flash, which was not the intention).

[smoogipoo]

• The first solution I don't think will handle a border colour with alpha.
• The second solution is interesting but I don't think it's correct, because the border is supposed to be drawn on top of everything else. The first solution is more accurate in this case, but only because there's no alpha.

One way I could foresee fixing this is by moving the border drawing out to the CompositeDrawable itself. Downside being, of course, that it would incur a 1x fill, but we're already doing a 1x fill in many cases: https://github.com/ppy/osu/blob/87947c6ef03d2db49e1380cc4e684127b998aeec/osu.Game/Beatmaps/Drawables/Cards/BeatmapCardContent.cs#L88-L100 On the plus side, this would handle a lot more edge cases that I could conjure up, such as what happens if the background doesn't fill the border-container.

Could also split it into its own Drawable type at that point.

[bdach]

I was going to say that we already have a ton of local workarounds for the border drawing being deferred to the children wherever we do want a border on a transparent drawable (like the IsPresent thing above). That said, moving the border draw to a full 1x fill always may be somewhat painful.

Not really sure this needs action at this rate, given it's never came up in however many years. I'll keep as issue because it is one, but we've worked past it in the original broken scenario and maybe this can be addressed one year or another when there's nothing better to do.