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.

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

Vector tiles polylines #4208

Merged
merged 17 commits into from
Oct 11, 2016
Merged

Vector tiles polylines #4208

merged 17 commits into from
Oct 11, 2016

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Aug 16, 2016

For CesiumGS/3d-tiles#25.

This is a branch off vector-tiles-polygons so I opened a PR into that to minimized the code diff. Either wait until both this and #4186 are ready or I can open a new PR into vector-tiles.

  • Add unit tests
  • Add doc
  • Investigate polygon offset values
  • Investigate incomplete polyline rendering issues (more likely an issue with the tileset)
  • Add per feature colors
  • Render during GROUND pass?
  • Update 3D Tiles Sandcastle example (could be just one vector example)

PolylineColorAppearanceVS,
BlendingState,
Pass
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting

        Pass) {
    'use strict';

@lilleyse
Copy link
Contributor

Looks good so far.

How easy would it be to support dynamic width?

@lilleyse
Copy link
Contributor

I set the width to 10 and am seeing some artifacts. Is this already considered as part of your to-do list?
capture

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

Added to tasklist:

  • Update 3D Tiles Sandcastle example (could be just one vector example)

@@ -0,0 +1,418 @@
/*global define*/
define([
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename this class to something more generic in case it is used for other batched renderers?

Copy link
Contributor

Choose a reason for hiding this comment

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

This organization would be similar to instanced models.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

Render during GROUND pass?

As discussed offline at one point, I think it is fine to have GROUND_OPAQUE and GROUND_TRANSLUCENT passes.

shaderProgram : primitive._sp,
uniformMap : uniformMap,
boundingVolume : primitive._boundingVolume,
modelMatrix : Matrix4.IDENTITY,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the default?

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

Just those comments.

@bagnell
Copy link
Contributor Author

bagnell commented Oct 11, 2016

Render during GROUND pass?

As discussed offline at one point, I think it is fine to have GROUND_OPAQUE and GROUND_TRANSLUCENT passes.

As part of this PR? Or in a separate PR to master?

@bagnell
Copy link
Contributor Author

bagnell commented Oct 11, 2016

How easy would it be to support dynamic width?

It should be pretty easy. I want to add it to the batch table instead of having it as a vertex attribute.

@bagnell
Copy link
Contributor Author

bagnell commented Oct 11, 2016

I'm going to merge this into the vector-tiles-polygons branch because its easier to work on them both in one branch. I've addressed most of the comments here. I'll move any that are still open and the checklist to the other PR.

@bagnell bagnell merged commit 1326f2e into vector-tiles-polygons Oct 11, 2016
@bagnell bagnell deleted the vector-tiles-polylines branch October 11, 2016 21:31
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 19, 2016

Render during GROUND pass?

As discussed offline at one point, I think it is fine to have GROUND_OPAQUE and GROUND_TRANSLUCENT passes.

As part of this PR? Or in a separate PR to master?

If this needs to be done for vector tiles, I would like to have this PR go into master so it doesn't still in 3d-tiles waiting for the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants