Skip to content

Commit

Permalink
Change how rects are drawn (and also will help batching eventually), …
Browse files Browse the repository at this point in the history 
…to workaround problem in #9913
  • Loading branch information
@reduz
reduz committed Dec 18, 2018
1 parent 3c999ae commit bec76cf
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 13 deletions.
86 changes: 78 additions & 8 deletions drivers/gles2/rasterizer_canvas_gles2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,6 @@ void RasterizerCanvasGLES2::_canvas_item_render_commands(Item *p_item, Item *cur
Item::CommandLine *line = static_cast<Item::CommandLine *>(command);

state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, false);
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_UV_ATTRIBUTE, false);
if (state.canvas_shader.bind()) {
_set_uniforms();
state.canvas_shader.use_material((void *)p_material);
Expand Down Expand Up @@ -391,10 +390,86 @@ void RasterizerCanvasGLES2::_canvas_item_render_commands(Item *p_item, Item *cur
glDisableVertexAttribArray(VS::ARRAY_COLOR);
glVertexAttrib4fv(VS::ARRAY_COLOR, r->modulate.components);

#if 1

This comment has been minimized.

Sign in to view

Copy link
@jahd2602

jahd2602 Dec 18, 2018

Contributor

why is this needed?

This comment has been minimized.

Sign in to view

Copy link
@akien-mga

akien-mga Dec 18, 2018

Member

To conditionally disable the older code in the #else branch.

This comment has been minimized.

Sign in to view

Copy link
@jahd2602

jahd2602 Dec 18, 2018

Contributor

Is this a common practice in the godot codebase?

Git preserves the history of the code so I don't see how preserving it in an #else is necessary.

This comment has been minimized.

Sign in to view

Copy link
@girng

girng Dec 19, 2018 •
edited
Loading

probably helpful to see the code comparatively, because of the intricacies of the bug. (or whoever tries to the tackle the bug in the future, if issues arise later)

This comment has been minimized.

Sign in to view

Copy link
@jahd2602

jahd2602 Dec 20, 2018

Contributor

Ok, if this is how things are done here then it is just a matter of getting used to it. It just kills the whole purpose of git and the whole Version Control Systems concept. If we follow the same logic then we should just keep all the git history in the files in commented lines.

This comment has been minimized.

Sign in to view

Copy link
@girng

girng Dec 20, 2018 •
edited
Loading

@jahd2602 no no. i totally understand what you mean :-), i was just posting on why i personally think it might have been left in. akien/reduz/lead devs have final say, it just is what it is.

This comment has been minimized.

Sign in to view

Copy link
@jahd2602

jahd2602 Dec 20, 2018

Contributor

Oh, I get it. Thanks to both for your kind explanation.

This comment has been minimized.

Sign in to view

Copy link
@akien-mga

akien-mga Dec 21, 2018

Member

Version control does preserve history and allow to get old code back, but that doesn't mean that it makes it easy. Git is a complex tool and I'm sure 80% of our contributors don't know how to restore code that has been deleted 6 months ago without spending hours fighting merge conflicts.

And here, the older code is kept for documentation purposes, and because it's superior to the new code. The new code (#if 1) is only used to workaround a Nvidia driver bug, and it seems to have caused big performance regressions (#24466), so it was definitely a valid decision to keep it around for now, as a mix of both branches might be needed eventually.

This comment has been minimized.

Sign in to view

Copy link
@jahd2602

jahd2602 Dec 21, 2018

Contributor

Yes, using the command line interface of git is hard, but it is easy to click in these buttons:
image
But also, what about using branches for this kind of unconfirmed changes that may break (as it did)? I have helped other godot contributors by compiling their branches and testing before they submitted a pull request.

This comment has been minimized.

Sign in to view

Copy link
@akien-mga

akien-mga Dec 21, 2018 •
edited
Loading

Member

Browsing history and restoring old code are very different thing. If a file has 500 commits, where do you find the commit which removed that code? How do you even know that this code used to exist? Blame doesn't tell you that, some command line tricks can do reverse blame and show when previous code was removed, but that's the kind of features that maybe only 10% of our devs know how to use.

Again, being able to do something doesn't mean that it's easy. And quite often, it makes sense to keep some code temporarily for documentation or testing purposes before removing it proper.

This comment has been minimized.

Sign in to view

Copy link
@akien-mga

akien-mga Dec 21, 2018

Member

And if that code had been pushed to a branch, well, we likely wouldn't know by now that it poses performance issues (which were also not expected, it's not like this was committed as a test).

This comment has been minimized.

Sign in to view

Copy link
@jahd2602

jahd2602 Dec 21, 2018

Contributor

I get it. Thank you. I suppose the commented-out lines of this file will be removed when Godot reaches stable.
//more compatible
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, false);

if (state.canvas_shader.bind()) {
_set_uniforms();
state.canvas_shader.use_material((void *)p_material);
}

Size2 abs_size = r->rect.size.abs();
Vector2 points[4] = {
r->rect.position,
r->rect.position + Vector2(abs_size.x, 0.0),
r->rect.position + abs_size,
r->rect.position + Vector2(0.0, abs_size.y),
};

if (r->rect.size.x < 0) {
SWAP(points[0], points[1]);
SWAP(points[2], points[3]);
}
if (r->rect.size.y < 0) {
SWAP(points[0], points[3]);
SWAP(points[1], points[2]);
}

RasterizerStorageGLES2::Texture *texture = _bind_canvas_texture(r->texture, r->normal_map);

if (texture) {
Size2 texpixel_size(1.0 / texture->width, 1.0 / texture->height);

Rect2 src_rect = (r->flags & CANVAS_RECT_REGION) ? Rect2(r->source.position * texpixel_size, r->source.size * texpixel_size) : Rect2(0, 0, 1, 1);

Vector2 uvs[4] = {
src_rect.position,
src_rect.position + Vector2(src_rect.size.x, 0.0),
src_rect.position + src_rect.size,
src_rect.position + Vector2(0.0, src_rect.size.y),
};

if (r->flags & CANVAS_RECT_FLIP_H) {
SWAP(uvs[0], uvs[1]);
SWAP(uvs[2], uvs[3]);
}
if (r->flags & CANVAS_RECT_FLIP_V) {
SWAP(uvs[0], uvs[3]);
SWAP(uvs[1], uvs[2]);
}

if (r->flags & CANVAS_RECT_TRANSPOSE) {
SWAP(uvs[1], uvs[3]);
}

state.canvas_shader.set_uniform(CanvasShaderGLES2::COLOR_TEXPIXEL_SIZE, texpixel_size);

bool untile = false;

if (r->flags & CANVAS_RECT_TILE && !(texture->flags & VS::TEXTURE_FLAG_REPEAT)) {
glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_REPEAT);
glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_REPEAT);
untile = true;
}

_draw_gui_primitive(4, points, NULL, uvs);

if (untile) {
glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
}
} else {
state.canvas_shader.set_uniform(CanvasShaderGLES2::COLOR_TEXPIXEL_SIZE, Vector2());
_draw_gui_primitive(4, points, NULL, NULL);
}

#else
//disabled because it fails on buggy nvidia drivers
_bind_quad_buffer();
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, true);
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_UV_ATTRIBUTE, false);
if (state.canvas_shader.bind()) {
_set_uniforms();
state.canvas_shader.use_material((void *)p_material);
Expand Down Expand Up @@ -469,15 +544,14 @@ void RasterizerCanvasGLES2::_canvas_item_render_commands(Item *p_item, Item *cur
glBindBuffer(GL_ARRAY_BUFFER, 0);
glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);

#endif
} break;

case Item::Command::TYPE_NINEPATCH: {

Item::CommandNinePatch *np = static_cast<Item::CommandNinePatch *>(command);

state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, false);
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_UV_ATTRIBUTE, true);
if (state.canvas_shader.bind()) {
_set_uniforms();
state.canvas_shader.use_material((void *)p_material);
Expand Down Expand Up @@ -641,7 +715,6 @@ void RasterizerCanvasGLES2::_canvas_item_render_commands(Item *p_item, Item *cur
Item::CommandCircle *circle = static_cast<Item::CommandCircle *>(command);

state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, false);
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_UV_ATTRIBUTE, false);

if (state.canvas_shader.bind()) {
_set_uniforms();
Expand Down Expand Up @@ -672,7 +745,6 @@ void RasterizerCanvasGLES2::_canvas_item_render_commands(Item *p_item, Item *cur
Item::CommandPolygon *polygon = static_cast<Item::CommandPolygon *>(command);

state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, false);
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_UV_ATTRIBUTE, true);

if (state.canvas_shader.bind()) {
_set_uniforms();
Expand All @@ -693,7 +765,6 @@ void RasterizerCanvasGLES2::_canvas_item_render_commands(Item *p_item, Item *cur
Item::CommandPolyLine *pline = static_cast<Item::CommandPolyLine *>(command);

state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, false);
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_UV_ATTRIBUTE, false);

if (state.canvas_shader.bind()) {
_set_uniforms();
Expand Down Expand Up @@ -726,7 +797,6 @@ void RasterizerCanvasGLES2::_canvas_item_render_commands(Item *p_item, Item *cur

Item::CommandPrimitive *primitive = static_cast<Item::CommandPrimitive *>(command);
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, false);
state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_UV_ATTRIBUTE, true);

if (state.canvas_shader.bind()) {
_set_uniforms();
Expand Down
1 change: 0 additions & 1 deletion drivers/gles2/rasterizer_gles2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,6 @@ void RasterizerGLES2::blit_render_target_to_screen(RID p_render_target, const Re
ERR_FAIL_COND(!rt);

canvas->state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_TEXTURE_RECT, true);
canvas->state.canvas_shader.set_conditional(CanvasShaderGLES2::USE_UV_ATTRIBUTE, false);

canvas->state.canvas_shader.set_custom_shader(0);
canvas->state.canvas_shader.bind();
Expand Down
5 changes: 1 addition & 4 deletions drivers/gles2/shaders/canvas.glsl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,8 @@ void main() {
#else
vec4 outvec = vec4(vertex.xy, 0.0, 1.0);

#ifdef USE_UV_ATTRIBUTE
uv_interp = uv_attrib;
#else
uv_interp = vertex.xy;
#endif


#endif

Expand Down

0 comments on commit bec76cf

Please sign in to comment.