Rework & separate renderers

[ACrazyTown]

Here we go, first step towards a renderer overhaul. Looks like a pretty big and scary change but 99% of this is just moving stuff around.

Shout out goes to Beeblerox & Austin East, a lot of this is based on the original renderer overhaul branch, I'm just porting bits and pieces to modern Flixel.

Changes

FlxRenderer

Adds a base FlxRenderer class, accessible via the global FlxG.renderer, which serves as the base for all rendering functionality. Renderer implementations extend it and implement the required methods.

The blitting and draw quads renderers have been ported to FlxBlitRenderer and FlxQuadRenderer, respectively.

Since the renderer is a global thing, and not per-camera anymore, it works slightly differently. Before any calls to drawing commands you need to call FlxG.renderer.begin(camera);. This will be done internally by Flixel during a sprite's draw phase, but it's something to keep in mind if you're doing something out of the ordinary!

FlxCameraView

Adds a base FlxCameraView class. Like with renderers, different implementations extend the base class and add onto it. Camera views mainly just hold per-camera rendering related objects, stuff like OpenFL sprites and whatnot.

To avoid breaking changes, you can get a typed reference to the camera view using the camera.viewBlit and camera.viewQuad shortcuts. Use this to reference stuff like camera.flashSprite or camera.canvas.

Other previously established stuff

Most rendering methods from FlxCamera have been deprecated. If you need to issue drawing commands manually, use the FlxG.renderer API instead.

I say most because some batch related methods (e.g. startQuadBatch()) have been left as-is. I plan to tackle these at a later point and in a different PR.

---

I'm opening this as a DRAFT, because:

• This needs to be tested thoroughly to make sure I didn't accidentally break something
• Would be nice to add docs to a bunch of stuff
• I don't know what to do with a bunch of private internal variables in FlxCamera and alike. What's Flixel's way of handling this? Should I simply get rid of them, or deprecate them and make them point to where they were moved?

TODO:

• flixel/addons/display/FlxShaderMaskCamera.hx:222 -- Field fill should be declared with overload since it was already declared as overload in superclass
• Documentation
• Figure out what to do with deprecated private functions/vars
• Better function names?

[ACrazyTown]

I decided to deprecate all the internals and make them point to where they were moved, to avoid breaking anything.

Addons and UI will need to be updated to avoid warnings, I'll get to that in a bit. Addons specifically seems to have an issue because FlxShaderMaskCamera overrides camera.fill(), it'll need to be updated to also add overload extern, hopefully that's not a blocker

I think this is in a good enough of a state now for a review, so I'll undraft

[ACrazyTown]

Some more notes and thoughts. I'm currently playing around trying to implement an OpenGL renderer to really test the flexibility of this system.

Overloading methods in FlxCameraView won't work:

For stuff like FlxMaterial and FlxTrianglesData, the function signatures of the core rendering functions need to be changed. To avoid breaking changes, I was just going to do this with overloads, but that didn't work out because overloads need to be inlined and you can't override inline functions.

I got around this by deprecating drawPixels() and copyPixels() for draw() and copy() in FlxCameraView, but I couldn't do the same in FlxCamera because it inherits FlxBasic.draw(). I ended up overloading the camera's drawPixels() and copyPixels() to call the new methods in camera view. Pretty gross solution IMO but I can't think of a better way without a breaking change.

Unifying renderBlit with other renderers

I did some work related to this in 3cd97d9, but there's still a couple places where we have to do things differently depending on the renderer, notably drawPixels() and copyPixels():

flixel/flixel/FlxCamera.hx

Lines 743 to 744 in c32ab91

	public function drawPixels(?frame:FlxFrame, ?pixels:BitmapData, matrix:FlxMatrix, ?transform:ColorTransform, ?blend:BlendMode, ?smoothing:Bool = false,
	?shader:FlxShader):Void

When the blitting renderer is used, pixels will be used for the rendering, otherwise frame is used. I wonder if it's somehow possible to avoid this special behavior and just pass in one thing, without having to know what renderer is used. The user should just have to call camera.drawPixels(...); once, and the underlying implementation should take care of any special quirks.

Isolating direct access to renderer

Flixel shouldn't access any renderer implementations directly from common code, it should be done through the renderer abstraction. First thing that comes to mind is that we need to abstract way maxTextureSize

EDIT: This could be done via the RenderFrontEnd via some method like FlxG.render.getMaxTextureSize()

EDIT 2: This is no longer an issue with recent changes, see next comment

[ACrazyTown]

Originally this was just a port of the renderer abstraction by Beeblerox that I ported over to modern Flixel. As I was playing around with it, I found that there were certain things I wasn't too fond of, specifically the fact that access to the renderer was only possible through cameras, so here goes an attempt at a V2.

FlxCameraView has been demoted, and no longer handles talking to the renderer. This is now done by the global FlxRenderer (Accessible via FlxG.renderer) and its implementations. FlxCameraView is still around, though it now mainly just stores the various objects needed per-camera for rendering (e.g. the flash sprites and whatever).

Considering that the renderer is now global, I've also integrated some of the helpers mentioned in #3527 directly into FlxRenderer.

I also wonder if it'd be worth deprecating the drawing methods in FlxCamera, and pointing users who need advanced control over the renderer to use FlxG.renderer instead.

[ACrazyTown]

I think this is now in a reviewable state. No clue why CI is failing tho

[Geokureli]

No clue why CI is failing tho

flixel-addons FlxShaderMaskCamera.hx:110: characters 20-26: Field render is declared 'override' but doesn't override any field
system.render.quad.FlxQuadRenderer.hx:96: characters 18-38: Type not found : FlxDrawTrianglesItem
system.render.quad.FlxQuadRenderer.hx:113: characters 18-38: Type not found : FlxDrawTrianglesItem

[Geokureli]

Do we need to remove the render method from FlxCamera?

Seems like we could call FlxG.render.begin(this) from FlxCamera.render

[ACrazyTown]

Do we need to remove the render method from FlxCamera?

Oops, this is my bad! I accidentally got rid of it while refactoring some of my changes. Brought it back now.

The CI confused me because I saw all the snippets compiling fine yet the action was failing. Turns out I missed that it also compiles the demos... Would it be possible to make the action stop as soon as there's a single failure?

[Geokureli]

Would it be possible to make the action stop as soon as there's a single failure?

Maybe that's what this does?

We're still getting a hxSehException on cpp unit tests. Not sure what that means. Are you able to run/debug unit tests locally? May get a stack trace, that way

I'll start looking over this PR today, I've only been skimming, so far

[ACrazyTown]

We're still getting a hxSehException on cpp unit tests. Not sure what that means. Are you able to run/debug unit tests locally? May get a stack trace, that way

Uncaught exception: Null access .method
Called from flixel.graphics.frames.$FlxFrame.__constructor__(flixel/graphics/frames/FlxFrame.hx:162)
Called from flixel.text._FlxBitmapText.$ReusableFrame.__constructor__(flixel/text/FlxBitmapText.hx:1797)
Called from .init(flixel/text/FlxBitmapText.hx:329)

Seems like FlxG.renderer is null during unit tests... which is weird since it should be created immediately along with the FlxGame. Even weirder, commenting this single line makes the unit test boot up without crashes:

flixel/tests/unit/src/TestMain.hx

Line 27 in 5f4dc29

suites.push(TestSuite);

I'm not familiar with the unit tests, so I'm not really sure what could be going on. Interesting that just pushing the TestSuite class into the array causes FlxG.renderer to be null.

EDIT: I'm fairly certain it's because ReusableFrame is static, therefore the render method check is done BEFORE we have a renderer

EDIT 2: Made a change to create the ReusableFrame once, at runtime. Hopefully this is an acceptable workaround. C++ CI now passes.

[ACrazyTown]

@Geokureli How do you feel about this way of preventing breaking changes:

	// The old deprecated signature, uses a helper material so that it can call the internal method
	@:deprecated	
	overload extern public inline function drawPixels(?frame:FlxFrame, ?pixels:BitmapData, matrix:FlxMatrix, ?transform:ColorTransform, ?blend:BlendMode,
			smoothing:Bool = false, ?shader:FlxShader):Void
	{
		_helperMaterial.blendMode = blend;
		_helperMaterial.smoothing = smoothing;
		_helperMaterial.shader = shader;
		
		drawPixelsInternal(frame, pixels, _helperMaterial, matrix, transform);
	}
	
	// The new signature, just call the internal method
	overload extern public inline function drawPixels(?frame:FlxFrame, ?pixels:BitmapData, material:FlxMaterial, matrix:FlxMatrix,
			?transform:ColorTransform):Void
	{
		drawPixelsInternal(frame, pixels, material, matrix, transform);
	}
	
	// For internal use only
	// For example, was reworked to use the new FlxMaterial API
	function drawPixelsInternal(?frame:FlxFrame, ?pixels:BitmapData, material:FlxMaterial, matrix:FlxMatrix, ?transform:ColorTransform) {}

drawPixels() is just a call to drawPixelsInternal(), which does the actual rendering. We could document the internal draw methods as "use at your own risk" and note that its signature could change at any time. Then we'd just overload the public method like I did above. This way there's a breaking change in only the renderer implementation, which would just be Flixel's, or someone else's who's crazy enough to build their own. Either way I think it's a very rare case so IMO that'd be fine.

I might have ideas for more changes soon, as I'm battletesting this implementation yet again!

[ACrazyTown]

Also while I'm at it, I might as well ask if you know what the deal is with the batching functions in FlxCamera (e.g. startQuadBatch()). I see they're public, but hidden from autocomplete. Are they meant to be a public API? Do we also have to avoid breaking changes with them?

I thought they were only meant to be used by the draw functions, but it seems they're also used by FlxBitmapText, FlxTilemap and some other addons classes, for some reason

[theoo-h]

hell yeah

[Geokureli]

Hello! I'm not dead, I've finally started my deep dive into this daunting thing. I'll probably have a million questions about the order in which things are happening and why things are done a certain way because I'm not very well-versed in rendering (yet). I'm gonna try to deploy this branch to https://dev.haxeflixel.com/demos/ and manually verify every demo, as its just too easy to miss something big in some one-off file.

I really wish there was a way to automate tests of the rendering systems, without capturing screen grabs and comparing to expected images.

At a glance, I'm not sure I like how FlxG.renderer is used, where a camera is set at the current target via FlxG.renderer.begin() followed by methods like clear() or render().

1. These seem like under the hood methods used by flixel systems, rather than something most devs will need global access to (like the other FrontEnds in FlxG).
2. I would really like to avoid this kind of modal global system, where a target camera is stored internally, and then has operations performed on it
   Is there a reason that, this code can't simply be FlxG.renderer.render(myTargetCamera);? Specifically, can methods in FlxRenderer take the target camera as arg, and perform operations on that instance without needing to store it internally first? My assumption is that other things down the road, likely in the draw tree, call FlxG.renderer methods, asserting or assuming that it's targeting the desired camera, or something.

My expectation was that this is what FlxCameraView was for. I.E.: internal flixel code would have access to each camera's view for low-level operations, which are separated from FlxCamera's high-level features. I was expecting calls like this:

if (FlxG.renderBlit)
{
	camera.fill(camera.bgColor, camera.useBgAlphaBlending);
	camera.screen.dirty = true;
}
else
{
	camera.fill(camera.bgColor.rgb, camera.useBgAlphaBlending, camera.bgColor.alphaFloat);
}

to become this:

camera.view.clear();

or

@:privateAccess camera.renderer.clear();

rather than this:

FlxG.renderer.clear();// which should honestly be called clearCurrent()

I assume there's good reasons, but they aren't clear to me, and therefore won't be clear to devs.

This seems like the most broad and structural complaint I can find, so let's talk about this while I go through this with a fine-toothed comb

[ACrazyTown]

1. These seem like under the hood methods used by flixel systems, rather than something most devs will need global access to (like the other FrontEnds in FlxG).

I agree somewhat, but this is where Flixel's architecture comes into play. FlxSprite.draw() is a public method that any extending class may override to change draw behavior, and in order to do that it needs to have access to the rendering methods. Overriding draw() and calling drawing methods directly is something that seems to be relatively common for more complex sprites. It's done by core classes, addons and also by libraries like FlxAnimate.

Compare this to something like OpenFL, where display objects don't have any control over how they're drawn, and their entire draw phase is handled internally. See for example: https://github.com/openfl/openfl/blob/develop/src/openfl/display/OpenGLRenderer.hx#L871-L894

Also, most of the methods in FlxRenderer that were ported over were already public in FlxCamera, with the exception of like clear() and render(). Sure, we could make them private. I kept them public mainly to avoid a breaking change

2. I would really like to avoid this kind of modal global system, where a target camera is stored internally, and then has operations performed on it

Truth is, Flixel's current render methods, and the way they're tied to the camera are a bit unconventional. Renderers are usually a global-ish concept, whereas cameras would just be render targets/render textures. (In Flixel land at least, generally they'd just be a matrix modifier or something)

Is there a reason that, this code can't simply be FlxG.renderer.render(myTargetCamera);? Specifically, can methods in FlxRenderer take the target camera as arg, and perform operations on that instance without needing to store it internally first? My assumption is that other things down the road, likely in the draw tree, call FlxG.renderer methods, asserting or assuming that it's targeting the desired camera, or something.

Your assumption is correct, I was hoping it'd simplify the draw tree by not having to constantly keep track of the camera. While renderer.begin() is not doing too much right now, in the future, with another renderer implementation it might start doing more work, especially once we throw stuff like render textures into the mix.

---

Some more thought definitely needs to be put in this. I've been trying to implement changes from #2915 in a separate branch, based on this, to try and stress test and future proof it as much as possible. I too have some mixed feelings about the singleton FlxG.renderer approach and how it works.

I've been trying to avoid breaking changes here, to make the transition easier, but I do wonder if biting the bullet and breaking things would allow us to make the nicer/easier to use. I assume maintaining 2 different branches with major changes between them would be a bit of a hassle, though.

Also, I had some additional concerns/questions, in case you missed them: #3539 (comment) , #3539 (comment)