Skip to content

Protobuf changes #472

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

Draft
bukue wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Protobuf changes #472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
22a2ef0
first pass to get protobuf working on streetscape
bukue Oct 20, 2020
6053499
colors done
bukue Oct 21, 2020
285bd33
simplifying all package.json files
bukue Oct 21, 2020
3bf0b70
addressing PR comments
bukue Nov 4, 2020
7dad4d6
luma.gl/webgl to 8.3.1
bukue Nov 4, 2020
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
11 changes: 7 additions & 4 deletions modules/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
],
"scripts": {},
"dependencies": {
"@deck.gl/core": "^8.1.5",
"@deck.gl/layers": "^8.1.5",
"@deck.gl/mesh-layers": "^8.1.5",
"@deck.gl/react": "^8.1.5",
"@deck.gl/core": "8.1.8",
Copy link
Contributor

@twojtasz twojtasz Oct 30, 2020

Choose a reason for hiding this comment

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

We should not be using fixed versions. Why is this necessary. Is there a patch version that is required? Otherwise ^ should cover this already.

Copy link
Author

@bukue bukue Nov 4, 2020

Choose a reason for hiding this comment

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

I partially agree with you, unfortunately dependencies among the whole viz.gl packages are complex and having fuzzy versions leads to tons of conflicts. Some of them can be solved with resolutions, but a lot of them cannot.

Copy link
Contributor

@twojtasz twojtasz Nov 4, 2020

Choose a reason for hiding this comment

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

we've made it this far? Do we have a concise statement of the problem? This does not seem to be a streetscape.gl issue, but an application issue. So the solution should be placed there, not in streetscape.gl

"@deck.gl/layers": "8.1.8",
"@deck.gl/mesh-layers": "8.1.8",
"@deck.gl/react": "8.1.8",
"@emotion/core": "^10.0.22",
"@emotion/styled": "^10.0.23",
"@streetscape.gl/monochrome": "1.0.6",
Expand All @@ -34,5 +34,8 @@
"peerDependencies": {
"react": ">=16.3.0",
"react-dom": ">=16.3.0"
},
"resolutions": {
"@luma.gl/webgl": "8.3.1"
}
}
101 changes: 82 additions & 19 deletions modules/core/src/layers/xviz-layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

/* eslint-disable camelcase */
import {CompositeLayer} from '@deck.gl/core';
import {ScatterplotLayer, PathLayer, PolygonLayer, TextLayer} from '@deck.gl/layers';
import {ScatterplotLayer, PathLayer, PolygonLayer, TextLayer, BitmapLayer} from '@deck.gl/layers';
import PointCloudLayer from './point-cloud-layer/point-cloud-layer';
// TODO/ib - Uncomment to enable binary/flat polygon arrays
// import PathLayer from './binary-path-layer/binary-path-layer';
Expand All @@ -30,6 +30,16 @@ import {XVIZObject} from '@xviz/parser';

import deepExtend from 'lodash.merge';

const XVIZ_PRIMITIVE_TYPES = {
circle: 'circle',
image: 'image',
point: 'point',
polygon: 'polygon',
polyline: 'polyline',
stadium: 'stadium',
text: 'text'
};

