[core] update shaders for line property functions #6658

mollymerp · 2016-10-11T17:57:02Z

attempting to make native work with the shaders update at mapbox/mapbox-gl-shaders#24

Currently the macOS build is 'succeeding' but it freezes/times out and the actual app never opens so I'm definitely doing something wrong -- possibly that you can't #define macros like that in OpenGL or something? 🔧 fixed

~~I'm planning on having the device pixel ratio not be hard-coded in the build-shaders script, but I figured I'd try to get that working as a first step 💭~~ 🔧 fixed

cc @lucaswoj

mention-bot · 2016-10-11T17:57:04Z

@mollymerp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kkaefer, @jfirebaugh and @ansis to be potential reviewers.

lucaswoj · 2016-10-11T18:54:16Z

scripts/build-shaders.js

@@ -33,12 +33,15 @@ var data = data.split('\n').map(function(line) {
        } else {
            return params[1] + params[3] + 'p ' + params[4] + ' ' + params[5] + ' = u_' + params[5] + ';';
        }
+    } else if (line.match(pixel_ratio_regex)){
+        return '\n#define DEVICE_PIXEL_RATIO 2.0\n' + line;


This line replaces any occurrences of /(.*)#define(.*)DEVICE_PIXEL_RATIO(.*)/ in the shader source with #define DEVICE_PIXEL_RATIO 2.0. Are there occurrences of /(.*)#define(.*)DEVICE_PIXEL_RATIO(.*)/ in the shader source?

It actually inserts the hardcoded definition before (#define DEVICE_PIXEL_RATIO 2.0) before the matching line in the line-*.vertex shaders. so the output is

#define DEVICE_PIXEL_RATIO 2.0 #define ANTIALIASING 1.0 / DEVICE_PIXEL_RATIO / 2.0

I've confirmed the build-shaders script is producing the .hpp shaders with the expected result but at least one of the shaders is not compiling and I can't find a helpful error message ☹️

@mollymerp There are some shaders which use the DEVICE_PIXEL_RATIO outside of #define statements. I suggest following the precedent set in GL JS and adding this #define to the top of all shaders.

mollymerp · 2016-10-11T20:27:34Z

Ok I fixed a bug in mapbox/mapbox-gl-shaders#24 and now everything is compiling/building, and I think all I have left to do is figure out how to access view.getPixelRatio() from src/mbgl/gl/shader.cpp so that I can dynamically #define the device pixel ratio instead of hard-coding it at 2.0.

mollymerp · 2016-11-03T22:41:28Z

Soooo this is more or less working, but the render tests show that (I think) antialiasing is messed up and maybe color too? I'll need to investigate that more.

kkaefer · 2016-11-04T10:31:17Z

src/mbgl/gl/shader.cpp

    : name(name_),
      program(context.createProgram()),
      vertexShader(context.createVertexShader()),
      fragmentShader(context.createFragmentShader()) {
    util::stopwatch stopwatch("shader compilation", Event::Shader);

-    if (!compileShader(vertexShader, vertexSource)) {
+    std::string vertex(vertexSource);
+    vertex.replace(vertex.find_first_of('\n'), 1,"\n#define DEVICE_PIXEL_RATIO " + std::to_string(shaderParameters.pixelRatio) + "\n");


I'm not sure hard-coding the device pixel ratio is a good idea. On desktop, we can switch windows between screens with differing pixel ratio. We don't reload any sprites, but we still do a best effort to render them at the correct size.

I'll take that back; we are initializing Map with a constant pixel ratio that can't be changed after the fact anyway.

kkaefer · 2016-11-04T10:31:39Z

src/mbgl/gl/shader.cpp

    std::string fragment(fragmentSource);
-    if (defines & Defines::Overdraw) {
+    fragment.replace(fragment.find_first_of('\n'), 1,"\n#define DEVICE_PIXEL_RATIO "+ std::to_string(shaderParameters.pixelRatio) + "\n");
+    if ((strcmp(name, "collision_box") != 0) & shaderParameters.overdraw) {


Why is this hard-coded?

Because I couldn't figure out how to modify things such that the collision_box_shader is always initialized with overdraw = false 😳

kkaefer · 2016-11-04T10:33:54Z

src/mbgl/renderer/painter.cpp

+    #ifndef NDEBUG
+        shaderParametersOverdraw = gl::ShaderParameters{ frame.pixelRatio, true };
+        overdrawShaders = std::make_unique<Shaders>(context, shaderParametersOverdraw);
+    #endif


#if/endif should not be indented; and the code it surrounds needs to be unindented. You can use clang-format to apply our standard code formatting.

kkaefer · 2016-11-04T10:34:53Z

src/mbgl/renderer/painter.cpp

+        shaders = std::make_unique<Shaders>(context, shaderParameters);
+    #ifndef NDEBUG
+        shaderParametersOverdraw = gl::ShaderParameters{ frame.pixelRatio, true };
+        overdrawShaders = std::make_unique<Shaders>(context, shaderParametersOverdraw);


Why did you move this to the render() method? This means the shaders are initialized on every frame, which isn't efficient.

When I put it originally in the Painter initialization, DEVICE_PIXEL_RATIO was getting set to 0 all the time 😬

kkaefer · 2016-11-04T10:35:29Z

src/mbgl/renderer/painter.hpp

@@ -151,6 +153,8 @@ class Painter : private util::noncopyable {
    LineAtlas* lineAtlas = nullptr;

    FrameHistory frameHistory;
+    gl::ShaderParameters shaderParameters;
+    gl::ShaderParameters shaderParametersOverdraw;


ShaderParameters are only used to initialize the shaders; there's no need to keep them around.

kkaefer · 2016-11-04T10:37:13Z

src/mbgl/gl/shader.hpp

@@ -31,7 +32,7 @@ class Shader : private util::noncopyable {
           const char* vertex,
           const char* fragment,
           Context&,
-           Defines defines = Defines::None);


Since Defines isn't used anymore, you should also remove the enum definition.

kkaefer · 2016-11-04T10:43:01Z

There seem to be a few commits in here that track your progress rather than individual self-contained changes (and some commented out code added, then removed subsequently). Can you please merge/split commits so that the changes represent self-contained units? Our general rule of thumb is that every commit in gl-native should have all tests passing. This helps bigly when using git bisect to find regressions later on.

fixed!

mollymerp · 2016-11-04T14:32:28Z

Thanks for the review @kkaefer ! I will squash my commits when this is closer to merging because I don't think any of these commits have all tests passing so far.

mollymerp · 2016-11-11T21:36:08Z

Ok I think this is finally ready for another glance @kkaefer @jfirebaugh 👀

jfirebaugh

Looking good, pretty much just formatting/style nits at this point.

jfirebaugh · 2016-11-11T22:23:48Z

package.json

@@ -20,10 +20,9 @@
    "csscolorparser": "^1.0.2",
    "ejs": "^2.4.1",
    "express": "^4.11.1",
-    "lodash": "^4.16.4",


Revert the lodash removal -- this is still used in a couple places.

jfirebaugh · 2016-11-11T22:23:59Z

src/mbgl/gl/context.cpp

@@ -40,7 +40,7 @@ Context::~Context() {

 UniqueShader Context::createShader(ShaderType type, const std::string& source) {
    UniqueShader result { MBGL_CHECK_ERROR(glCreateShader(static_cast<GLenum>(type))), { this } };
-
+    


Revert the whitespace changes in this file.

jfirebaugh · 2016-11-11T22:25:09Z

src/mbgl/programs/line_program.cpp

@@ -12,13 +12,11 @@ static_assert(sizeof(LineAttributes::Vertex) == 8, "expected LineVertex size");

 template <class Values, class...Args>
 Values makeValues(const style::LinePaintProperties& properties,
-                  float pixelRatio,
                  const RenderTile& tile,
                  const TransformState& state,
                  Args&&... args) {
    // the distance over which the line edge fades out.


Remove the comments here; they were attached to the line that was deleted.

jfirebaugh · 2016-11-11T22:27:38Z

src/mbgl/programs/program.hpp

        std::string fragment = Shaders::fragmentSource;
-        if (defines == ProgramDefines::Overdraw) {
+
+        fragment.replace(fragment.find_first_of('\n'), 1,"\n#define DEVICE_PIXEL_RATIO "+ std::to_string(programParameters.pixelRatio) + "\n");


What's the purpose of finding the first newline here? Could it instead simply prepend "#define DEVICE_PIXEL_RATIO ...\n" to the source?

jfirebaugh · 2016-11-11T22:28:08Z

src/mbgl/programs/programs.hpp

-          symbolGlyph(context, defines),
-          debug(context, ProgramDefines::None),
-          collisionBox(context, ProgramDefines::None) {
+    Programs(gl::Context& context, ProgramParameters& programParameters)


const ProgramParameters&

jfirebaugh · 2016-11-11T22:28:22Z

src/mbgl/programs/programs.hpp


 namespace mbgl {

+
+


Revert this added whitespace.

jfirebaugh · 2016-11-11T22:28:40Z

src/mbgl/renderer/painter.cpp

@@ -21,6 +22,7 @@
 #include <mbgl/geometry/line_atlas.hpp>
 #include <mbgl/text/glyph_atlas.hpp>

+#include <mbgl/programs/program_parameters.hpp>


Duplicate #include.

jfirebaugh · 2016-11-11T22:31:39Z

src/mbgl/programs/program_parameters.hpp

+
+namespace mbgl {
+
+class ProgramParameters {


Formatting nits:

class ProgramParameters { public: ProgramParameters(float pixelRatio_ = 1.0, bool overdraw_ = false) : pixelRatio(pixelRatio_), overdraw(overdraw_) {} float pixelRatio; bool overdraw; };

jfirebaugh · 2016-11-11T22:32:31Z

src/mbgl/renderer/painter.hpp

@@ -151,6 +152,7 @@ class Painter : private util::noncopyable {

    FrameHistory frameHistory;

+


Revert whitespace change.

jfirebaugh · 2016-11-11T22:34:19Z

src/mbgl/renderer/painter_circle.cpp

-                : pixelsToGLUnits },
-            uniforms::u_devicepixelratio::Value{ frame.pixelRatio },
-        },
+                : pixelsToGLUnits }        },


Formatting nit:

: pixelsToGLUnits } }, *bucket.vertexBuffer,

mollymerp · 2016-11-12T00:18:16Z

Thanks @jfirebaugh -- fixed and squashed!

mollymerp · 2016-11-12T00:22:31Z

Launch steps:

merge shaders PR Add support for line paint property functions mapbox-gl-shaders#24
merge test-suite PR Add support for line paint property functions mapbox-gl-test-suite#136
update SHAs in native + gl-js Add support for line property functions mapbox-gl-js#3033
merge native + gl-js

jfirebaugh · 2016-11-12T00:57:37Z

src/mbgl/programs/program.hpp

        std::string fragment = Shaders::fragmentSource;
-        if (defines == ProgramDefines::Overdraw) {
+
+        fragment.insert(0, "\n#define DEVICE_PIXEL_RATIO "+ std::to_string(programParameters.pixelRatio) + "\n");


static std::string pixelRatioDefine(const ProgramParameters& parameters) { return std::string("#define DEVICE_PIXEL_RATIO ") + std::to_string(parameters.pixelRatio) + "\n"; } static std::string fragmentSource(const ProgramParameters& parameters) { std::string source = pixelRatioDefine(parameters) + Shaders::fragmentSource; ... } static std::string vertexSource(const ProgramParameters& parameters) { return pixelRatioDefine(parameters) + Shaders::vertexSource; }

mollymerp · 2016-11-15T23:48:12Z

@jfirebaugh ✅

jfirebaugh · 2016-11-15T23:51:52Z

@kkaefer Have all your requested changes been addressed?

kkaefer

Yup!

kkaefer · 2016-11-16T15:56:31Z

src/mbgl/renderer/painter.cpp

@@ -86,9 +87,12 @@ Painter::Painter(gl::Context& context_, const TransformState& state_)
    gl::debugging::enable();
 #endif

-    programs = std::make_unique<Programs>(context);
+    ProgramParameters programParameters = ProgramParameters{ pixelRatio, false };


Can be shortened to ProgramParameters programParameters{ pixelRatio, false };

update deps, define device pixel ratio for all shaders [core] create ShaderParameter struct to store pixel ratio and overdraw param repair rebase errs update shaders to include pixel ratio make sure collision_box never overdraws update test suite, move shaders to Painter::render so the correct pixel ratio is applied move shader compiling back to the Painter constructor rebase from shader --> program refactor re-factor parameters for collisionBox and debug programs remove unused vars from line-program, move blur math to shader update core files remove unecessary files update shaders PR, remove comments bump test suite sha fix formatting, incorporate feedback refactor program.hpp

…mp shader sha

kkaefer · 2016-12-05T09:16:15Z

Looks like the mapbox-gl-shaders repository branch didn't get into master after this branch got merged?

mollymerp added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Oct 11, 2016

mollymerp added the Core The cross-platform C++ core, aka mbgl label Oct 11, 2016

mollymerp changed the title ~~update shaders for line property functions~~ [core] update shaders for line property functions Oct 11, 2016

lucaswoj previously requested changes Oct 11, 2016

View reviewed changes

mollymerp force-pushed the dpr branch from e7141b5 to a491c83 Compare November 3, 2016 20:11

kkaefer suggested changes Nov 4, 2016

View reviewed changes

mollymerp force-pushed the dpr branch 3 times, most recently from 51871b7 to 162da32 Compare November 11, 2016 01:35

jfirebaugh suggested changes Nov 11, 2016

View reviewed changes

mollymerp force-pushed the dpr branch 2 times, most recently from 740dced to 4e6da9e Compare November 12, 2016 00:16

jfirebaugh suggested changes Nov 12, 2016

View reviewed changes

mollymerp force-pushed the dpr branch 2 times, most recently from 1e221cd to 91c64d4 Compare November 15, 2016 22:23

jfirebaugh approved these changes Nov 15, 2016

View reviewed changes

jfirebaugh removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Nov 15, 2016

kkaefer approved these changes Nov 16, 2016

View reviewed changes

mollymerp force-pushed the dpr branch from 270ca4f to 50330b9 Compare November 16, 2016 20:25

Molly Lloyd added 4 commits November 16, 2016 16:59

[core] remove line transformations that were moved to the shaders, bu…

6a0172b

…mp shader sha

[core] shorten ProgramParameter instantiation

47abdd3

[core] bump shader+test suite shas

2b1c942

mollymerp force-pushed the dpr branch from 6c1f7d3 to 2b1c942 Compare November 17, 2016 01:00

mollymerp merged commit 3380f08 into master Nov 17, 2016

mollymerp deleted the dpr branch November 17, 2016 01:34

		@@ -40,7 +40,7 @@ Context::~Context() {

		UniqueShader Context::createShader(ShaderType type, const std::string& source) {
		UniqueShader result { MBGL_CHECK_ERROR(glCreateShader(static_cast<GLenum>(type))), { this } };

		@@ -151,6 +152,7 @@ class Painter : private util::noncopyable {

		FrameHistory frameHistory;

[core] update shaders for line property functions #6658

[core] update shaders for line property functions #6658

Conversation

mollymerp commented Oct 11, 2016 • edited Loading

mention-bot commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollymerp commented Oct 11, 2016

mollymerp commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkaefer commented Nov 4, 2016

mollymerp commented Nov 4, 2016

mollymerp commented Nov 11, 2016

jfirebaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mollymerp commented Nov 12, 2016

mollymerp commented Nov 12, 2016

Choose a reason for hiding this comment

mollymerp commented Nov 15, 2016

jfirebaugh commented Nov 15, 2016

kkaefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkaefer commented Dec 5, 2016

mollymerp commented Oct 11, 2016 •

edited

Loading