-
Notifications
You must be signed in to change notification settings - Fork 444
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
Improve performance of debug drawing #3168
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conflicts with some changes in #3158
this will have to wait until then (shouldn't take long).
If this does improve performance over the old way, AWESOME! this is much simpler than what I was imagining. Some notes
- I'd like some actual benchmarking tests done on various targets and reported here, just to make sure it's doing what you say, and that it doesn't tank on a specific target
- We should make a FlxDebugDrawUtil or some
abstract DebugGraphic(openfl.display.Graphics)
that creates new helpful drawing tools for all the debug drawing classes to use, right now all it needs is
- drawRect with ways to customize x, y, width, height, thickness, color and alpha
- drawLine with ways to customize x1, y1, x2, y2, thickness, color and alpha
- and maybe a drawCircle or ellipse with all the expected args
flixel/FlxObject.hx
Outdated
endDrawDebug(camera); | ||
} | ||
|
||
function drawDebugBoundingBox(gfx:Graphics, rect:FlxRect, allowCollisions:Int, partial:Bool) | ||
function drawDebugBoundingBox(gfx:Graphics, rect:FlxRect, allowCollisions:Int, partial:Bool, ?zoom:Float = 1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to simply use a size of 0.5 like it has been, in fact, there's benefits to having this not perfectly align with the camera's pixel resolution, as it more discernible, plus adding new args may break overriding classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I don't think you would want that, I didn't want to do it either, but without doing a "zoom" variable, this is what it looks like:
This is normal btw, this is OpenGL doing its shenanigans and sometimes its not 100% accurate to display stuff in exchange of performance.
One thing that I tried as possible to make it not break a lot, is adding a "Nulled" already set variable at the end, so people with older code and trying to port it could use this function and would not give an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to omit the zoom arg, without missing lines (I didn't see any at zoom = 0.3, but I see a little at zoom=0.2, which I'm okay with)
rather than drawing the rect at ±.5 from the rect, I draw a rect where the outer rect matches the given rect, and the inner rect is 1px smaller, this aligns them more with the actual pixels and more closely matches the old behavior
flixel/FlxObject.hx
Outdated
gfx.beginFill(color, 0.5); | ||
var size = Math.max(1 / zoom, 1); | ||
|
||
gfx.drawRect(rect.x - size, rect.y - size, size, rect.height + size); | ||
gfx.drawRect(rect.x, rect.y - size, rect.width + size, size); | ||
gfx.drawRect(rect.x + rect.width, rect.y, size, rect.height - size); | ||
gfx.drawRect(rect.x, rect.y + rect.height - size, rect.width + size, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will these have stronger color in the overlapping corners? Rather than drawing 4 rects could we specify 8 points to make a hollow rect shape?
also 0.5 alpha should be .75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this won't have stronger color cos they're not overlapping, they're drawn in a specific way so they don't touch, that's why all the weird math between the positions and the sizes lol
The alpha was set 0.5
before I changed it, so it was like that. altho I could change it to .75 no problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should affect negatively in any shape or form, its basically using the GPU (hardware accelerated mode) of the Graphics, so it should not affect at all.
Thats... Thats way complicated than it sounds lol, I mean, drawRect should be easy enough with just adding a rectangle and adding 4 more, but drawCircle is a shader, same with drawLine. I have some tests im trying to do on OpenFL as a revamp of the Graphics rendering class so it doesn't have to use the CPU based rendering, but it should be part of a major PR there imo |
Something worth noting, using this method basically removes the issue with memory rising when you zoom in, so not only it is performance wise, but also memory wise it helps |
Point is I need to make sure this actually has the benefit that you claim it does, and doesn't have unintended side effects on some specific target
drawHollowRect, and drawFilledRect should suffice, for now. draw line would be great too, its important that we centralize this because we're gonna need to convert all the existing debug drawers over anyway |
I mean, Ik you have mac, you could test the changes on that platform, I could verdict that on Windows, Hashlink-Windows and HTML5 seem to be working good to me |
I meant share actual tests and the results, this is pretty standard stuff on perf PRs |
in my test I saw better performance on chrome, but far, FAR worse performance on macos and hl. without this change, got 10-30fps (fluctuating wildly), with this change I see consistent 5fps. Seeing the same-ish results in HL. about 30-40 fps before and 8fps after @Cheemsandfriends, could you try this code with my test state on windows and see if it performs better? We may need to check for mac |
Yeah... apparently it's due to the rendering being 4x when rendering per line, in Browser it seems to be fixed cos it does some caching on their end I believe, but desktop does not, I'm trying to fix it by doing some kind of shader that directly does this |
does this mean you're also seeing worse performance on windows as well? |
Yeah Yeah, sorry for not saying it sooner, normally on fewer elements and on really big graphics the new version clears the old version. Even now I think the equivalent of the method as it is right now is if the old version got called 4 times the same (it takes one render per line). That's why the way to fix it would be to call a drawQuads and make a small shader that does the outlining |
for shits and giggles i tried using a single shape, and now i'm seeing about 20fps instead of 5 // outer
this.moveTo(x, y);
this.lineTo(x + width, y);
this.lineTo(x + width, y + height);
this.lineTo(x, y + height);
this.lineTo(x, y);
// inner
this.lineTo(x + thickness, y + thickness);
this.lineTo(x + thickness, y + height - thickness);
this.lineTo(x + width - thickness, y + height - thickness);
this.lineTo(x + width - thickness, y + thickness);
this.lineTo(x + thickness, y + thickness);
this.endFill();
TBH i don't see a lot of issues from fewer large objects, it still runs pretty good, i mostly have trouble with tilemaps drawing hundreds of tiny boxes |
|
I tried a fewer larger objects and both the old and new ways had no performance issues |
if you mean on the non accelerated version, No it does not, from my experience at least. Im taking this as an example of zoomed, which acts the same way as having a big image. Besides the point that, it is not benefitial to keep it due to memory reasons. It does not run good, REALLY big images like 2k or 1k would be impossible to debug when it is at its normal zoom at its normal scale, imagine bigger zooms |
the zoom here is 3 |
i think we can avoid using the shader if we use drawQuads to draw all 4 rects |
It doesn't seem like we have a clear path forward on this. Gonna move this off of 5.9.0 and come back to this, later |
FlxObjects use
lineStyle
as an outline. This is common practice in Flash, but in OpenFL, everything is set to rendering to CPU, causing big unoptimised images when you're using a hardware accelerated mode of the application.Fixes #3164