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

Fixed vertex color initialization with default value in GLES3 #31270

Merged

Conversation

@pouleyKetchoupp
Copy link
Contributor

pouleyKetchoupp commented Aug 10, 2019

This change makes sure vertex color is initialized with white as a default value, the same way as for GLES2, so materials with 'Use as Albedo' flag don't render with the last used vertex color.

Fixes #30275, fixes #31250.

@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented Aug 10, 2019

I'm not sure this is the proper location. As you will still the bug on viewports that are set to 3D - effects disabled. In GLES2 glvertexattrib* is reset on every call to render_scene.

I think a proper solution would be to find where labels are setting vertex attributes and make sure the state is properly reset when they finish drawing.

@pouleyKetchoupp

This comment has been minimized.

Copy link
Contributor Author

pouleyKetchoupp commented Aug 10, 2019

@clayjohn Ok, I'll see if can find where to reset vertex attributes for labels instead if it's a better solution. But why not doing the same thing as for GLES2 and just reset on each call to render_scene to be safe? Is it going to cause specific issues with GLES3?

@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented Aug 10, 2019

It's not a big deal. If it is too difficult to determine where the state is being changed then resetting on each call to render_scene is fine.

Its just an extra API call which is a little bit of unnecessary overhead. In GLES3 the drivers tend to be more unpredictable, so having the extra API call could result in no performance impact, or it could result in certain operations hanging. In GLES2 things are more stable and the risk is much lower, so it changes state a lot more and is less concerned about avoiding API calls.

In my opinion, we should go through GLES2 and find things like that and clean them up as well. I think it would result in slight performance improvements.

@pouleyKetchoupp

This comment has been minimized.

Copy link
Contributor Author

pouleyKetchoupp commented Aug 11, 2019

The vertex color attribute for fonts is set in TYPE_RECT command in the canvas rendering:

glVertexAttrib4f(VS::ARRAY_COLOR, rect->modulate.r, rect->modulate.g, rect->modulate.b, rect->modulate.a);

So I can reset the values specifically in this spot after drawing the triangles, or in canvas_end() to make it a bit more generic since other commands are changing vertex color too (and possibly result in less calls to change attributes back and forth?).

Which option makes more sense to you?

@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented Aug 11, 2019

I would go with canvas_end.

Great work!

@pouleyKetchoupp

This comment has been minimized.

Copy link
Contributor Author

pouleyKetchoupp commented Aug 11, 2019

I'll do that! Thanks for the help :)

@pouleyKetchoupp pouleyKetchoupp force-pushed the nekomatata:fix-vertex-color-init-gles3 branch from c9f014c to e852b3a Aug 11, 2019
Copy link
Contributor

clayjohn left a comment

Looks good! Thanks!

@akien-mga akien-mga added this to the 3.2 milestone Aug 12, 2019
@akien-mga akien-mga merged commit 0e823cf into godotengine:master Aug 12, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Aug 12, 2019

Thanks!

@pouleyKetchoupp pouleyKetchoupp deleted the nekomatata:fix-vertex-color-init-gles3 branch Aug 12, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Nov 8, 2019

Cherry-picked for 3.1.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.