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

Fixes to FlxStrip rendering #2696

Merged
merged 5 commits into from
Jan 3, 2023
Merged

Conversation

UncertainProd
Copy link
Contributor

This should fix #2694

@UncertainProd
Copy link
Contributor Author

Also in the issue linked, I had proposed using a 'repeat' field to control whether a bitmapfill or a shaderfill would be used but it turns out that ShaderInput objects already have a 'wrap' field that controls whether the texture gets repeated or not, so the old solution I proposed isn't needed and I just set shader.bitmap.wrap to REPEAT here

@Geokureli
Copy link
Member

Geokureli commented Dec 15, 2022

Firstly, my schedule is insane for all of december, so I may take a bit to get to this. second, I can't merge this until I fully understand it.

  1. How does this fix the issue.
  2. Why wasn't this needed before to achieve the same result?
  3. why change numVertices to numTriangles

I'm still considering reverting the previous PR, and then having a new pr that implements it without breaking everything. reason being is that these changes seem to be far reaching, and can effect things that cannot be unit tested. So before I release a changes like this We either need to develop a manual testing strategy that ensures nothing is broken in flixel, addons or ui, or we need to develop automated tests that notify us when things are broken. I have no idea how to automate graphical verification. So perhaps we need to list out everything that uses vertice drawing and develop a comprehensive test project that allows us to visually verify that these changes are okay?

My ultimate fear is that this fix will break something else and it will just continue forever, at a time when I can't dedicate a lot of time to HF. also developing a test strategy would likely help me understand flixel's rendering system better, which is always good

@UncertainProd
Copy link
Contributor Author

  1. Basically, FlxTiledSprite has vertices at (0, 0) (Width, 0) (Width, Height) and (0, Height) and it tries to draw the tile graphic over that area. But if Width and Height are bigger than the size of the tile graphic then (before the fix), it would basically extend the colors on the edge to fill in the remaining area, kinda like this (the yellow border represents the size of the tile texture):
    206659253-c7bc28aa-91f7-4b72-8e8f-fb009dd2b1e6
    Which is what led to the broken behaviour before. However, it is possible to change the way this situation is handled by setting shader.bitmap.wrap to REPEAT, which will override the above behaviour and instead make the graphic repeat itself just like it used to before

  2. Before, when FlxDrawTrianglesItem used beginBitmapFill instead of beginShaderFill, we were able to specify if the texture should repeat or not using the repeat parameter in beginBitmapFill, which was always set to true:

    camera.canvas.graphics.beginBitmapFill(graphics.bitmap, null, true, (camera.antialiasing || antialiasing));

    But since beginShaderFill did not have this repeat parameter, we instead have to change shader.bitmap.wrap to REPEAT to preserve the old behaviour

  3. When I was trying to fix the repeat thing, I noticed that I would get a warning when testing on html5 in firefox which looked like this:
    webgl_warn
    And here I kinda just tweaked stuff related to vertices till it went away (I don't know much about webgl), which is why I ended up using numTriangles instead of numVertices.

What I had done for testing was basically to ctrl-f for 'drawTriangles' across flixel and flixel-addons and then I just made instances of all those classes into one project and compiled it

@Geokureli
Copy link
Member

tested this branch on my test project and it looks like shaders and color effects aren't working on this branch. I should mention that I reverted #2701 for the flixel 5.1.0 release. So you may need to add all that back into this branch

@UncertainProd
Copy link
Contributor Author

UncertainProd commented Dec 28, 2022

Oh, but when I tested it using this code, it worked for me (its a slightly modified version of the test link you had sent: https://github.com/Geokureli/flixel-tests/blob/main/Source/states/FlxStripShaderTestState.hx ):

package;

import flixel.FlxG;
import flixel.FlxSprite;
import flixel.FlxState;
import flixel.FlxStrip;
import flixel.graphics.tile.FlxDrawTrianglesItem.DrawData;
import flixel.system.FlxAssets.FlxShader;
import flixel.util.FlxColor;

class PlayState extends FlxState
{
	var sprite:FlxSprite;
	var strip:FlxStrip;

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

		add(sprite = createSprite(0, 0));

		add(sprite = createSprite(110, 0, FlxColor.BLUE));
		sprite.color = 0xFF000080;

		add(sprite = createSprite(220, 0));
		sprite.color = 0xFFff0080;

		add(sprite = createSprite(330, 0));
		sprite.shader = new GreenShader();

		add(sprite = createSprite(440, 0));
		sprite.setColorTransform(.5, 1.0, 0.0, 0.5, 0x40, 0x40, 0x40); // testing alphaMultiplier

		// strips

		add(strip = createStrip(0, 110));

		add(strip = createStrip(110, 110, FlxColor.BLUE));
		strip.color = 0xFF000080;

		add(strip = createStrip(220, 110));
		strip.color = 0xFFff0080;

		add(strip = createStrip(330, 110));
		strip.shader = new GreenShader();

		add(strip = createStrip(440, 110));
		strip.setColorTransform(.5, 1.0, 0.0, 0.5, 0x40, 0x40, 0x40); // testing alphaMultiplier
	}

	function createSprite(x = 0.0, y = 0.0, color = 0xFFffffff)
	{
		return new FlxSprite(x, y).makeGraphic(100, 100, color);
	}

	function createStrip(x = 0.0, y = 0.0, color = 0xFFffffff)
	{
		var strip:FlxStrip;
		strip = new FlxStrip(x, y);
		strip.makeGraphic(100, 100, color);
		strip.vertices = DrawData.ofArray([0.0, 0.0, 50, 0, 25, 50]);
		strip.indices = DrawData.ofArray([0, 1, 2]);
		strip.uvtData = DrawData.ofArray([0, 0, 0, 1, 1, 1.0]);
		return strip;
	}
}

class GreenShader extends FlxShader
{
	@glFragmentSource('
	#pragma header

	void main()
	{
		vec4 clr = texture2D(bitmap, openfl_TextureCoordv);
		gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0) * clr.a;
	}
	')
	public function new()
	{
		super();
	}
}

Did you try it with the latest commit I made? (899dae4)

@Geokureli
Copy link
Member

Geokureli commented Dec 29, 2022

Turns out I was on haxelib version 5.1.0, when I thought I was on dev. This is working.

I edited my test to include FlxTiledSprite, it seems to work in all cases
Screenshot 2022-12-28 at 6 39 52 PM
Here's the new test. My goal is to update this with various things we need to visually test as we make changes to FlxStrip rendering

@mastereric Any issues with this?

@UncertainProd
Copy link
Contributor Author

UncertainProd commented Dec 29, 2022

I found that these classes from flixel-addons either use drawTriangles or extends FlxStrip, so these can also be added into the test:

  • FlxSliceSprite (uses drawTriangles and extends FlxStrip)
  • FlxSpine (uses FlxStrip in it's wrapper variable)
  • FlxClothSprite (uses drawTriangles)
  • FlxTiledSprite (extends FlxStrip)

Although some of these (like FlxClothSprite) need some changes to it's source code for shaders to work on it, and those changes would only work on this fork of flixel. I've made those changes on this fork of flixel-addons

@Geokureli
Copy link
Member

Geokureli commented Dec 29, 2022

Thanks for the list, we dont need to add shaders to all of these were just need to make sure existing features aren't broken, I'll add these to my test.

@UncertainProd
Copy link
Contributor Author

Cool 👍

@Geokureli
Copy link
Member

Fyi should be able to check all these tomorrow, sorry for the delay

@Geokureli Geokureli merged commit 0bda25e into HaxeFlixel:dev Jan 3, 2023
@UncertainProd UncertainProd deleted the flxstrip-fixes branch January 7, 2023 08:53
Geokureli added a commit that referenced this pull request Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changes to FlxStrip break FlxTiledSprite
2 participants