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

Use separate attribute elements for line normals #5073

Merged
merged 2 commits into from
Aug 8, 2017
Merged

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented Aug 1, 2017

Broadcom GPUs don't cope well with using the least significant bit for this.

Refs mapbox/mapbox-gl-native#7583.

@jfirebaugh jfirebaugh requested a review from ansis August 1, 2017 17:38
@lbud
Copy link
Contributor

lbud commented Aug 1, 2017

Given that the line program is one of the programs that has hit snags by exceeding GL_MAX_VERTEX_ATTRIBS on some hardware, should we try to minimize space used here at least by packing the two normal elements into one number?

@jfirebaugh
Copy link
Contributor Author

Good thought, but this PR doesn't increase the number of attributes -- it uses the previously unused z and w components of an existing attribute. Packing both bits into a single component is possible, and would reduce the buffer size slightly, but it wouldn't affect the attribute count, and I'm wary of bitpacking now that I've seen it fail on at least one GPU.

@lbud
Copy link
Contributor

lbud commented Aug 1, 2017

Aha — I had skimmed something somewhere that said

Arrays of vertex shader inputs (vertex attributes) are supported, but each element of the array counts against your GL_MAX_VERTEX_ATTRIBS limit.

but I see now based on official docs ("the maximum number of 4-component generic vertex attributes accessible to a vertex shader") that that source is either mistaken or poorly worded.

@jfirebaugh jfirebaugh requested a review from lbud August 2, 2017 00:53
@mourner
Copy link
Member

mourner commented Aug 2, 2017

I'm worried that we once again increase the amount of data uploaded to the GPU, this time by 50% for line layers (which are by far the heaviest in our maps).

@jfirebaugh
Copy link
Contributor Author

@mourner Do you have an alternative in mind?

Copy link
Contributor

@lbud lbud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐

Broadcom GPUs don't cope well with using the least significant bit for this.
@jfirebaugh
Copy link
Contributor Author

Ok, going to go with adding a single extra component -- tested, works on device, only increases size by 25%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants