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

Add additional sample layers #385

Merged
merged 6 commits into from
Mar 2, 2017
Merged

Add additional sample layers #385

merged 6 commits into from
Mar 2, 2017

Conversation

ibgreen
Copy link
Collaborator

@ibgreen ibgreen commented Mar 1, 2017

@Pessimistress This is the label layer we talked about, plus a few others.

Note: this PR is for discussion only, not ready for final review. Also EnhancedChoroplethLayer commented out due to build issues.

@ibgreen ibgreen force-pushed the more-sample-layers branch from 7fc81bb to 50feacd Compare March 1, 2017 16:19
howtimeflies0
howtimeflies0 reviewed Mar 1, 2017
View reviewed changes
Copy link

@howtimeflies0 howtimeflies0 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Old comments removed...

howtimeflies0
howtimeflies0 suggested changes Mar 1, 2017
View reviewed changes
Copy link

@howtimeflies0 howtimeflies0 left a comment

Choose a reason for hiding this comment

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

There is a bug on commit 321f3b0, which is where this PR is based on. Please rebase the PR to latest dev.

@ibgreen ibgreen force-pushed the more-sample-layers branch from 50feacd to 0efbdbd Compare March 1, 2017 19:14
@ibgreen
Copy link
Collaborator Author

ibgreen commented Mar 1, 2017

There is a bug on commit 321f3b0, which is where this PR is based on. Please rebase the PR to latest dev.

@shaojingli Rebased. The label layer works now (but updates too frequently).

@ibgreen
Copy link
Collaborator Author

ibgreen commented Mar 1, 2017

@shaojingli Fixed the excessive update issue (was related to the sublayer not having a proper id).

@Pessimistress This PR also contains bitmap and mesh layers, we'll need to pick some proper bitmaps and meshes to use in the example layers.

The labels don't look that great for some reason, but at least this is a start. I suspect selecting a heavier font-weight will help. Should investigate if we can control anti-aliasing using this technique.

screen shot 2017-03-01 at 2 15 27 pm

Pessimistress
Pessimistress reviewed Mar 1, 2017
View reviewed changes
src/layers/deprecated/choropleth-layer/choropleth-layer.js Outdated
@@ -77,9 +77,15 @@ export default class ChoroplethLayer extends Layer {
attributeManager.invalidateAll();
}

if (oldProps.opacity !== props.opacity) {
if (props.opacity !== oldProps.opacity) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need this any more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't need this any more?

Good catch. Removed.

Pessimistress
Pessimistress reviewed Mar 1, 2017
View reviewed changes
examples/sample-layers/label-layer/label-layer.js
}

renderLayers() {
return new IconLayer(Object.assign({}, this.props, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IconLayer doesn't currently preserve aspect ratio. We should fix it in a separate PR.

howtimeflies0
howtimeflies0 approved these changes Mar 2, 2017
View reviewed changes
Copy link

@howtimeflies0 howtimeflies0 left a comment

Choose a reason for hiding this comment

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

Assuming that we are going to provide a proper mesh / bitmap to mesh / bitmap layers, we can land this change.

@howtimeflies0
Copy link

@ibgreen You might want to add a fp64 prop to the label layer later (now we have 64-bit icon layer after #387) as the wobbling effect of texts rendered by 32-bit shaders is really prominent.

@ibgreen ibgreen force-pushed the more-sample-layers branch from 5dfab6a to 4fa9273 Compare March 2, 2017 00:41
@ibgreen
Copy link
Collaborator Author

ibgreen commented Mar 2, 2017

Assuming that we are going to provide a proper mesh / bitmap to mesh / bitmap layers, we can land this change.

Agreed. I could try to scrounge something up but I have a feeling that it will look much nicer if @Pessimistress does it. If we don't get examples in place we can always remove these sample layers before we ship.

@ibgreen You might want to add a fp64 prop to the label layer later (now we have 64-bit icon layer after #387) as the wobbling effect of texts rendered by 32-bit shaders is really prominent.

Good idea. I believe that I am passing through all props to the icon layer, so a 64bit prop should work if the IconLayer supports it. I added fp64 to defaultProps to make sure that the layer-browser example can discover the prop. Unfortunately I still see a wobble so something is missing. Maybe you can take a look after I land?

@ibgreen ibgreen merged commit 7ca17e4 into dev Mar 2, 2017
@ibgreen ibgreen deleted the more-sample-layers branch March 2, 2017 00:44
howtimeflies0 pushed a commit that referenced this pull request Mar 8, 2017
@ibgreen
Add additional sample layers (#385)
194bd0e 
* Add additional sample layers, LabelLayer, MeshLayer, BitmapLayer, OutlinePolygonLayer

* Working LabelLayer Example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howtimeflies0 howtimeflies0 howtimeflies0 approved these changes

@Pessimistress Pessimistress Pessimistress approved these changes

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

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