const XVIZ_TO_LAYER_TYPE = {
// V1
points2d: 'scatterplot',
Copy link
Contributor

@twojtasz twojtasz Oct 30, 2020

Choose a reason for hiding this comment

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

Lets not change the existing style unless there is some reason. When in rome ...

Expand All @@ -46,7 +56,8 @@ const XVIZ_TO_LAYER_TYPE = {
polyline: 'path',
polygon: 'polygon',
text: 'text',
stadium: 'stadium'
stadium: 'stadium',
image: 'image'
};

const STYLE_TO_LAYER_PROP = {
Expand All @@ -59,9 +70,9 @@ const STYLE_TO_LAYER_PROP = {
filled: 'filled',
stroke_width_min_pixels: 'lineWidthMinPixels',
stroke_width_max_pixels: 'lineWidthMaxPixels',
stroke_width: 'getLineWidth',
stroke_color: 'getLineColor',
fill_color: 'getFillColor'
strokeWidth: 'getLineWidth',
strokeColor: 'getLineColor',
fillColor: 'getFillColor'
},
pointcloud: {
opacity: 'opacity',
Expand All @@ -74,34 +85,34 @@ const STYLE_TO_LAYER_PROP = {
opacity: 'opacity',
stroke_width_min_pixels: 'widthMinPixels',
stroke_width_max_pixels: 'widthMaxPixels',
stroke_color: 'getColor',
stroke_width: 'getWidth'
strokeColor: 'getColor',
strokeWidth: 'getWidth'
},
stadium: {
opacity: 'opacity',
radius_min_pixels: 'widthMinPixels',
radius_max_pixels: 'widthMaxPixels',
fill_color: 'getColor',
fillColor: 'getColor',
radius: 'getWidth'
},
polygon: {
opacity: 'opacity',
stroked: 'stroked',
filled: 'filled',
extruded: 'extruded',
stroke_color: 'getLineColor',
stroke_width: 'getLineWidth',
strokeColor: 'getLineColor',
strokeWidth: 'getLineWidth',
stroke_width_min_pixels: 'lineWidthMinPixels',
stroke_width_max_pixels: 'lineWidthMaxPixels',
fill_color: 'getFillColor',
fillColor: 'getFillColor',
height: 'getElevation'
},
text: {
opacity: 'opacity',
fill_color: 'getColor',
fillColor: 'getColor',
font_family: 'fontFamily',
font_weight: 'fontWeight',
text_size: 'getSize',
textSize: 'getSize',
text_rotation: 'getAngle',
text_anchor: 'getTextAnchor',
text_baseline: 'getAlignmentBaseline'
Expand Down Expand Up @@ -130,15 +141,22 @@ const getStylesheetProperty = (context, propertyName, objectState) =>
// to be inline, stylesheet, then default.
//
/* eslint-disable complexity */
function getProperty(context, propertyName, f = EMPTY_OBJECT) {
function getProperty(
context,
propertyName,
f = EMPTY_OBJECT,
primitiveType = null
) {
let objectState = f;

// Handle XVIZ v1 color override where our semantic color mapping
// differs from current OCS colors. In XVIZ v2 we should be aligned.
if (context.useSemanticColor) {
switch (propertyName) {
case 'stroke_color':
case 'strokeColor':
case 'fill_color':
case 'fillColor':
objectState = XVIZObject.get(f.id) || f;
break;

Expand All @@ -154,9 +172,12 @@ function getProperty(context, propertyName, f = EMPTY_OBJECT) {
switch (propertyName) {
case 'stroke_color':
case 'fill_color':
case 'strokeColor':
case 'fillColor':
altPropertyName = 'color';
break;
case 'stroke_width':
case 'strokeWidth':
altPropertyName = 'thickness';
break;
case 'radius':
Expand Down Expand Up @@ -190,25 +211,47 @@ function getProperty(context, propertyName, f = EMPTY_OBJECT) {

// 3. Property from default style
if (property === null) {
property = context.style.getPropertyDefault(propertyName);
property = context.style.getPropertyDefault(propertyName, primitiveType);
}

if (propertyName === 'text_anchor' || propertyName === 'text_baseline') {
if (
property &&
(propertyName === 'text_anchor' || propertyName === 'text_baseline')
) {
// These XVIZ enumerations map to Deck.gl as lowercase strings
property = property.toLowerCase();
}
if (
property &&
(propertyName === 'textAnchor' || propertyName === 'textBaseline')
) {
// These XVIZ enumerations map to Deck.gl as lowercase strings
const lcProp = property.toLowerCase();
property = `${lcProp.substring(0, 4)}_${lcProp.substring(4)}`;
}

return property;
}
/* eslint-enable complexity */

export default class XVIZLayer extends CompositeLayer {
_getProperty(propertyName) {
return getProperty(this.props, propertyName);
return getProperty(
this.props,
propertyName,
{},
this._getLayerType(this.props.data)
);
}

_getPropertyAccessor(propertyName) {
return f => getProperty(this.props, propertyName, f);
return f =>
getProperty(
this.props,
propertyName,
f,
this._getLayerType(this.props.data)
);
}

// These props are persistent unless data type and stylesheet change
Expand Down Expand Up @@ -331,7 +374,10 @@ export default class XVIZLayer extends CompositeLayer {
length: data[0].points.length / 3,
attributes: {
getPosition: data[0].points,
getColor: data[0].colors
getColor: {
Copy link
Contributor

@twojtasz twojtasz Oct 30, 2020

Choose a reason for hiding this comment

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

This is fixing the difference between rgb and rgba? We should make sure we have a test for this

Copy link
Author

@bukue bukue Oct 31, 2020

Choose a reason for hiding this comment

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

yep, I will add some tests

value: data[0].colors,
size: data[0].points.length === data[0].colors.length ? 3 : 4
}
}
},
vehicleRelativeTransform: this.props.vehicleRelativeTransform,
Expand All @@ -353,6 +399,7 @@ export default class XVIZLayer extends CompositeLayer {
})
);

// TODO (mauricio): also figure out wht a box appears on top of the stadium
case 'stadium':
return new PathLayer(
forwardProps,
Expand All @@ -378,6 +425,7 @@ export default class XVIZLayer extends CompositeLayer {
data,
lightSettings,
wireframe: layerProps.stroked,
extruded: layerProps.stroked, //TODO: check for height instead
getPolygon: f => f.vertices,
updateTriggers: deepExtend(updateTriggers, {
getLineColor: {useSemanticColor: this.props.useSemanticColor},
Expand All @@ -400,6 +448,21 @@ export default class XVIZLayer extends CompositeLayer {
})
);

case XVIZ_PRIMITIVE_TYPES.image:
Copy link
Contributor

@twojtasz twojtasz Nov 4, 2020

Choose a reason for hiding this comment

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

What does this constant buy us? The rest of the code is just using a string. Just curious if i'm missing something.
Mainly this creates a dissonance in the code and prompts the question "why"

// TODO (mauricio): images stream is being filtered out, figure out why
return new BitmapLayer(
forwardProps,
layerProps,
this.getSubLayerProps({
id: XVIZ_PRIMITIVE_TYPES.image,
image: URL.createObjectURL(
Copy link
Contributor

@marionleborgne marionleborgne Nov 6, 2020

Choose a reason for hiding this comment

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

URL is not defined.

// TODO (mauricio): adjust this once we can send mime type from the backend
new Blob([data[0].data], { type: 'image/png' })
),
bounds: [-1, -1, 1, 1]
Copy link
Contributor

@twojtasz twojtasz Oct 30, 2020

Choose a reason for hiding this comment

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

add comment on this. We need to get the actual width changes added to xviz.

})
);

default:
return null;
}
Expand Down
4 changes: 2 additions & 2 deletions modules/layers/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
],
"scripts": {},
"peerDependencies": {
"@deck.gl/core": "^8.1.5",
"@deck.gl/layers": "^8.l.5"
"@deck.gl/core": "8.1.8",
"@deck.gl/layers": "8.1.8"
}
}
9 changes: 6 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@
"@storybook/addon-links": "^4.0.7",
"@storybook/addon-options": "^4.0.7",
"@storybook/react": "^4.0.7",
"@deck.gl/test-utils": "^8.1.5",
"@luma.gl/test-utils": "^8.1.2",
"@deck.gl/test-utils": "8.1.8",
"@luma.gl/test-utils": "8.1.3",
"@probe.gl/test-utils": "^3.2.1",
"babel-loader": "^8.0.0",
"coveralls": "^3.0.0",
Expand All @@ -58,5 +58,8 @@
"copyright-check",
"test-fast"
],
"private": true
"private": true,
"resolutions": {
"@luma.gl/webgl": "8.3.1"
}
}
Loading