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

Draco compression for pnts #310

Merged

ggetz merged 5 commits into master from point-cloud-draco

Sep 19, 2018

Contributor

lilleyse commented May 18, 2018 •

edited

Loading

This PR adds support for Draco compression to the Point Cloud tile format.

Do not merge yet - this is targeting 1.0-formatting just to minimize the amount of formatting differences, but will eventually target 3d-tiles-next.

Cesium implementation: CesiumGS/cesium#6559

TODO:

Schema updates


          Added DRACO to pnts

51389d9

ggetz reviewed

View reviewed changes

specification/TileFormats/PointCloud/README.md Outdated

+                 * `byteLength` specifies the length of the Draco binary data.
+                 * `semantics` is an array containing the per-point semantics contained in the Draco binary data. Allowed semantics are `"POSITION"`, `"RGBA"`, `"RGB"`, `"NORMAL"`, and `"BATCH_ID"`.
+              Point attribute data shall be encoded and decoded using the parameters in the following table.

Contributor

ggetz May 21, 2018

Should we specify that they must be encoded/decoded with the draco tools as well (and link to them)?

specification/TileFormats/PointCloud/README.md Outdated

               The `BATCH_ID` semantic may have a `componentType` of `UNSIGNED_BYTE`, `UNSIGNED_SHORT`, or `UNSIGNED_INT`. When `componentType` is not present, `UNSIGNED_SHORT` is used.
               The global semantic `BATCH_LENGTH` defines the number of unique `batchId` values, similar to the `batchLength` field in the [Batched 3D Model](../Batched3DModel/README.md) header.
+              ### Draco
+              Per-point semantics such as position, color, normal, and batch id may may be compressed with [Draco](https://google.github.io/draco/). Draco binary data is stored in the Feature Table binary and a list of the compressed per-point semantics is stored in the Feature Table JSON.

Contributor

ggetz May 21, 2018

batch id -> batchId or batch ID

specification/TileFormats/PointCloud/README.md Outdated

+              `RGB` | `uint8` | 3 | `COLOR`
+              `BATCH_ID` | `uint8`, `uint16`, or `uint32` | 1 | `GENERIC`
+              If a semantic type is defined in both `DRACO.semantics` and in the Feature Table JSON header, the Draco attribute data should be used for rendering.

Contributor

ggetz May 21, 2018

Also state that POSITION or POSITION_QUANTIZED is no longer required.

specification/TileFormats/PointCloud/README.md Outdated

+              };
+              // Use Draco encoder to build dracoBuffer
+              var featureTableBinary = dracoBuffer;

Contributor

ggetz May 21, 2018

I don't think this line in the example adds much. Instead I would say that the binary much be encoded with the Draco encoder in the section above and link to the encoder tool(s).

specification/TileFormats/PointCloud/README.md Outdated

               The `BATCH_ID` semantic may have a `componentType` of `UNSIGNED_BYTE`, `UNSIGNED_SHORT`, or `UNSIGNED_INT`. When `componentType` is not present, `UNSIGNED_SHORT` is used.
               The global semantic `BATCH_LENGTH` defines the number of unique `batchId` values, similar to the `batchLength` field in the [Batched 3D Model](../Batched3DModel/README.md) header.
