This repository has been archived by the owner on Aug 8, 2023. It is now read-only.
Bind only active attributes in order to avoid exceeding attribute limits #9373
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
added 2 commits
June 26, 2017 15:23
jfirebaugh
suggested changes
Jun 26, 2017
src/mbgl/gl/attribute.hpp
Outdated
| } | ||
|
|
||
| return Locations{ bindAttributeLocation( | ||
| id, locationToBindAttribute(activeAttributes, As::name()), As::name())... }; |
There was a problem hiding this comment.
This is close, but we want to number them in the order defined by the As argument pack, rather than lexicographical order by name. The latter might put an attribute that's sometimes disabled at location 0, which as we know, causes issues. Try this:
AttributeLocation location = 0;
return Locations {
(activeAttributes.count(As::name()) ? bindAttributeLocation(id, location++, As::name()) : -1)...
};
src/mbgl/gl/attribute.hpp
Outdated
| @@ -242,7 +247,18 @@ class Attributes { | |||
| static constexpr std::size_t Index = TypeIndex<A, As...>::value; | |||
|
|
|||
| static Locations bindLocations(const ProgramID& id) { | |||
| return Locations { bindAttributeLocation(id, Index<As>, As::name())... }; | |||
| std::set<std::string> activeAttributes; | |||
There was a problem hiding this comment.
You can minimize the declarations and code here in the header by writing:
std::set<std::string> activeAttributes = getActiveAttributes(id);Then getActiveAttributes is the only thing that needs to be declared; everything else can go in the .cpp.
src/mbgl/gl/attribute.cpp
Outdated
| namespace mbgl { | ||
| namespace gl { | ||
|
|
||
| AttributeLocation bindAttributeLocation(ProgramID id, AttributeLocation location, const char* name) { | ||
| MBGL_CHECK_ERROR(glBindAttribLocation(id, location, name)); |
There was a problem hiding this comment.
Add assert(location <= 8) for good measure.
|
@lbud Currently crashes on android devices: |
lbud
changed the title
added 7 commits
June 27, 2017 15:28
…d as many attributes as are assigned locations
ivovandongen
approved these changes
Jun 28, 2017
lbud
changed the title
|
One small thing I wanted to flag is that is that I had to make |
jfirebaugh
reviewed
Jun 29, 2017
src/mbgl/gl/attribute.hpp
Outdated
| @@ -255,8 +266,9 @@ class Attributes { | |||
| } | |||
|
|
|||
| template <class DrawMode> | |||
| static Bindings bindings(const VertexBuffer<Vertex, DrawMode>& buffer) { | |||
There was a problem hiding this comment.
Does anything actually fail if you don't make this change? In theory, this function is called only for active attributes. If it's called for an attribute that isn't actually present, then there's a bug somewhere I think.
There was a problem hiding this comment.
🤔 @jfirebaugh good question — I added it after this build, which was terminated during the render tests with
terminate called after throwing an instance of 'mbgl::gl::Error'
what(): glEnableVertexAttribArray(location): Error GL_INVALID_VALUE at ../../../src/mbgl/gl/attribute.cpp:62
(glEnableVertexAttribArray documentation)
But I was able to reproduce that error locally at the time using a fill-color property function, which now works locally if I remove it… I'll remove it and see if any of the CI builds still have issues.
There was a problem hiding this comment.
Actually, I spoke too soon — locally I get the same error:
libc++abi.dylib: terminating with uncaught exception of type mbgl::gl::Error: glEnableVertexAttribArray(location): Error GL_INVALID_VALUE at /Users/lbud/src/mapbox-gl-native/src/mbgl/gl/attribute.cpp:70
jfirebaugh
approved these changes
Jun 29, 2017
There was a problem hiding this comment.
Ah, the attempt to bind at location -1 comes from the FillProgram, which includes fill-outline-color in its paint properties, but doesn't use it or have an a_outline_color attribute. Can you add a comment to the if (location == -1) return; explaining this? Looks good otherwise!
20 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.
I think this fixes #9331, although I'm not sure how to go about testing on an older Android device.
This PR
uniformsStateandattributeLocationsin theProgramconstructor, so that the program is initially linked before callingattributeLocations,Attributes::bindLocationsto query active attributes, temporarily store a set of those attributes, and then manually bind only the active attributes, and