-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Introduce ProgramConfiguration #3439
Conversation
3d00695
to
7183863
Compare
I've read through this code and am 👍 on all the changes from a technical perspective. From an architectural perspective, I'm not sold that this architecture is better than the previous one. Simple well-defined interfaces (like the old flat object approach to Could you provide more context on the motivation behind this PR? |
The approach to Prior to this PR, there was hidden coupling between
|
I see your point. The interface of the data shared between these methods is undocumented and unenforced. This is an instance of "data coupling" both before and after the refactor. There is no distinction made in general CS literature between data coupling caused by an "interface" and that caused by an instance of a "class". My opinion is that coupling caused by an interface is preferable. My plan for implementing #145 involves creating more "interface coupling" and less "class coupling" in our rendering pipeline. All that said, I respect your design sense and appreciate that you have additional PRs blocked on this one getting merged. I am 👍 on the code and have spoken my architectural concerns. |
ProgramConfiguration contains code extracted from Bucket, Painter, and a few other key places, so that the logic implementing the shader #pragma system is located in a single place. From the JSDoc: ProgramConfiguration contains the logic for binding style layer properties and tile layer feature data into GL program uniforms and vertex attributes. Non-data-driven property values are bound to shader uniforms. Data-driven property values are bound to vertex attributes. In order to support a uniform GLSL syntax over both, [Mapbox GL Shaders](https://github.com/mapbox/mapbox-gl-shaders) defines a `#pragma` abstraction, which ProgramConfiguration is responsible for implementing. At runtime, it examines the attributes of a particular layer, combines this with fixed knowledge about how layers of the particular type are implemented, and determines which uniforms and vertex attributes will be required. It can then substitute the appropriate text into the shader source code, create and link a program, and bind the uniforms and vertex attributes in preparation for drawing.
7183863
to
cafd459
Compare
ProgramConfiguration
contains code extracted fromBucket
,Painter
, and a few other key places, so that the logic implementing the shader#pragma
system is located in a single place.From the JSDoc:
ProgramConfiguration
contains the logic for binding style layer properties and tile layer feature data into GL program uniforms and vertex attributes.Non-data-driven property values are bound to shader uniforms. Data-driven property values are bound to vertex attributes. In order to support a uniform GLSL syntax over both, Mapbox GL Shaders defines a
#pragma
abstraction, which ProgramConfiguration is responsible for implementing. At runtime, it examines the attributes of a particular layer, combines this with fixed knowledge about how layers of the particular type are implemented, and determines which uniforms and vertex attributes will be required. It can then substitute the appropriate text into the shader source code, create and link a program, and bind the uniforms and vertex attributes in preparation for drawing.Benchmarks
map-load
master 79e40c9: 143 ms
program-configuration 879150f: 89 ms
style-load
master 79e40c9: 105 ms
program-configuration 879150f: 100 ms
buffer
master 79e40c9: 1,106 ms
program-configuration 879150f: 1,099 ms
fps
master 79e40c9: 60 fps
program-configuration 879150f: 60 fps
frame-duration
master 79e40c9: 7 ms, 0% > 16ms
program-configuration 879150f: 7.3 ms, 1% > 16ms
query-point
master 79e40c9: 1.01 ms
program-configuration 879150f: 1.14 ms
query-box
master 79e40c9: 83.05 ms
program-configuration 879150f: 89.59 ms
geojson-setdata-small
master 79e40c9: 8 ms
program-configuration 879150f: 9 ms
geojson-setdata-large
master 79e40c9: 104 ms
program-configuration 879150f: 102 ms