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

Batch GLES2 draw calls #20965

Merged
merged 1 commit into from
Aug 18, 2018
Merged

Batch GLES2 draw calls #20965

merged 1 commit into from
Aug 18, 2018

Conversation

dragmz
Copy link
Contributor

@dragmz dragmz commented Aug 13, 2018

Fixes #19917

Adds GLES2 draw calls batching for the same render list item that uses
multiple rasterizer commands (e.g. Label node; a node with multiple
GDScript draw_* calls).

This does not cover glBufferSubData calls batching that I've tried to fix in #20077.

Some stats for the number of draw calls -

Before:

image

After:

image

}

return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could just have:

return (active != this || !version || new_conditional_version.key != conditional_version.key);

(Just a very little thing in what looks like a really great PR, can't wait to see the speedup)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this code smell, I'll clean it up.

Adds GLES2 draw calls batching for the same render list item that uses
multiple rasterizer commands (e.g. Label node; a node with multiple
GDScript draw_* calls).
@reduz
Copy link
Member

reduz commented Aug 15, 2018

Looks good to me, @karroffel ?

Copy link
Contributor

@karroffel karroffel 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!

buffer[(3 * 4 * 4) + 14] = (source.position.x + source.size.x) * texpixel_size.x;
buffer[(3 * 4 * 4) + 15] = (source.position.y + source.size.y) * texpixel_size.y;
const bool p_singlecolor = pline->triangle_colors.size() == 1;
const Color *p_colors = pline->triangle_colors.ptr();
Copy link
Contributor

Choose a reason for hiding this comment

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

Those two shouldn't have a p_ as a prefix, since those are used for parameters, not local variables. Also p_singlecolor is unused.

if (pline->triangles.size()) {
const int p_vertex_count = pline->triangles.size();
const int p_triangle_count = p_vertex_count - 2;
const int p_index_count = p_triangle_count * 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

All those shouldn't have the p_ prefix. In Godot's naming convention we use that only for parameter declarations, not local variables. This happens in multiple places, but it's actually not a big deal and doens't block this PR in the slightest :)

@akien-mga
Copy link
Member

Just mentioning here for clarity that this was reverted in #21204.

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.

Text rendering performance low due to missing batching
6 participants