This repository has been archived by the owner on Aug 8, 2023. It is now read-only.
[core] Reduce number of vertex attributes passed to shaders #8267
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mollymerp, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh to be potential reviewers. |
mollymerp
changed the title
|
Couple outstanding TODOs:
|
|
When source functions are used, this implementation will create vertex buffers that contain both min and max values, when only the min value is used. In other words, the buffers are twice as big as they need to be. We discussed an implementation where the vertex buffer contains only the min value, even though the shader continues to use a |
|
Ah, yep @jfirebaugh meant to put that on the todo list -- updated now. |
anandthakker
force-pushed
the
fewer-vertex-attrs
branch
from
5ea3d41 to
7c1b63b
Compare
anandthakker
changed the base branch from
master
to
release-ios-v3.5.0-android-v5.0.0
anandthakker
force-pushed
the
fewer-vertex-attrs
branch
from
7c1b63b to
4b25352
Compare
jfirebaugh
reviewed
Mar 8, 2017
src/mbgl/programs/attributes.hpp
Outdated
| static auto name() { | ||
| static const std::string name = Attr::name() + std::string("_max"); | ||
| static const std::string name = Attr::name(); |
There was a problem hiding this comment.
This can return Attr::name() directly.
src/mbgl/programs/attributes.hpp
Outdated
| static auto name() { return "a_stroke_color"; } | ||
|
|
||
There was a problem hiding this comment.
Nit: whitespace changes.
jfirebaugh
approved these changes
Mar 8, 2017
anandthakker
force-pushed
the
fewer-vertex-attrs
branch
from
05a2ff9 to
6d2d7b4
Compare
anandthakker
changed the base branch from
release-ios-v3.5.0-android-v5.0.0
to
master
|
Rebased back to master. Because of #8316 -- and the fact that we had multiple commits that modified the shaders, it was easier to first squash to a single commit before rebasing. (The history wasn't especially useful in this case anyway.) |
jfirebaugh
force-pushed
the
fewer-vertex-attrs
branch
from
be7a504 to
46d4653
Compare
Some devices supported by Mapbox GL provide only 8 vertex attributes; this change packs existing attributes to get us just under that limit. For properties using a composite function, pack the min and max values into a single attribute with two logical components instead of using two separate attributes and buffers. Special logic is included for color attributes, whose integer components must be packed into the available bits of floating-point attributes. (We don't have access to ivec types in GL ES 2.0.) For source functions, continue to bind just a one-component attribute even though the GLSL type is vec2 (or vec4 for colors). The type-checking done by gl::Attribute is relaxed slightly to accommodate this.
jfirebaugh
force-pushed
the
fewer-vertex-attrs
branch
from
46d4653 to
03f770c
Compare
3 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #8183
related gl-js PR: mapbox/mapbox-gl-js#4363
@anandthakker and I worked on this for the past couple days 🤖
cc @jfirebaugh