Fix flxstrip color causing not rendering other flxstrips

[davidetan]

When multiple FlxStrip are rendered using the same shader, if one of them uses the color property, it causes the other meshes to not be rendered.

Here an example:

class BugExample extends FlxState {
	override public function create():Void {

	var graphic = FlxGraphic.fromRectangle(100, 100, 0xFFff40ff);
	var mesh1:FlxStrip = new FlxStrip(100, 100);
	mesh1.graphic = graphic;
        mesh1.indices = Vector.ofArray([0, 1, 2, 0, 2, 3]);
        mesh1.vertices = Vector.ofArray([0., 0., 300., 0., 300., 300., 0., 300.]);
        mesh1.uvtData = Vector.ofArray([0., 0., 1., 0., 1., 1., 0., 1.]);
        add(mesh1);

        var mesh2:FlxStrip = new FlxStrip(200, 150);
	mesh2.graphic = graphic;
        mesh2.indices = Vector.ofArray([0, 1, 2, 0, 2, 3]);
        mesh2.vertices = Vector.ofArray([0., 0., 300., 0., 300., 300., 0., 300.]);
        mesh2.uvtData = Vector.ofArray([0., 0., 1., 0., 1., 1., 0., 1.]);
        add(mesh2);

	var mesh3:FlxStrip = new FlxStrip(300, 50);
	mesh3.graphic = graphic;
        mesh3.indices = Vector.ofArray([0, 1, 2, 0, 2, 3]);
        mesh3.vertices = Vector.ofArray([0., 0., 300., 0., 300., 300., 0., 300.]);
        mesh3.uvtData = Vector.ofArray([0., 0., 1., 0., 1., 1., 0., 1.]);

	// mesh3.color = -9306137; // comment this line to enable the render bug

	add(mesh3);
	super.create();
	}
}

Commenting and uncommenting the mesh3.color = -9306137; line, it will trigger the bug.
In this example mesh1 and mesh2 are added to the same triangles batch. A second triangle batch is created for mesh3.

Here wath happens at the first render iteration:

• the first batch is selected and shader.colorMultiplier.value is null. The draw call happens and the two meshes are visible.
• the second batch is selected and shader.colorMultiplier.value is set using the colorMultipliers of the batch. The draw call happens and the third mesh is painted over the others two.

Then the second iteration happens. Canvas is cleared and:

• the first batch is selected and shader.colorMultiplier.value has the previous values set by the second batch. The draw call and I guess since there is a number of colors that does not match the number of vertices, nothing is rendered for this batch.
• the second batch does the same thing of first iteration.

Same occurs for the next iterations.

This PR reset shader.colorMultiplier.value and shader.colorOffset.value to null, if there is no color multipliers or offsets.

I'm not entirely sure if that is the right place to reset these values. There is the batch reset method, but the shader is already null in that context, even though I could acces the graphic.shader, before the super.reset() is called.

---

Moreover, I've noticed that during the fill of color multipliers and offsets, the iteration occurs for all the indices in the batch, rather then the indices of the added triangle. This PR fix also that.

---

I'd like also to ask if there is any specific reason why color (and alpha) tinting, a new batch is initiated.

In my example, we need two draw call to render the three meshes. If we remove avoid to start a new batch when the mesh is colored through color or alpha, we can prevent to break batching. What do you think about this?

[davidetan]

Hello, it would be really helpful if anyone can consider reviewing this PR.
Right now this bug prevents tinted meshes from being fully functional.
Thanks! :D

[Geokureli]

Sorry, was dealing with family stuff

This PR seems to work in all my tests, I just wanna discuss a couple things, firstly, this odd behavior:

Screen.Recording.2024-08-13.at.2.12.01.PM.mov

Note: tint on the last mesh behaves as described, but the same happens for the first and then by colorizing two meshes it suddenly works on all future frames for all configurations. Now, your fix prevents all of this weirdness, I just thought I'd bring it up in case this means anything to you

Secondly, can you elaborate on what the numTriangles -> indicesLength fixes? It looks like a performance fix to prevent calculating numTriangles a bunch of times. I'm not against this change but you talk about it as if its a bug fix and I just want to make sure I understand what bug it fixes, if any

I'd like also to ask if there is any specific reason why color (and alpha) tinting, a new batch is initiated.

In my example, we need two draw call to render the three meshes. If we remove avoid to start a new batch when the mesh is colored through color or alpha, we can prevent to break batching. What do you think about this?

I'm not really well-versed in flixel's rendering, or rendering in general, could you elaborate on what you're proposing here? Specifically, if we were to implement your proposal, would that be a separate issue altogether, or would that effect this PR, which I'm ready to approve

[davidetan]

No need to say sorry! Family stuff wins over everything :)

Note: tint on the last mesh behaves as described, but the same happens for the first and then by colorizing two meshes it suddenly works on all future frames for all configurations. Now, your fix prevents all of this weirdness, I just thought I'd bring it up in case this means anything to you

I do not have access to your code example. When I tried to repro your example, it behaved in a different bugged way.

This is the flixes shader:

precision mediump float;
attribute float openfl_Alpha;
attribute vec4 openfl_ColorMultiplier;
attribute vec4 openfl_ColorOffset;
attribute vec4 openfl_Position;
attribute vec2 openfl_TextureCoord;
varying float openfl_Alphav;
varying vec4 openfl_ColorMultiplierv;
varying vec4 openfl_ColorOffsetv;
varying vec2 openfl_TextureCoordv;
uniform mat4 openfl_Matrix;
uniform bool openfl_HasColorTransform;
uniform vec2 openfl_TextureSize;
attribute float alpha;
attribute vec4 colorMultiplier;
attribute vec4 colorOffset;
uniform bool hasColorTransform;
void main(void) {
    openfl_Alphav = openfl_Alpha;
    openfl_TextureCoordv = openfl_TextureCoord;
    if (openfl_HasColorTransform) {
        openfl_ColorMultiplierv = openfl_ColorMultiplier;
        openfl_ColorOffsetv = openfl_ColorOffset / 255.0;
    }
    gl_Position = openfl_Matrix * openfl_Position;
    openfl_Alphav = openfl_Alpha * alpha;
    if (hasColorTransform) {
        openfl_ColorOffsetv = colorOffset / 255.0;
        openfl_ColorMultiplierv = colorMultiplier;
    }

}

It declares the vertex attribute colorMultiplier:

attribute vec4 colorMultiplier;

If this attribute is enabled, the shader expects this vector to be populated with a number of colors corresponding to the number of vertices (1 RGBA x vertex).

As far as I saw, custom attributes are set directly into the shader in flixel. In this case we do:

shader.colorMultiplier.value = colorMultipliers;

For triangles batching, the code uses a high level representation named FlxDrawTrianglesItems. Before reusing them, their old values are reset using the reset method.
Here we have:

if (colorMultipliers != null)
   colorMultipliers.splice(0, colorMultipliers.length);

This colorMultipliers is emptied. However, the shader.colorMultiplier.value is still holding the latest colorMultipliers values.

Consequently, in the render method of the FlxDrawTrianglesItems, when the shader.colorMultiplier.value is set:

if (colored || hasColorOffsets)
{
	shader.colorMultiplier.value = colorMultipliers;
	shader.colorOffset.value = colorOffsets;
}
else
{
	shader.colorMultiplier.value = null;
	shader.colorOffset.value = null;
}

If we do not nullify its value as in the else, we are asking to the shader to use those colors.
However, the first batch (the one without colors) has twice the number of vertex of the second batch (the colored one), and we are providing a number of color attributes that is lower than the number of vertices of the batch.
I guess that the shader program detects this incongruity and just skip the batch (super strange it does not error/warn anything out!).

---

Secondly, can you elaborate on what the numTriangles -> indicesLength fixes? It looks like a performance fix to prevent calculating numTriangles a bunch of times. I'm not against this change but you talk about it as if its a bug fix and I just want to make sure I understand what bug it fixes, if any

That's not a performance fix, that's another bug :D
addTriangles adds a triangle to the batch, in flixel for triangle is the FlxDrawTrianglesItems. The batch accumulates in its data structures (vertices, indices, uvs, colorMultipliers, ...) the respective values of the single FlxStrip that is being added.

Since we need to pass a color per vertex, but the color transform of the `FlxStrip holds a single color, the code loops over the number of vertices and adds for each vertex the color (and the alpha in the loop above).

However, the loop is using the wrong variable to loop! Currently is uses numTriangles that is the number of triangles of the entire batch.
This implies that if I add three consecutive FlxStrip where each of them has a single triangle, the array of colors will grow like this:

- first  `FlxStrip` -> [r1, g1, b1]
  (numTriangles is 1)
  3 * 1 element added, total 3
- second `FlxStrip` -> [r1, g1, b1, r2, g2, b2, r2, g2, b2]
  (numTriangles is 2)
  3 * 2 element added, total 9
- third  `FlxStrip` -> [r1, g1, b1, r2, g2, b2, r2, g2, b2, r3, g3, b3, r3, g3, b3, r3, g3, b3]
  (numTriangles is 3)
  3 * 3 element added, total 18

The expected result would be:

- first `FlxStrip` -> [r1, g1, b1]
  (indicesLength of the first FlxStrip is 3)
  3 * 1 element added, total 3
- second `FlxStrip` -> [r1, g1, b1, r2, g2, b2]
  (indicesLength of the second FlxStrip is 3)
  3 * 1 element added, total 6
- third  `FlxStrip` -> [r1, g1, b1, r2, g2, b2, r3, g3, b3]
  (indicesLength of the FlxStrip is 3)
  3 * 1 element added, total 9

The same thing happens for alpha.
This probably won't give any error because usually if we have more values for an attribute, then the number of vertices, the shader simply uses the the provided attributes up to the number of vertices. Differently from the previous case where we had less attributes than the vertices, in that case the shader has missing attributes and usually errors and does not render.

However, that's a bug because some vertices of the batch would use the wrong colors. In this case the third FlxStrip would use r2, g2, b2

---

I'd like also to ask if there is any specific reason why color (and alpha) tinting, a new batch is initiated.
In my example, we need two draw call to render the three meshes. If we remove avoid to start a new batch when the mesh is colored through color or alpha, we can prevent to break batching. What do you think about this?

I'm not really well-versed in flixel's rendering, or rendering in general, could you elaborate on what you're proposing here? Specifically, if we were to implement your proposal, would that be a separate issue altogether, or would that effect this PR, which I'm ready to approve

The easiest way to render is to make a draw call per FlxStrip. A draw call is in general the operation that takes more time in rendering.
Batching allows to reduce the number of draw calls by accumulating the vertices that can be drawn using the same "instruments" (same shader, same texture, same blending mode, same uniforms).
In that way we can render multiple FlxStrip in the same draw call.

Colors and alpha are vertex attributes, not uniforms. So there is no apparent reason to me to break the batch for that.
See this PixiJS example about tinting: https://pixijs.com/7.x/examples/basic/tinting
There is a single texture that is used to render 20 characters. Each of them is tinted with a different color. A single draw call is used to render all of them. In flixel, that would take 20 draw calls.
In any case, If we want to consider this chage, I think it's better to create a new PR.

---

Hope everything is clear, let me know if you need any additional clarifications :)

[Geokureli]

If we want to consider this chage, I think it's better to create a new PR.

cool, make one whenever you like

Here's the test I made, BTW

package states;

import flixel.FlxG;
import flixel.FlxStrip;
import flixel.group.FlxGroup;
import flixel.graphics.FlxGraphic;
import openfl.Vector;

class FlxStripColorTestState extends flixel.FlxState
{
	final meshes = new FlxTypedGroup<Mesh>();
	
	override public function create()
	{
		super.create();
		
		for (i in 0...3)
			meshes.add(new Mesh(10, 10 + i * 110));
		
		add(meshes);
	}
	
	override function update(elapsed:Float)
	{
		super.update(elapsed);
		
		if (FlxG.mouse.justPressed)
		{
			final mouse = FlxG.mouse.getWorldPosition();
			for (mesh in meshes)
			{
				if (mesh.overlapsPoint(mouse))
					mesh.toggleColor();
			}
			mouse.put();
		}
	}
}

@:forward
abstract Mesh(FlxStrip) to FlxStrip
{
	static inline var ON_COLOR = 0xFFffff00;
	static inline var OFF_COLOR = 0xFFffffff;
	
	inline public function new (x = 0.0, y = 0.0)
	{
		this = new FlxStrip(x, y);
		this.makeGraphic(50, 50, 0xFFff80ff);
		this.width = 100;
		this.height = 100;
		this.indices = Vector.ofArray([0, 1, 2, 0, 2, 3]);
		this.vertices = Vector.ofArray([0., 0., this.width, 0., this.width, this.height, 0., this.height]);
		this.uvtData = Vector.ofArray([0., 0., 1., 0., 1., 1., 0., 1.]);
		this.color = OFF_COLOR;
	}
	
	public function toggleColor()
	{
		this.color = (this.color == OFF_COLOR ? ON_COLOR : OFF_COLOR);
	}
}

[Geokureli]

Thanks!