Improve FlxSprite color transform handling, deprecate FlxColor.to24Bit()

[richTrash21]

Changes:

• FlxSprite.setColorTransform() now bypasses color and alpha setters.
• FlxSprite.useColorTransform now accounts for alpha offset.
• FlxColor.to24Bit() is now deprecated in favor of FlxColor.rgb field.

[Geokureli]

In general, splitting each change into a separate PR is preferred. Multi-issue PRs tend to take much longer and are less likely to get approved, but let's see what we got here

• FlxSprite.setColorTransform() now bypasses color and alpha setters.

What was the issue, previously?

• FlxSprite.useColorTransform now accounts for alpha offset.

Can you elaborate on this?

[Geokureli]

side note, I really hate these setters, and may remove them in the future
Edit: at least remove the clamp and updateColorTransform call

[richTrash21]

• FlxSprite.setColorTransform() now bypasses color and alpha setters.

What was the issue, previously?

Calling setColorTransform() would set color and alpha via setters. If values of these fields were changed, they would call updateColorTransform(), which is pointless since setColorTransform() does the same thing. Adding @:bypassAccessor metadata makes so fields are set directly, reducing unnececeary updateColorTransform() calls.

• FlxSprite.useColorTransform now accounts for alpha offset.

Can you elaborate on this?

After calling setColorTransform(), useColorTransform represented if the sprite had any transparency, coloring or RGB offset, but ignored alphaOffset. This was fixed by using RGBA offset check, instead of RGB.
There was a similar problem with updateColorTransform(), after calling which, useColorTransform ignored any offset.

Note: while testing useColorTransform i also discovered a bug where using useFramePixels would result in incorrect color transformation on animated sprites. Writing an issue about that.

[Geokureli]

I don't see any evidence that alphaOffset actually works, enough to support checking for it

Edit:I also don't understand why useColorTransform is needed, in most cases

[Geokureli]

why is this checked, here, rather than in line 1043?

[richTrash21]

At first we need to cache the basic transform, i.e. color and alpha, then we set colorTransform multipliers based on that, and after this we cache color offset transform.
Note: on second thought, maybe it will be better to just set color offset to 0 on updateColorTransform()?

[Geokureli]

I still don't understand a case where this will behave differently than:

useColorTransform = alpha != 1 || color.rgb != 0xffffff || colorTransform.hasRGBAOffsets();

furthermore why can't this just be:

useColorTransform = alpha != 1 || color.rgb != 0xffffff || colorTransform.hasRGBAOffsets();
colorTransform.setMultipliers(color.redFloat, color.greenFloat, color.blueFloat, alpha);

Note: on second thought, maybe it will be better to just set color offset to 0 on updateColorTransform()?

I agree, but this is could cause unintended side effects to existing projects. Also, this is called whenever alpha changes. We shouldn't clear color transform offsets whenever alpha changes

[richTrash21]

I don't see any evidence that alphaOffset actually works, enough to support checking for it

Here's a test project for that.

package;

import flixel.FlxG;
import flixel.FlxSprite;
import flixel.math.FlxMath;
import flixel.text.FlxText;

class PlayState extends flixel.FlxState
{
	var sprite:FlxSprite;
	var alphaText:FlxText;
	var alphaOffsetText:FlxText;

	var alpha = 1.0;
	var alphaOffset = 0.0;

	override public function create()
	{
		super.create();

		// FlxG.camera.bgColor = 0xFF999999;

		sprite = new FlxSprite(60, 60).makeGraphic(200, 200);
		add(sprite);

		alphaText = new FlxText(60, 280, 0, "", 16);
		add(alphaText);

		alphaOffsetText = new FlxText(60, 320, 0, "", 16);
		add(alphaOffsetText);
	}

	override public function update(elapsed:Float)
	{
		super.update(elapsed);

		final mult = 3 * (FlxG.keys.pressed.SHIFT ? 10 : 1);

		if (FlxG.keys.pressed.Q)
			setAlpha(alpha - 0.01 * elapsed * mult);
		if (FlxG.keys.pressed.E)
			setAlpha(alpha + 0.01 * elapsed * mult);
		if (FlxG.keys.justPressed.W)
			setAlpha(1);

		if (FlxG.keys.pressed.A)
			setAlphaOffset(alphaOffset - 1 * elapsed * mult);
		if (FlxG.keys.pressed.D)
			setAlphaOffset(alphaOffset + 1 * elapsed * mult);
		if (FlxG.keys.justPressed.S)
			setAlphaOffset(0);

		sprite.setColorTransform(
			sprite.colorTransform.redMultiplier, sprite.colorTransform.greenMultiplier, sprite.colorTransform.blueMultiplier, alpha,
			sprite.colorTransform.redOffset,     sprite.colorTransform.greenOffset,     sprite.colorTransform.blueOffset,     alphaOffset
		);

		alphaText.text = "alpha: " + FlxMath.roundDecimal(alpha, 3);
		alphaOffsetText.text = "alphaOffset: " + FlxMath.roundDecimal(alphaOffset, 3);
	}

	function setAlpha(value:Float)
		alpha = FlxMath.bound(value, 0, 1);

	function setAlphaOffset(value:Float)
		alphaOffset = FlxMath.bound(value, -255, 255);
}

Note: only negative alphaOffset values have visible effect, but doc states that positive work too? I also tested openfl sprite to compare behaviour (not a part of this test project) and it's the same, so i guess this is intended.

I also don't understand why useColorTransform is needed, in most cases

useColorTransform is used to determine if updateFramePixels() should color framePixels. This also causes #3414, so we may just deprecate it.
Update: made a pr to fix this issue.

[Geokureli]

updateFramePixels and useFramePixels only effect renderBlit targets, btw, are you using these in a project

[richTrash21]

updateFramePixels and useFramePixels only effect renderBlit targets

useFramePixels actually works perfectly on renderTile, have no idea what you ment by that.

btw, are you using these in a project

Yes, but now as an experiment, disabled caching is a bit concerning

[Geokureli]

updateFramePixels and useFramePixels only effect renderBlit targets

useFramePixels actually works perfectly on renderTile, have no idea what you ment by that.

Yeah sorry, this works on renderTile, but it seems like the purpose of these is to enable blitting functionality when flixel is in renderTile mode

btw, are you using these in a project

Yes, but now as an experiment, disabled caching is a bit concerning

What I mean is, are you only using these in academic experiments, or do you have a practical use for this?

I recently found out that updateFramePixels was used in getPixelAt() when it didn't need to. It was causing worse performance, and I'm wondering if it should just be avoided in renderTile, altogether

[Geokureli]

1. I noticed that bypassing the setters affected FlxText which overrides set_color and updateColorTransform
2. I made colorTransform final I removed the part where colorTransform is destroyed, openfl rectangles aren't pooled so nulling them in destroy won't make a significant different. eventually this field will be final, when flixel-addons is updated
3. I deprecated useColorTransform, and added a simple hasColorTransform()
4. I added the code from Fix incorrect coloring of animated sprites with useFramePixels on renderTile #3416, just so I can test these changes together

let me know if you see any issues, here. also in 3416 you mention:

Running this code will result in this: applied color appear darker or as a mix of previous and current colors.

I do not see that, and I'm not sure why I would, am I missing something?