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

Jump to bottom

Publish work-in-progress layers in new deck.gl-layers module #1003

Merged
merged 2 commits into from
Oct 13, 2017
Merged

Publish work-in-progress layers in new deck.gl-layers module #1003

merged 2 commits into from
Oct 13, 2017

Conversation

ibgreen
Copy link
Collaborator

@ibgreen ibgreen commented Oct 10, 2017

…nd MeshLayer

@ibgreen
Copy link
Collaborator Author

ibgreen commented Oct 10, 2017

Tested with layer-browser. All new layers, should not break anything.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Oct 10, 2017

screen shot 2017-10-09 at 7 44 31 pm

@georgios-uber Initial support for direction arrow rendering in PathLayer (not yet perfect but a start).

@Pessimistress
Copy link
Collaborator

Shall we define a process how a layer will make it into the library?
The TextLayer model (start from example then graduate to core layers) worked pretty well IMO.
I do not have a problem with the MeshLayer, but the value of the other two layers is not very clear.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Oct 10, 2017

Shall we define a process how a layer will make it into the library?

Yes. My suggestion is that we start by moving such layers from sample-layers to experimental layers and start iterative improvements. At the end we either move a much improved layer back to sample layers or we decide on a good layer catalog to publish it in.

The TextLayer model (start from example then graduate to core layers) worked pretty well IMO.
I do not have a problem with the MeshLayer, but the value of the other two layers is not very clear.

  • The PathOutlineLayer in my opinion should be a default part of the PathLayer. Since that is a planar layer it seems natural to want to be able to handle relative relations and overlap. I just didn't want to propose merging it into the PathLayer at the moment due to the status of the PathLayer itself.

  • The PathMarkerLayer is something I see applications implement on their own again and again. The interesting question here is if the layer can become sufficiently generic to be worth publishing.

@ibgreen ibgreen force-pushed the experimental-layers branch 2 times, most recently from 6ac2dc4 to a38fef4 Compare October 12, 2017 03:08
Publish experimental-layers as a separate node module, including Path…
8964d36 
…MarkerLayer, PathOutlineLayer and MeshLayer
@ibgreen
Copy link
Collaborator Author

ibgreen commented Oct 12, 2017

In response to concerns about bundling work-in-progress layers into alpha releases:

  • Now publishes experimental-layers into a new deck.gl-layers npm module, in anticipation of longer-term consensus "monorepo" setup.
  • layer-browser has been updated to install new deck.gl-layers module and import experimental layers from it, and npm run start-local updated to consume from source.

@ibgreen ibgreen changed the title Add experimental-layers including PathMarkerLayer, PathOutlineLayer a… Publish work-in-progress layers in new deck.gl-layers module Oct 12, 2017
Pessimistress
Pessimistress approved these changes Oct 13, 2017
View reviewed changes
src/experimental-layers/package.json Outdated
"src"
],
"scripts": {
"start": "(cd examples/layer-browser && (path-exists node_modules || npm i) && npm run start-local)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was for the root. Just remove?

src/experimental-layers/package.json
"lint-yarn": "!(grep -q unpm.u yarn.lock) || (echo 'Please rebuild yarn file using public npmrc' && false)",
"publish-prod": "npm run build && npm run test && npm run test-dist && npm publish",
"publish-beta": "npm run build && npm run test && npm run test-dist && npm publish --tag beta",
"test": "npm run lint && npm run build && npm run test-node",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we intend to have separate test set up for each module?

src/experimental-layers/src/mesh-layer/mesh-layer-fragment.glsl.js Outdated
varying float vLightWeight;

void main(void) {
gl_FragColor = vec4(0., 0., 0., 1.);
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

src/experimental-layers/src/mesh-layer/mesh-layer-vertex.glsl.js

vTexCoord = texCoords;
vColor = instanceColors;
vLightWeight = getLightWeight(pos, normals);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should apply rotationMatrix to normals too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should apply rotationMatrix to normals too.

Good point left a TODO for now

src/experimental-layers/src/mesh-layer/mesh-layer.js Outdated
loadTexture(src) {
const {gl} = this.context;
const {model} = this.state;
getTexture(gl, src).then(texture => model.setUniforms({sampler1: texture}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The layer will not rerender when the texture loads because needsRedraw is not marked.

src/experimental-layers/src/mesh-layer/mesh-layer.js Outdated
for (const point of data) {
const position = getPosition(point);
value[i + 0] = position[0] || 0;
value[i + 1] = position[1] || 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

lng and lat should not need fallbacks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Use get for consistency?

src/experimental-layers/src/path-marker-layer/path-marker-layer.md Outdated
Create small markers along a path (defaults to arrows showing "direction").


// TODO - Move to Nebula
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove

@ibgreen ibgreen merged commit d46ac15 into master Oct 13, 2017
@ibgreen ibgreen deleted the experimental-layers branch October 13, 2017 20:38
@kylebarron kylebarron mentioned this pull request Mar 21, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pessimistress Pessimistress Pessimistress approved these changes

@howtimeflies0 howtimeflies0 Awaiting requested review from howtimeflies0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ibgreen @Pessimistress