Breaking Change: Rendering cleanup #3488
Conversation
|
FYI I think @richTrash21 tried a few of these changes in their branch, so their input is appreciated, here. Also, this contains multiple separate changes, can any of the non-breaking changes go in a separate PR? It's always exponentially more effort to verify multiple changes than separate individual ones. Can these breaking changes be rectified with, say overloaded functions, or by creating new functions with unique names? It seems too short to release a 7.0.0, and preferably, all breaking changes should be preceded by warnings to devs that a breaking change is coming, so they have ample time to prevent them. |
There was a problem hiding this comment.
I skimmed over and found a couple concerns, so far. In general I'd really prefer separate changes in separate PRs, so I can run each one through the gauntlet, especially the innocuous little style changes.
There's a lot that can be improved with flixel's rendering and if it takes breaking changes or a bunch of sweeping changes in orchestration, so be it, but It's important that for every individual change to flixel, we think about unit testing, or creating ways to visually verify that this and future changes work in all expected edge cases.
| public var colors:DrawData<Int> = new DrawData<Int>(); | ||
|
|
||
| public var repeat:Bool = false; | ||
| public var repeat:Bool = true; |
There was a problem hiding this comment.
I don't like change the default value of args, even if it is in a major release there's no good way to broadcast this to devs and they end up having either unknown bugs, or hard to track down bugs in their game.
There was a problem hiding this comment.
That's why I believe this part would be more fit for a major version change, though for this part in particular its mandatory, as the repeat being set to false by default would result in more broken visual behaviour with the repeat wrap mode actually being added as a setting for tile rendering.
| if (colored || hasColorOffsets) | ||
| if (colored) |
There was a problem hiding this comment.
I wanna say I tried this before and got unexpected issues in some cases. I'll try and find it.
There was a problem hiding this comment.
Please do let me know if it causes any issues, I've tried in many of my projects and its not causing any problems on my part.
| if (!(hasTransform || openfl_HasColorTransform)) | ||
| if (color.a == 0.0) | ||
| return color; | ||
| if (color.a == 0.0) | ||
| return vec4(0.0, 0.0, 0.0, 0.0); | ||
| if (!(hasColorTransform || openfl_HasColorTransform)) | ||
| return color * openfl_Alphav; |
There was a problem hiding this comment.
This part simplifies the number of checks in the shader. The old if (!(hasTransform || openfl_HasColorTransform)) part never had any effect because hasTransform was always set to true. Plus the alpha check should be done early since if its fully transparent it would never get affected by the color transform either way.
There was a problem hiding this comment.
Do you have any way of profiling shaders?
There was a problem hiding this comment.
Yes, using NVIDIA Nsight Graphics I can get some detailed information about the draw time required, both on CPU and GPU of openfl/flixel draw calls. For my testing I used the FlxBunnyMark demo with 20000 bunnies, which fill the screen up with a lot of pixels to be passed through the shader.
On my testing, with the graphics shader code, the bunnies take around 2.98 GPU ms to be rendered on screen.
In comparison, with the new graphics shader code, the bunnies take around 2.74 GPU ms to be rendered on screen.
Its not a major performance improvement since its just the removal of one if check, but its an improvement nonetheless.
I've been thinking about it for a bit and I do think I could divide some of the changes to seperate PRs to be easier to merge and pin point. I also rethought the idea of the hasColorOffsets breaking change, while internally I think it should be removed as it causes no issues, I'll add it back to startQuadBatch and startTrianglesBatch as an extra value that determines if the draw item will be colored or not, just for compatibility sake with older code. |
|
Just wanna add that I made #3467 a bit ago, which would check off one of the things from this list (remove unused blending variable and blendToInt() method) |
For a while, HaxeFlixel has had remnants in it's rendering code of different approaches to rendering, deprecated openfl behaviour or features, among other things. This PR is meant as a cleanup of all of that unused/broken behaviour to have clean and straight to the point rendering code.
Its important to note that this PR contains breaking changes, therefore it should only be merged in a major version change, along with all the necessary fixes to external libraries like flixel-addons.
Here's a list of all the changes done:
splicewithresize.blendingandblendToInt, as they're remnants from before openfl supported blends in the graphics api.hasColorOffsetsfrom draw items. Even though this function was passed around it was never actually used with the original purpose of reducing the number of variables needed to push to the graphics vertex shader. I tried implementing it myself but it ended up being overcomplicated and only slowed down performance with all the extra checks it requires. Now its unified with thecoloredvariable.repeatvalue to tile rendering. If it's turned off it'll use the same wrap method as normal quad rendering.Making this PR a draft first to make sure it works on all targets, so far I've tested desktop and web.
Feedback is appreciated!