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

Avoid material rebind when using skeleton #37667

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Apr 7, 2020

Fixes #37660 For 3.2. Because of the nullptr change, I need a different PR for each.

Changes to use the same logic as GLES3

if (prev_skeleton != skeleton) {
if ((prev_skeleton == NULL) != (skeleton == NULL)) {
state.scene_shader.set_conditional(SceneShaderGLES3::USE_SKELETON, skeleton != NULL);
rebind = true;
}
if (skeleton) {
glActiveTexture(GL_TEXTURE0 + storage->config.max_texture_image_units - 1);
glBindTexture(GL_TEXTURE_2D, skeleton->texture);
}
}

Previously, every time a valid skeleton was used, a rebind would occur. Now, a rebind on occurs when the previous skeleton was null and the current is valid or vice versa (i.e. state changes only when state needs to change)

I am not sure why (prev_skeleton != skeleton) evaluates to true every frame, but I assume it has to do with how GLES2 treats skeletons, it seems to make a slightly different skeleton for each mesh instead of sharing one across many.

@clayjohn clayjohn added this to the 3.2 milestone Apr 7, 2020
@clayjohn clayjohn requested a review from reduz as a code owner April 7, 2020 17:44
@akien-mga akien-mga merged commit 02ed72c into godotengine:3.2 Apr 7, 2020
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

For 3.2. Because of the nullptr change, I need a different PR for each.

Note that nullptr is still supported on 3.2 (which builds with -std=gnu++14), so it would have been OK to cherry-pick the master commit too.

@clayjohn clayjohn deleted the GLES2-32-skeleton-rebind branch June 22, 2022 05:57
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.

2 participants