+              ### Draco
+              Per-point semantics such as position, color, normal, and batch id may may be compressed with [Draco](https://google.github.io/draco/). Draco binary data is stored in the Feature Table binary and a list of the compressed per-point semantics is stored in the Feature Table JSON.

Contributor

ggetz May 21, 2018

Maybe add a quick sentence on why this would be used, eg. the glTF extension says

This allows glTF to support streaming compressed geometry data instead of the raw data

specification/TileFormats/PointCloud/README.md Outdated

+              `NORMAL` | `float32` | 3 | `NORMAL`
+              `RGBA` | `uint8` | 4 | `COLOR`
+              `RGB` | `uint8` | 3 | `COLOR`
+              `BATCH_ID` | `uint8`, `uint16`, or `uint32` | 1 | `GENERIC`

Contributor

ggetz May 21, 2018

How do we define the component here?

Contributor Author

lilleyse commented May 22, 2018 •

edited

Loading

Sorry for the churn, but I noticed a flaw with the current design yesterday. Since draco shuffles around the order of points we'll need to encode batch table properties in the draco buffer so that feature table properties and batch table properties are in sync.

This realization led to a few changes:

We need to store Draco attribute id's now that multiple properties may use the GENERIC type. Previously we just stored names of the encoded semantics.
Draco compression for pnts #310 (comment) - this comment brings up a good point, even though Draco encodes the type/component type in its buffer, we should still include the componentType/type in the json.
Draco point compression might be better off as an extension rather than core spec, like in glTF.

The new design is modeled more after glTF and looks like:

Feature Table

{
    POSITION: {
        byteOffset: 0 // ignored if draco encoded
    },
    RGB: {
        byteOffset: 0
    },
    BATCH_ID: {
        byteOffset: 0,
        componentType: 'UNSIGNED_BYTE'
    },
    extensions: {
        "3DTILES_draco_point_compression": {
            properties: {
                POSITION: 0,
                RGB: 1,
                BATCH_ID: 2
            },
            byteOffset: 0,
            byteLength: 100
        }
    }
}

Batch Table

{
    Intensity: {
        byteOffset: 0,
        type: 'SCALAR',
        componentType: 'UNSIGNED_BYTE'
    },
    Classification: {
        byteOffset: 0,
        type: 'SCALAR',
        componentType: 'UNSIGNED_BYTE'
    }
    extensions: {
        "3DTILES_draco_point_compression": {
            properties: {
                Intensity: 3,
                Classiciation: 4
            }
        }
    }
}

If a property is encoded in Draco its byteOffset is ignored.

Contributor Author

lilleyse commented May 22, 2018

One other thing, the draco buffer is still stored in the feature table binary but this is pretty awkward when batch table properties are stored there as well. Ideally draco would be its own buffer after the feature table and batch table sections but I wasn't sure if this was worth it since it complicates the overall tile design.

lilleyse mentioned this pull request

Added Draco support to point clouds CesiumGS/cesium#6559

Merged

3 tasks

Contributor

pjcozzi commented Jun 4, 2018

Why does this target the 1.0 branch? Isn't that branch just for the OGC Community Standard? Even as an extension, this should target a 3d-tiles-next branch.

Contributor Author

lilleyse commented Jun 4, 2018

I mentioned this in the PR description:

Do not merge yet - this is targeting 1.0-formatting just to minimize the amount of formatting differences, but will eventually target 3d-tiles-next.

Due to all the formatting changes and moving around of files I thought it would be best to write this extension against the most recent spec. Once 3d-tiles-next is updated to use the new formatting as well I can retarget to that branch.

Contributor

pjcozzi commented Jun 4, 2018

Ah, gotcha.

lilleyse changed the base branch from 1.0-formatting to 1.0

June 13, 2018 12:59

lilleyse added 2 commits

July 5, 2018 20:28


          Merge branch '1.0' into point-cloud-draco

a4a2e03


          Reverted changes to PointCloud/README.md

88c4cf2

lilleyse force-pushed the point-cloud-draco branch 9 times, most recently from 48e0064 to b7f41ea Compare

July 6, 2018 03:08


          Updated extension

d8ea050

lilleyse force-pushed the point-cloud-draco branch from b7f41ea to d8ea050 Compare

July 6, 2018 03:09

Contributor Author

lilleyse commented Jul 6, 2018

@ggetz updated. Rewritten to account for the changes in #310 (comment). I think I've addressed most of the initial feedback.

This is still targeting 1.0. After 1.0 is merged into master this should be ok to merge into master.

ggetz requested changes

View reviewed changes

Contributor

ggetz left a comment

Thanks @lilleyse! Just some comments to clarify, looks good otherwise.

extensions/3DTILES_draco_point_compression/README.md Outdated


		### Batch Table

		Per-point metadata can also be compressed. The Batch Table may be extended to include a `3DTILES_draco_point_compression` object that defines additional compressed properties.

Contributor

ggetz Jul 6, 2018

Per-point metadata can also be compressed.

Maybe in the "Feature Table" section, define when you would specify a property there vs in the Batch Table.

extensions/3DTILES_draco_point_compression/README.md Outdated

+              When a semantic is compressed its `byteOffset` property is ignored and may be set to zero. Its `componentType`, if present,
+              defines the component type of the uncompressed data.
+              Each property is associated with a unique ID. This ID is used by the Draco decoder to get the property data from the compressed data.

Contributor

ggetz Jul 6, 2018

Unique only to the list of properties, correct? They do not reference the index of a property. Maybe just clarify a little.

extensions/3DTILES_draco_point_compression/README.md Outdated

+              When a property is compressed its `byteOffset` property is ignored and may be set to zero. Its `componentType` and `type` properties
+              define the component type and type, respectively, of the uncompressed data.
+              Each property is associated with a unique ID. This ID is also unique among properties in the Feature Table extension.

Contributor

ggetz Jul 6, 2018

Oh, that answers my "what is this unique to" question. I think you may want to re-iterate this above, and maybe link here.

extensions/3DTILES_draco_point_compression/README.md Outdated

+              If some properties are compressed and others are not, the Draco encoder must apply the `POINT_CLOUD_SEQUENTIAL_ENCODING` encoding method.
+              This ensures that Draco preserves the original ordering of point data.
+              > **Implementation Note:** For best results when using Draco, all properties in the Feature Table and Batch Table should be Draco compressed, in which

Contributor

ggetz Jul 6, 2018

What are the best results? Faster performance, smaller file sizes?

extensions/3DTILES_draco_point_compression/README.md

+              }
+              ```
+              #### properties

Contributor

ggetz Jul 6, 2018

Generate Documentation with wetzel?

extensions/3DTILES_draco_point_compression/README.md Outdated


		This extension is based on [Draco bitstream version 2.2](https://google.github.io/draco/spec/), which is normative and included in scope.

		## Point Cloud schema updates

Contributor

ggetz Jul 6, 2018

I don't think schema also encompass changes to the binary body? I would say Point Cloud tile format updates, and have a section for the JSON schema updates and binary body updates for both the Feature and Batch Tables, to make it clear that both are required.

Contributor Author

lilleyse Jul 8, 2018

I changed the title, however I'm not sure if there are any binary body updates, besides that the extension references data in the binary body.

I left it as one section but added a Schema updates section within the Feature Table and Batch Table sections.

...ILES_draco_point_compression/schema/3DTILES_draco_point_compression.featureTable.schema.json Outdated

+                          "type": "number",
+                          "minimum": 0
+                      }
+                  },

Contributor

ggetz Jul 6, 2018

Should we disallow additional properties?

lilleyse force-pushed the point-cloud-draco branch 6 times, most recently from eb9ed95 to 73ed516 Compare

July 8, 2018 22:14

Contributor Author

lilleyse commented Jul 8, 2018

@ggetz Updated!


          Updates

b061080

lilleyse force-pushed the point-cloud-draco branch from 73ed516 to b061080 Compare

July 8, 2018 22:24

Contributor

ggetz commented Jul 9, 2018

Looks good, thanks @lilleyse ! Did we want to merge this into 1.0 or wait until 1.0 is in master? I think it should be fine to merge into 1.0 since it's an extension.

Contributor Author

lilleyse commented Jul 9, 2018

I would wait to merge this into master.

lilleyse changed the base branch from 1.0 to master

August 7, 2018 21:56

Contributor Author

lilleyse commented Aug 7, 2018

@ggetz I retargeted this to master. This should be ready to go now.

Contributor

pjcozzi commented Sep 19, 2018

What is the plan with this PR? If this goes into master, there should be a separate ogc-community-standard-1.0 branch unless this was included in the Community Standard submission. Perhaps that is the best approach compared to merging this into a 3d-tiles-next branch.

pjcozzi mentioned this pull request

Option/extension for draco point compression #188

Closed

Contributor Author

lilleyse commented Sep 19, 2018

I think the plan was to merge into master still. We have a 1.0 tag that marks when the OGC community standard submission was released which can be converted to a branch if needed.

Contributor

ggetz commented Sep 19, 2018

The 1.0 tag is also what we linked to from the OGC documents. So I think this should be fine to go into master. Otherwise changes here look good to me and this should be ready to go.

ggetz approved these changes

View reviewed changes

ggetz merged commit 142549c into master

ggetz deleted the point-cloud-draco branch

September 19, 2018 20:55

Contributor

pjcozzi commented Sep 19, 2018

Please make sure there is a clear link to the OGC version from the main link.

Also, how will the tag work and bug fixes go into the OGC version?

Contributor

ggetz commented Sep 19, 2018

I can add a link. We can branch off the tag (now or later), and we can update the tag as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet