Feature: PNTs Draco support #326
Conversation
There was a problem hiding this comment.
Thanks! This looks like a good thing to add.
It also allowed me to use the three.js DRACOLoader class which I don't think supports arbitrary Draco attributes either (I could be wrong there).
This is something we can clarify if we need to broaden support to other fields like NORMAL or BATCH_ID.
It only supports Positions & Colors currently.
This seemed fine because 3DTilesRendererJS only supports those attributes currently.
For now, yes - it looks like any non-glTF file formats are deprecated in the latest 1.1 spec (see #323 (comment)) so I'm no less worried about supporting these formats in a completely robust way at the moment.
I'm not 100% happy with the way in which I inject the Draco loader (via the the .drc handler) but it seemed like the lightest touch method that vaguely makes sense.
Yeah but I don't know if I like the alternatives, either, especially given the above point:
- Specifically we could have a
setDRACOLoaderfunction on the tiles class. - Ask the user to provide a custom handler for ".pnts" files that already has the draco loader set the same way we do for GLTF.
If anything maybe the second option makes the most sense? Though I'm not picky here.
src/three/PNTSLoader.js
Outdated
| for ( const [ key, value ] of Object.entries( dracoIDs ) ) { | ||
|
|
||
| } | ||
| attributeIDs[ DRACO_ATTRIBUTE_MAP[ key ] ] = value; | ||
|
|
||
| } ); | ||
| } |
There was a problem hiding this comment.
This will cause an undefined key if a DRACO property exists that is not "RGB" or "POSITION", right?
src/three/PNTSLoader.js
Outdated
| const dracoGeometry = await this.dracoLoader | ||
| .decodeGeometry( buffer, taskConfig ); | ||
|
|
||
| geometry.copy( dracoGeometry ); |
There was a problem hiding this comment.
Why copy instead of just using the DRACO geometry directly?
|
|
||
| if ( geometry.attributes.color ) { | ||
|
|
||
| geometry.attributes.color.normalized = true; |
There was a problem hiding this comment.
Does the DRACO loader not set the color field to normalized correctly automatically? Is this something that should be fixed in three.js?
There was a problem hiding this comment.
Good idea, I'll have a look
There was a problem hiding this comment.
made an issue and PR here
unsure if this desired behaviour for everyone, but feels like it
src/three/PNTSLoader.js
Outdated
| // hacky way of getting the draco loader from the manager | ||
| this.dracoLoader = this.manager.getHandler( 'draco.drc' ); |
There was a problem hiding this comment.
I think this should be done in the "parse" function rather than in the constructor.
| isDraco() { | ||
|
|
||
| const { extensions } = this.header; | ||
| return extensions && !! extensions[ '3DTILES_draco_point_compression' ]; | ||
|
|
||
| } | ||
|
|
||
| getDracoBuffer() { | ||
|
|
||
| if ( ! this.isDraco() ) { | ||
|
|
||
| throw new Error( 'FeatureTable: Feature data is not draco encoded' ); | ||
|
|
||
| } | ||
|
|
||
| const { buffer, binOffset } = this; | ||
|
|
||
| const { byteOffset, byteLength } = this.header.extensions[ '3DTILES_draco_point_compression' ]; | ||
|
|
||
| return buffer.slice( binOffset + byteOffset, binOffset + byteOffset + byteLength ); | ||
|
|
||
| } | ||
|
|
||
| getDracoProperties() { | ||
|
|
||
| if ( ! this.isDraco() ) { | ||
|
|
||
| throw new Error( 'FeatureTable: Feature data is not draco encoded' ); | ||
|
|
||
| } | ||
|
|
||
| return this.header.extensions[ '3DTILES_draco_point_compression' ].properties; | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
I think ideally the FeatureTable class wouldn't have to change since this is all really just relevant to the points properties, right? Otherwise if there are other relevant compressed buffers in the FeatureTable then it might make most sense to make a derivative class that abstracts away the draco decompression and automatically decompresses the data when a user calls "getData", for example.
There was a problem hiding this comment.
I've now moved more of the logic to a PNTSFeatureTable to keep the draco related logic away from other data types.
I wasn't able to abstract it fully so that you could just use getData() as normal. This was due to the dracoLoader returning geometry not just a typed array.
|
Just published! Thanks again |
This PR provides support for PNTs files that are using the
3DTILES_draco_point_compressionextension.It only supports Positions & Colors currently.
This seemed fine because 3DTilesRendererJS only supports those attributes currently.
It also allowed me to use the three.js DRACOLoader class which I don't think supports arbitrary Draco attributes either (I could be wrong there).
I'm not 100% happy with the way in which I inject the Draco loader (via the the .drc handler) but it seemed like the lightest touch method that vaguely makes sense.