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.

Sign up for GitHub

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

Enable property functions for "fill-color" and "fill-outline-color" #2629

Merged
merged 7 commits into from
May 27, 2016
Merged

Enable property functions for "fill-color" and "fill-outline-color" #2629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1b1985f
Enable property functions for fill-color and fill-outline-color
May 26, 2016
90fb462
Respond to code review feedback
May 27, 2016
3a1751f
Bump test suite
May 27, 2016
6ad381a
Bump shaders version
May 27, 2016
7efe4fd
Respond to PR feedback
May 27, 2016
b82d89b
Fix perf regression in createProgram
May 27, 2016
fa0cb03
Optimize useProgram
May 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions js/data/bucket/fill_bucket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,41 @@ FillBucket.prototype.programInterfaces = {
name: 'a_pos',
components: 2,
type: 'Int16'
}],
paintAttributes: [{
name: 'a_color',
components: 4,
type: 'Uint8',
getValue: function(layer, globalProperties, featureProperties) {
return util.premultiply(layer.getPaintValue("fill-color", globalProperties, featureProperties));
},
multiplier: 255,
paintProperty: 'fill-color'
}, {
name: 'a_outline_color',
components: 4,
type: 'Uint8',
getValue: function(layer, globalProperties, featureProperties) {
return util.premultiply(layer.getPaintValue("fill-outline-color", globalProperties, featureProperties));
},
multiplier: 255,
paintProperty: 'fill-outline-color'
}]
}
};

FillBucket.prototype.addFeature = function(feature) {
var lines = loadGeometry(feature);
var polygons = classifyRings(lines, EARCUT_MAX_RINGS);

var startGroup = this.makeRoomFor('fill', 0);
var startIndex = startGroup.layout.vertex.length;

for (var i = 0; i < polygons.length; i++) {
this.addPolygon(polygons[i]);
}

this.populatePaintArrays('fill', {zoom: this.zoom}, feature.properties, startGroup, startIndex);
};

FillBucket.prototype.addPolygon = function(polygon) {
Expand Down
21 changes: 10 additions & 11 deletions js/data/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
'use strict';

var assert = require('assert');

module.exports = Buffer;

/**
Expand Down Expand Up @@ -65,16 +63,17 @@ Buffer.prototype.setVertexAttribPointers = function(gl, program) {
for (var j = 0; j < this.attributes.length; j++) {
var member = this.attributes[j];
var attribIndex = program[member.name];
assert(attribIndex !== undefined, 'array member "' + member.name + '" name does not match shader attribute name');

gl.vertexAttribPointer(
attribIndex,
member.components,
gl[AttributeType[member.type]],
false,
this.arrayType.bytesPerElement,
member.offset
);
if (attribIndex !== undefined) {
gl.vertexAttribPointer(
attribIndex,
member.components,
gl[AttributeType[member.type]],
false,
this.arrayType.bytesPerElement,
member.offset
);
}
}
};

Expand Down
18 changes: 18 additions & 0 deletions js/render/create_uniform_pragmas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';

var assert = require('assert');

module.exports = function(uniforms) {
var pragmas = {};

for (var i = 0; i < uniforms.length; i++) {
var uniform = uniforms[i];
assert(uniform.name.slice(0, 2) === 'u_');

var type = '$1 ' + (uniform.components === 1 ? 'float' : 'vec' + uniform.components);
pragmas['define ' + uniform.name.slice(2)] = 'uniform ' + type + ' ' + uniform.name + ';\n';
pragmas['initialize ' + uniform.name.slice(2)] = type + ' ' + uniform.name.slice(2) + ' = ' + uniform.name + ';\n';
}

return pragmas;
};
4 changes: 3 additions & 1 deletion js/render/draw_background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ var TilePyramid = require('../source/tile_pyramid');
var pyramid = new TilePyramid({ tileSize: 512 });
var util = require('../util/util');
var pixelsToTileUnits = require('../source/pixels_to_tile_units');
var createUniformPragmas = require('./create_uniform_pragmas');

module.exports = drawBackground;

Expand Down Expand Up @@ -47,7 +48,8 @@ function drawBackground(painter, source, layer) {
// Draw filling rectangle.
if (painter.isOpaquePass !== (color[3] === 1)) return;

program = painter.useProgram('fill');
var pragmas = createUniformPragmas([{name: 'u_color', components: 4}]);
program = painter.useProgram('fill', [], pragmas, pragmas);
gl.uniform4fv(program.u_color, color);
gl.uniform1f(program.u_opacity, opacity);
painter.tileExtentVAO.bind(gl, program, painter.tileExtentBuffer);
Expand Down
7 changes: 4 additions & 3 deletions js/render/draw_circle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@ function drawCircles(painter, source, layer, coords) {
var bufferGroups = bucket.bufferGroups.circle;
if (!bufferGroups) continue;

var programOptions = bucket.paintAttributes.circle[layer.id];
var program = painter.useProgram(
'circle',
bucket.paintAttributes.circle[layer.id].defines,
bucket.paintAttributes.circle[layer.id].vertexPragmas,
bucket.paintAttributes.circle[layer.id].fragmentPragmas
programOptions.defines,
programOptions.vertexPragmas,
programOptions.fragmentPragmas
);

gl.uniform2f(program.u_extrude_scale,
Expand Down
57 changes: 37 additions & 20 deletions js/render/draw_fill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ function draw(painter, source, layer, coords) {

var color = util.premultiply(layer.paint['fill-color']);
var image = layer.paint['fill-pattern'];
var strokeColor = util.premultiply(layer.paint['fill-outline-color']);
var opacity = layer.paint['fill-opacity'];
var isOutlineColorDefined = layer.getPaintProperty('fill-outline-color');

// Draw fill
if (image ? !painter.isOpaquePass : painter.isOpaquePass === (color[3] === 1 && opacity === 1)) {
Expand All @@ -24,14 +24,12 @@ function draw(painter, source, layer, coords) {
}
}

// Draw stroke
if (!painter.isOpaquePass && layer.paint['fill-antialias']) {
if (strokeColor || !layer.paint['fill-pattern']) {
var outlineProgram = painter.useProgram('outline');
if (isOutlineColorDefined || !layer.paint['fill-pattern']) {
painter.lineWidth(2);
painter.depthMask(false);

if (strokeColor) {
if (isOutlineColorDefined) {
// If we defined a different color for the fill outline, we are
// going to ignore the bits in 0x07 and just care about the global
// clipping mask.
Expand All @@ -44,15 +42,11 @@ function draw(painter, source, layer, coords) {
// the (non-antialiased) fill.
painter.setDepthSublayer(0);
}
gl.uniform2f(outlineProgram.u_world, gl.drawingBufferWidth, gl.drawingBufferHeight);
gl.uniform4fv(outlineProgram.u_color, strokeColor ? strokeColor : color);
gl.uniform1f(outlineProgram.u_opacity, opacity);

for (var j = 0; j < coords.length; j++) {
drawStroke(painter, source, layer, coords[j]);
}
} else {
var outlinePatternProgram = painter.useProgram('outlinepattern');
painter.lineWidth(2);
painter.depthMask(false);
// Otherwise, we only want to drawFill the antialiased parts that are
Expand All @@ -61,7 +55,6 @@ function draw(painter, source, layer, coords) {
// the current shape, some pixels from the outline stroke overlapped
// the (non-antialiased) fill.
painter.setDepthSublayer(0);
gl.uniform2f(outlinePatternProgram.u_world, gl.drawingBufferWidth, gl.drawingBufferHeight);

for (var k = 0; k < coords.length; k++) {
drawStroke(painter, source, layer, coords[k]);
Expand All @@ -80,23 +73,29 @@ function drawFill(painter, source, layer, coord) {

var gl = painter.gl;

var color = util.premultiply(layer.paint['fill-color']);
var image = layer.paint['fill-pattern'];
var opacity = layer.paint['fill-opacity'];
var program;

if (image) {
if (!image) {

var programOptions = bucket.paintAttributes.fill[layer.id];
program = painter.useProgram(
'fill',
programOptions.defines,
programOptions.vertexPragmas,
programOptions.fragmentPragmas
);
bucket.setUniforms(gl, 'fill', program, layer, {zoom: painter.transform.zoom});
gl.uniform1f(program.u_opacity, opacity);

} else {
// Draw texture fill
program = painter.useProgram('pattern');
setPattern(image, opacity, tile, coord, painter, program);

gl.activeTexture(gl.TEXTURE0);
painter.spriteAtlas.bind(gl, true);

} else {
program = painter.useProgram('fill');
gl.uniform4fv(program.u_color, color);
gl.uniform1f(program.u_opacity, opacity);
}

gl.uniformMatrix4fv(program.u_matrix, false, painter.translatePosMatrix(
Expand All @@ -110,7 +109,7 @@ function drawFill(painter, source, layer, coord) {

for (var i = 0; i < bufferGroups.length; i++) {
var group = bufferGroups[i];
group.vaos[layer.id].bind(gl, program, group.layout.vertex, group.layout.element);
group.vaos[layer.id].bind(gl, program, group.layout.vertex, group.layout.element, group.paint[layer.id]);
gl.drawElements(gl.TRIANGLES, group.layout.element.length, gl.UNSIGNED_SHORT, 0);
}
}
Expand All @@ -125,7 +124,25 @@ function drawStroke(painter, source, layer, coord) {

var image = layer.paint['fill-pattern'];
var opacity = layer.paint['fill-opacity'];
var program = image ? painter.useProgram('outlinepattern') : painter.useProgram('outline');
var isOutlineColorDefined = layer.getPaintProperty('fill-outline-color');

var program;
if (image && !isOutlineColorDefined) {
program = painter.useProgram('outlinepattern');
gl.uniform2f(program.u_world, gl.drawingBufferWidth, gl.drawingBufferHeight);

} else {
var programOptions = bucket.paintAttributes.fill[layer.id];
program = painter.useProgram(
'outline',
programOptions.defines,
programOptions.vertexPragmas,
programOptions.fragmentPragmas
);
gl.uniform2f(program.u_world, gl.drawingBufferWidth, gl.drawingBufferHeight);
gl.uniform1f(program.u_opacity, opacity);
bucket.setUniforms(gl, 'fill', program, layer, {zoom: painter.transform.zoom});
}

gl.uniformMatrix4fv(program.u_matrix, false, painter.translatePosMatrix(
coord.posMatrix,
Expand All @@ -140,7 +157,7 @@ function drawStroke(painter, source, layer, coord) {

for (var k = 0; k < bufferGroups.length; k++) {
var group = bufferGroups[k];
group.secondVaos[layer.id].bind(gl, program, group.layout.vertex, group.layout.element2);
group.secondVaos[layer.id].bind(gl, program, group.layout.vertex, group.layout.element2, group.paint[layer.id]);
gl.drawElements(gl.LINES, group.layout.element2.length * 2, gl.UNSIGNED_SHORT, 0);
}
}
Expand Down
4 changes: 3 additions & 1 deletion js/render/painter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ var StructArrayType = require('../util/struct_array');
var Buffer = require('../data/buffer');
var VertexArrayObject = require('./vertex_array_object');
var RasterBoundsArray = require('./draw_raster').RasterBoundsArray;
var createUniformPragmas = require('./create_uniform_pragmas');

module.exports = Painter;

Expand Down Expand Up @@ -150,7 +151,8 @@ Painter.prototype._renderTileClippingMasks = function(coords) {

gl.stencilFunc(gl.ALWAYS, id, 0xF8);

var program = this.useProgram('fill');
var pragmas = createUniformPragmas([{name: 'u_color', components: 4}]);
var program = this.useProgram('fill', [], pragmas, pragmas);
gl.uniformMatrix4fv(program.u_matrix, false, coord.posMatrix);

// Draw the clipping mask
Expand Down
5 changes: 2 additions & 3 deletions js/render/painter/use_program.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module.exports._createProgram = function(name, defines, vertexPragmas, fragmentP
var program = gl.createProgram();
var definition = shaders[name];

var definesSource = '';
var definesSource = '#define MAPBOX_GL_JS;\n';
for (var j = 0; j < defines.length; j++) {
definesSource += '#define ' + defines[j] + ';\n';
}
Expand Down Expand Up @@ -73,9 +73,8 @@ module.exports.useProgram = function (nextProgramName, defines, vertexPragmas, f
var gl = this.gl;

defines = defines || [];
defines.push('MAPBOX_GL_JS');
if (this._showOverdrawInspector) {
defines.push('OVERDRAW_INSPECTOR');
defines = defines.concat('OVERDRAW_INSPECTOR');
}

var nextProgram = this._createProgramCached(nextProgramName, defines, vertexPragmas, fragmentPragmas);
Expand Down
46 changes: 44 additions & 2 deletions js/style/style_layer/fill_style_layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,48 @@ function FillStyleLayer() {
StyleLayer.apply(this, arguments);
}

module.exports = FillStyleLayer;
FillStyleLayer.prototype = util.inherit(StyleLayer, {

getPaintValue: function(name, globalProperties, featureProperties) {
if (name === 'fill-outline-color' && this.getPaintProperty('fill-outline-color') === undefined) {
return StyleLayer.prototype.getPaintValue.call(this, 'fill-color', globalProperties, featureProperties);
} else {
return StyleLayer.prototype.getPaintValue.call(this, name, globalProperties, featureProperties);
}
},

getPaintValueStopZoomLevels: function(name) {
if (name === 'fill-outline-color' && this.getPaintProperty('fill-outline-color') === undefined) {
return StyleLayer.prototype.getPaintValueStopZoomLevels.call(this, 'fill-color');
} else {
return StyleLayer.prototype.getPaintValueStopZoomLevels.call(this, arguments);
}
},

getPaintInterpolationT: function(name, zoom) {
if (name === 'fill-outline-color' && this.getPaintProperty('fill-outline-color') === undefined) {
return StyleLayer.prototype.getPaintInterpolationT.call(this, 'fill-color', zoom);
} else {
return StyleLayer.prototype.getPaintInterpolationT.call(this, name, zoom);
}
},

FillStyleLayer.prototype = util.inherit(StyleLayer, {});
isPaintValueFeatureConstant: function(name) {
if (name === 'fill-outline-color' && this.getPaintProperty('fill-outline-color') === undefined) {
return StyleLayer.prototype.isPaintValueFeatureConstant.call(this, 'fill-color');
} else {
return StyleLayer.prototype.isPaintValueFeatureConstant.call(this, name);
}
},

isPaintValueZoomConstant: function(name) {
if (name === 'fill-outline-color' && this.getPaintProperty('fill-outline-color') === undefined) {
return StyleLayer.prototype.isPaintValueZoomConstant.call(this, 'fill-color');
} else {
return StyleLayer.prototype.isPaintValueZoomConstant.call(this, name);
}
}

});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not an ideal long term architecture. I'm thinking on how to improve this...

Copy link
Member

Choose a reason for hiding this comment

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

You could DRY up the code by something like this:

FillStyleLayer.prototype.fallbackMapping = {'fill-outline-color': 'fill-color'};

And then using this object to map properties in StyleLayer, avoiding code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sold on that design

  • Similar branching logic would still need to be written, but it'd be in StyleLayer rather than FillLayer
  • I can't think of many (any?) other style properties that have fallback behaviour like this

My preferred solution would be to factor out all the per-paint-property methods in StyleLayer into a StyleValue class (that might replace StyleDeclaration and/or StyleTransition)

type StyleValue<T> {
  isZoomConstant: boolean;
  isFeatureConstant: boolean;
  calculate(zoom:number, featureProperties: {}) => T;
  getInterpolationT(zoom: number) =>;
  ...
}

Then, FillStyleLayer's implementation would only have one overriden method:

getStyleValue: function(name) {
  if (name === 'fill-outline-color' && this.getPaintProperty('fill-outline-color') === undefined) {
    return StyleLayer.prototype.getStyleValue.call(this, 'fill-color');
  } else {
    return StyleLayer.prototype.getStyleValue.call(this, name);
  }
}

All that said, I'd be happy to 🚢 with the code in this PR for now. It may be verbose and non-DRY but it is clear, robust, and well-contained.

Copy link
Member

Choose a reason for hiding this comment

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

The proposed refactor sounds great! Yeah, we can follow-up on that later. I'd make sure to run benchmarks before merging though because things like this make things slower if they're in hot path.


module.exports = FillStyleLayer;
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"grid-index": "^0.1.0",
"mapbox-gl-function": "^1.2.1",
"mapbox-gl-js-supported": "^1.1.0",
"mapbox-gl-shaders": "mapbox/mapbox-gl-shaders#0eda4509d2893adb3aaf92ac68301cdc7722e7b1",
"mapbox-gl-shaders": "mapbox/mapbox-gl-shaders#e4737bb136d718f9c5fe8d943380f05db6249b57",
"mapbox-gl-style-spec": "^8.7.0",
"minifyify": "^7.0.1",
"pbf": "^1.3.2",
Expand Down Expand Up @@ -47,7 +47,7 @@
"express": "^4.13.4",
"gl": "^2.1.5",
"istanbul": "^0.4.2",
"mapbox-gl-test-suite": "mapbox/mapbox-gl-test-suite#5d51f251a04ef919a0e37ccc5bb60a270e8b41fa",
"mapbox-gl-test-suite": "mapbox/mapbox-gl-test-suite#5c38f278f92688c68db115601663d5ac125a1961",
"nyc": "^6.1.1",
"sinon": "^1.15.4",
"st": "^1.0.0",
Expand Down