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

Accelerate 2D by tracking state client-side #35696

Closed
wants to merge 1 commit into from

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Jan 29, 2020

GLES2 only first.

Godot 2D performance is usually bottlenecked by the methods used with OpenGL, there are currently drawcalls per quad.

In this PR basic state tracking is added to rasterizer_canvas_gles2::_canvas_item_render_commands().

This gives a 'free' performance increase of around 40% frames per second in a test scene, while requiring minimal changes to the overall mechanism.

Notes

  • I have been using the test project in this issue Godot 3.1 running slower than Godot 2.1.4 #27230 with the tilemap selected as the one under test.

  • The actual speed increase will scale with the number of Rects being drawn. There is also likely to be more benefit on some hardware / platform combinations than others, all should receive some benefit.

  • The best solution to this problem is batching, which will give far greater performance increases. But this might help as a simpler interim solution.

Mentioning @clayjohn as we discussed this yesterday.

@clayjohn
Copy link
Member

Changes look good so far. Definitely some style changes are needed to bring this into the Godot style. I am getting a speed increase, however mine is slightly less pronounced, more like a 20% boost (still extremely good).

I am however, getting a crash coming from line 869. Looks like _bind_canvas_texture() doesn't always return a valid texture.

Also, at some point we should extend this to other commands.

This is a very good start!

@lawnjelly
Copy link
Member Author

Changes look good so far. Definitely some style changes are needed to bring this into the Godot style. I am getting a speed increase, however mine is slightly less pronounced, more like a 20% boost (still extremely good).

I am however, getting a crash coming from line 869. Looks like _bind_canvas_texture() doesn't always return a valid texture.

Also, at some point we should extend this to other commands.

This is a very good start!

Yup, I've just fixed a crash when there was a NULL texture (needed to check for this), and there's still a state that needs refreshing because project settings in the editor isn't drawing correctly, but am almost there. 👍

@lawnjelly lawnjelly force-pushed the state-2d branch 2 times, most recently from 0d676ea to 8692f17 Compare January 30, 2020 11:12
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too many nitpicks! Looking good so far.

From my brief testing it appears all the crashing is gone. I will do further testing before approval

drivers/gles2/rasterizer_canvas_gles2.cpp Outdated Show resolved Hide resolved
tiling = p_tile;
}

int type; // Item::Command::Type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Item::Command type instead of int

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only reason I used an int at the time was because I needed a way or specifying 'unset' (and used -1) as the enum didn't have a NULL value. I'll have a look tomorrow whether it is possible to add to the enum without affecting other bits of the codebase, as agreed that would be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, may be best to add a "state_dirty" boolean or something similar then. The benefit of using the Enum is that we get the compiler to enforce a lot of static error checking stuff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look at putting a null value into the enum (as this will be the fastest at runtime and clear) and it was doable, so I have gone ahead with this, although it does mean adding extra case statements in a couple of other files to prevent compile warnings (switch not using all the enum values).

It doesn't look like the extra value in the enum will affect anything else (there is no use of the enum for determining array sizes for example), and shouldn't have any significant effect on jump tables. I've put the null value as the first in the enum (as is usually tradition). It could be named something else like 'TYPE_UNSET', but null seems quite language agnostic.

drivers/gles2/rasterizer_canvas_gles2.cpp Outdated Show resolved Hide resolved
drivers/gles2/rasterizer_canvas_gles2.cpp Outdated Show resolved Hide resolved
drivers/gles2/rasterizer_canvas_gles2.cpp Outdated Show resolved Hide resolved
drivers/gles2/rasterizer_canvas_gles2.cpp Outdated Show resolved Hide resolved
@clayjohn
Copy link
Member

For future reference. This PR only optimizes the use of TYPE_RECT objects. However, that encompasses the majority of objects used in 2D in Godot (e.g most GUI, Sprites, tilemap)

@lawnjelly lawnjelly force-pushed the state-2d branch 3 times, most recently from dceecbe to 4b4d55b Compare January 31, 2020 09:41
GLES2 only first.

Godot 2D performance is usually bottlenecked by the methods used with OpenGL, there are currently drawcalls per quad.

In this PR basic state tracking is added to rasterizer_canvas_gles2::_canvas_item_render_commands().

This gives a 'free' performance increase of around 40% frames per second in a test scene, while requiring minimal changes to the overall mechanism.
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I like how simple the implementation feels. I think it will be easy for others to understand. And it will be easy for us to add other command types when needed.

@akien-mga akien-mga added this to the 4.0 milestone Jan 31, 2020
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jan 31, 2020
@lawnjelly lawnjelly changed the title [WIP] Accelerate 2D by tracking state client-side Accelerate 2D by tracking state client-side Jan 31, 2020
@volzhs
Copy link
Contributor

volzhs commented Jan 31, 2020

@akien-mga is not available for 3.1.x?

@akien-mga
Copy link
Member

We can consider it for 3.1.x too if there's demand for it yeah.

@volzhs
Copy link
Contributor

volzhs commented Jan 31, 2020

I use 3.1.x for real project. if it's good enough, we could adapt it for 3.1.x for boosting performance. :)
many users would be happy with it.

@lawnjelly
Copy link
Member Author

Closing for now as superseded by batching PR #35957 which is much faster.

@lawnjelly lawnjelly closed this Feb 7, 2020
@clayjohn
Copy link
Member

clayjohn commented Feb 7, 2020

@lawnjelly This is still relevant for code paths that dont take advantage of batching and for users that disable batching.

@arkology
Copy link
Contributor

arkology commented Feb 7, 2020

I also think it's still relevant. Because godot really need performance.

@lawnjelly
Copy link
Member Author

Ah ok sorry, I'll leave open. 👍

@Calinou
Copy link
Member

Calinou commented Feb 2, 2021

Is this PR still relevant following the merge of unified GLES3/GLES2 batching?

@lawnjelly
Copy link
Member Author

I'd be in favour of closing it and just archiving it for reference. Few people should be using the legacy path now, and the batching by nature has far fewer state changes.

@clayjohn
Copy link
Member

clayjohn commented Feb 2, 2021

I agree.

@lawnjelly lawnjelly closed this Feb 2, 2021
@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants