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 #124

Closed
wants to merge 9 commits into from
Closed

Vector tiles #124

wants to merge 9 commits into from

Conversation

lasalvavida
Copy link
Contributor

@lasalvavida lasalvavida commented Aug 26, 2016

@bagnell @pjcozzi, this should be ready for a look.

This is largely based on the existing vector tiles implementation, but I have made some tweaks to make it more consistent with the way that the i3dm and pnts specifications work.

TODO

  • Write schema after this gets looked over a bit.

@lasalvavida
Copy link
Contributor Author

link

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 26, 2016

I am part way through this. Made some edits in 47d6089.

Please update the status section of the main README.md.

POLYGON_COUNT

Perhaps better named POLYGON_LENGTHS or maybe POLYGON_POINTS_LENGTHS

POLYLINE_COUNT

Similar comment.

This refers to POLYGON_INDICES if it is defined, otherwise it refers to POSITION or POSITION_QUANTIZED.

I can see the use case for polylines, but are we sure about this for polygons? CC @bagnell

@bagnell
Copy link
Contributor

bagnell commented Aug 26, 2016

I can see the use case for polylines, but are we sure about this for polygons? CC @bagnell

We need to know the individual indices/positions of the polygons for picking and/or dynamically changing the polygons (we need to re-batch or reorder draw calls).

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 27, 2016

Ah, OK

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

TODO: Write schema after this gets looked over a bit.

I added this to a tasklist at the top of this PR. Please add any other items that need to be addressed before this is merged, e.g., review by @bagnell, etc.

If both `POSITION` and `POSITION_QUANTIZED` are defined, the higher precision `POSITION` will be used.
Per-feature semantics specific to a feature type are prefixed with the name of the feature type. e.g. `POLYGON` for polygons and `POLYLINE` for polylines.

At least one global `LENGTH` semantic must be defined.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and below in the Global Semantics table, I don't think we need to require this. Throughout 3D Tiles if length is zero (e.g., no feature table), the tile doesn't need to be rendered. Requiring that one of these be defined still does not guarantee that one is non-zero so it is not really a meaningful requirement.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

This refers to POLYGON_INDICES if it is defined, otherwise it refers to POSITION or POSITION_QUANTIZED.

I can see the use case for polylines, but are we sure about this for polygons? CC @bagnell

We need to know the individual indices/positions of the polygons for picking and/or dynamically changing the polygons (we need to re-batch or reorder draw calls).

Actually, my question is if this is for supporting triangle soup, and if so why can't we just require indices for polygons?

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

We'll also need to change the pnts and i3dm spec, but the Data Type column is confusing. For example, uint32 means "array of uint32" in the Vector Semantics table, but it means "unit32" in the Global Semantics.

Perhaps

  • Rename this to "Component Type" (we use this term elsewhere in 3D Tiles, right? It is from WebGL/glTF) in the Vector Semantics table with a sentence beforehand to say that these are arrays, even if it is redundant with the Feature Table spec.
  • Rename "Vector Semantics" to "Per-Feature Semantics"

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

Also consider renaming POLYGON and POLYLINE prefixes for per-feature semantics to POLYGONS and POLYLINES.

For example, POLYLINE_BATCH_ID sounds like one batch id, and POLYLINE_BATCH_IDS sounds like several batch ids for one polyline. POLYLINES_BATCH_ID sounds like what we intend.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

CLAMP_TO_GROUND

As discussed offline, I would drop this for now until we have the full game plan for extrusions, etc.


### Examples

This example will define positions for polylines, but the same procedure is applied for polygons as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't expect this to be true.

Polygons should always have indices.

Polylines should optionally have indices.

Copy link
Contributor

Choose a reason for hiding this comment

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

Polylines should optionally have indices.

Or not have indices and just be strips.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

Made a few tweaks in 09f1222, and also removed CLAMP_TO_GROUND, which had some misinformation.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

Can we make this explicit from #24?

  • The geometry is well behaved: there are no degenerate triangles nor are the duplicate vertex positions.

We should also be explicit that polygons do not have self-intersections (in WGS84 coordinates).

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

Can we allow per-feature indices arrays to either be uint16 or uint32?

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

Can we allow per-feature indices arrays to either be uint16 or uint32?

Same comment for batch ids throughout the spec.

@pjcozzi pjcozzi mentioned this pull request Aug 29, 2016
9 tasks
@lasalvavida
Copy link
Contributor Author

Actually, my question is if this is for supporting triangle soup, and if so why can't we just require indices for polygons?

During my discussion with @bagnell, I did bring this up, but he said it was reasonable to support both. If that changes, I'm happy to adjust the spec.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

During my discussion with @bagnell, I did bring this up, but he said it was reasonable to support both. If that changes, I'm happy to adjust the spec.

Given the "fast by default" spirit of 3D Tiles, I see no reason to encourage triangle soup.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

We also need to define if polygons and polylines are sub-sampled as geodesics or rhumb lines. This can be a global semantic as I think this needs to be an optional instead of just, for example, always going with rhumb lines.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 29, 2016

@bagnell after discussing offline with @lasalvavida, can we explore a more concise representation for positions for ground clamped positions? I think we can do even better than quantized x, y, z by taking advantage of the fact that the positions are on the ground.

Longitude/latitude is the obvious choice, but would require a lot of precision and runtime conversion.

Perhaps something in the tile's tangent plane or angular measurements from the tile's center. The key is to not require many bits, but still have good precise by using a local origin, and to have a fast conversion to WGS84 for rendering.

@pjcozzi pjcozzi mentioned this pull request Aug 29, 2016
15 tasks
@bagnell
Copy link
Contributor

bagnell commented Oct 6, 2016

Given the "fast by default" spirit of 3D Tiles, I see no reason to encourage triangle soup.

I agree.

@lasalvavida We discussed this offline, but what are POLYGON_COUNT and POLYGON_INDICES. Assuming that indices are now required, they are the number of positions and number of indices?

@bagnell
Copy link
Contributor

bagnell commented Oct 6, 2016

angular measurements from the tile's center

Isn't that the same as latitude/longitude or do you mean polar coordinates in the tangent plane?

Perhaps something in the tile's tangent plane

I like this idea, but I would be concerned with points on the plane with large distances to the ellipsoid. We would also run into the same problems that we have with triangulating polygons on the tangent plane in Cesium. Can we restrict the tile regions so they can never get that big?

Another idea is to store them in 2D (GeographicProjection or WebMercatorProjection). This would be expensive to convert back to WGS84.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 19, 2016

angular measurements from the tile's center

Isn't that the same as latitude/longitude or do you mean polar coordinates in the tangent plane?

Polar coordinates in the tangent plane

Perhaps something in the tile's tangent plane

I like this idea, but I would be concerned with points on the plane with large distances to the ellipsoid. We would also run into the same problems that we have with triangulating polygons on the tangent plane in Cesium. Can we restrict the tile regions so they can never get that big?

Another idea is to store them in 2D (GeographicProjection or WebMercatorProjection). This would be expensive to convert back to WGS84.

All good points, the question still remains:

can we explore a more concise representation for positions for ground clamped positions? I think we can do even better than quantized x, y, z by taking advantage of the fact that the positions are on the ground.

Perhaps profile to see how bad Cartographic-to-Cartesian is in practice. Perhaps there are optimizations if the points are all close by (there are definitely optimizations, for example, when the points are on the same line of longitude or latitude).

No rush, but we should explore or decide to punt before finalizing the initialize spec.

@pjcozzi pjcozzi mentioned this pull request Nov 22, 2016
2 tasks
@bagnell
Copy link
Contributor

bagnell commented Jun 1, 2017

Thanks for the initial work here @lasalvavida! I'm closing this since the format has changed significantly. I merged your commits to the vector-tiles branch in the main repo. I may draw from the history when we support arbitrary volumes.

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.

4 participants