-
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
Fix pattern attributes vertex layout #9482
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,12 @@ | ||
// @flow | ||
import {createLayout} from '../../util/struct_array'; | ||
|
||
export default createLayout([ | ||
const patternAttributes = createLayout([ | ||
// [tl.x, tl.y, br.x, br.y] | ||
{name: 'a_pattern_from', components: 4, type: 'Uint16'}, | ||
{name: 'a_pattern_to', components: 4, type: 'Uint16'}, | ||
{name: 'a_pixel_ratio_from', components: 1, type: 'Uint8'}, | ||
{name: 'a_pixel_ratio_to', components: 1, type: 'Uint8'}, | ||
]); | ||
|
||
export default patternAttributes; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ import {register} from '../util/web_worker_transfer'; | |
import {PossiblyEvaluatedPropertyValue} from '../style/properties'; | ||
import {StructArrayLayout1f4, StructArrayLayout2f8, StructArrayLayout4f16, PatternLayoutArray} from './array_types'; | ||
import {clamp} from '../util/util'; | ||
|
||
import patternAttributes from './bucket/pattern_attributes'; | ||
import EvaluationParameters from '../style/evaluation_parameters'; | ||
import FeaturePositionMap from './feature_position_map'; | ||
import { | ||
|
@@ -312,20 +312,13 @@ class CrossFadedCompositeBinder implements AttributeBinder { | |
zoomOutPaintVertexBuffer: ?VertexBuffer; | ||
paintVertexAttributes: Array<StructArrayMember>; | ||
|
||
constructor(expression: CompositeExpression, names: Array<string>, type: string, useIntegerZoom: boolean, zoom: number, PaintVertexArray: Class<StructArray>, layerId: string) { | ||
constructor(expression: CompositeExpression, type: string, useIntegerZoom: boolean, zoom: number, PaintVertexArray: Class<StructArray>, layerId: string) { | ||
this.expression = expression; | ||
this.type = type; | ||
this.useIntegerZoom = useIntegerZoom; | ||
this.zoom = zoom; | ||
this.layerId = layerId; | ||
|
||
this.paintVertexAttributes = names.map((name) => ({ | ||
name: `a_${name}`, | ||
type: 'Uint16', | ||
components: 4, | ||
offset: 0 | ||
})); | ||
|
||
this.zoomInPaintVertexArray = new PaintVertexArray(); | ||
this.zoomOutPaintVertexArray = new PaintVertexArray(); | ||
} | ||
|
@@ -371,8 +364,8 @@ class CrossFadedCompositeBinder implements AttributeBinder { | |
|
||
upload(context: Context) { | ||
if (this.zoomInPaintVertexArray && this.zoomInPaintVertexArray.arrayBuffer && this.zoomOutPaintVertexArray && this.zoomOutPaintVertexArray.arrayBuffer) { | ||
this.zoomInPaintVertexBuffer = context.createVertexBuffer(this.zoomInPaintVertexArray, this.paintVertexAttributes, this.expression.isStateDependent); | ||
this.zoomOutPaintVertexBuffer = context.createVertexBuffer(this.zoomOutPaintVertexArray, this.paintVertexAttributes, this.expression.isStateDependent); | ||
this.zoomInPaintVertexBuffer = context.createVertexBuffer(this.zoomInPaintVertexArray, patternAttributes.members, this.expression.isStateDependent); | ||
this.zoomOutPaintVertexBuffer = context.createVertexBuffer(this.zoomOutPaintVertexArray, patternAttributes.members, this.expression.isStateDependent); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this hard-coding though? are patterns the only kind of cross faded composite attributes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes they are the only type at this time, we don't emplace any other vertices in this layout so we can make it explicit https://github.com/mapbox/mapbox-gl-js/blob/b238b15f64e00540c29d0ec3d2065861aa90d1f9/src/data/program_configuration.js#L357_L368. We could leave a note to prevent programmer error if this gets extended. Were there any other case you were thinking of? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about cross-referencing from a lookup table? We could either update the lookup table by hand, or codegen it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logging offline discussion: we decided on going the simple way for the time being to cut out extra logic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
} | ||
} | ||
|
||
|
@@ -438,7 +431,7 @@ export default class ProgramConfiguration { | |
} else if (expression.kind === 'source' || isCrossFaded) { | ||
const StructArrayLayout = layoutType(property, type, 'source'); | ||
this.binders[property] = isCrossFaded ? | ||
new CrossFadedCompositeBinder(expression, names, type, useIntegerZoom, zoom, StructArrayLayout, layer.id) : | ||
new CrossFadedCompositeBinder(expression, type, useIntegerZoom, zoom, StructArrayLayout, layer.id) : | ||
new SourceExpressionBinder(expression, names, type, StructArrayLayout); | ||
keys.push(`/a_${property}`); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
{ | ||
"version": 8, | ||
"metadata": { | ||
"test": { | ||
"height": 256, | ||
"width": 256, | ||
"pixelRatio": 1, | ||
"operations": [ | ||
[ | ||
"wait" | ||
], | ||
[ | ||
"addImage", | ||
"pattern", | ||
"./image/marker.png", | ||
{ | ||
"pixelRatio": 2 | ||
} | ||
], | ||
[ | ||
"wait" | ||
], | ||
[ | ||
"addSource", | ||
"geojson", | ||
{ | ||
"type": "geojson", | ||
"data": { | ||
"type": "FeatureCollection", | ||
"features": [ | ||
{ | ||
"type": "Feature", | ||
"properties": { | ||
"height": 10 | ||
}, | ||
"geometry": { | ||
"type": "Polygon", | ||
"coordinates": [ | ||
[ | ||
[ | ||
-0.0001, | ||
-0.0001 | ||
], | ||
[ | ||
-0.0001, | ||
0.0001 | ||
], | ||
[ | ||
0.0001, | ||
0.0001 | ||
], | ||
[ | ||
0.0001, | ||
-0.0001 | ||
], | ||
[ | ||
-0.0001, | ||
-0.0001 | ||
] | ||
] | ||
] | ||
} | ||
} | ||
] | ||
} | ||
} | ||
], | ||
[ | ||
"wait" | ||
], | ||
[ | ||
"addLayer", | ||
{ | ||
"id": "extrusion", | ||
"type": "fill-extrusion", | ||
"source": "geojson", | ||
"paint": { | ||
"fill-extrusion-pattern": { | ||
"property": "height", | ||
"type": "interval", | ||
"stops": [[0, "pattern"]] | ||
}, | ||
"fill-extrusion-height": 10 | ||
} | ||
} | ||
], | ||
[ | ||
"wait" | ||
] | ||
] | ||
} | ||
}, | ||
"pitch": 60, | ||
"zoom": 19, | ||
"sources": {}, | ||
"layers": [] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue lies here, instead of using
0
, we need to usepatternAttributes.members
which has the correct offsets and sizes.