Add .dds support to GLTFLoader #13820

Bergmam · 2018-04-10T12:26:43Z

I implemented support for using the GLTFLoader with .gltf-files containing the MSFT_texture_dds extension (https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Vendor/MSFT_texture_dds/README.md)

If the GLTF-file loaded by the GLTFLoader contains the extension, the loader will now import DDSLoader.js and load the correct .dds files as specified by the extension.

I also added another version of the Boombox gltf model that uses the extension with .dds image files, as well as extended the webgl_loader_gltf_extensions example to showcase the GLTFLoader functionality on the new Boombox model.

…files. Add dds boombox to gltf extensions example.

olleman42 · 2018-04-10T12:31:33Z

👍

takahirox · 2018-04-10T12:37:48Z

examples/js/loaders/GLTFLoader.js

@@ -1780,7 +1786,6 @@ THREE.GLTFLoader = ( function () {
 	 * @return {Promise<THREE.Texture>}
 	 */
 	GLTFParser.prototype.loadTexture = function ( textureIndex ) {
-


I think no need to remove an empty line here.

takahirox · 2018-04-10T12:38:52Z

examples/js/loaders/GLTFLoader.js

+		if ( textureExtensions[ EXTENSIONS.MSFT_TEXTURE_DDS ] ) {
+
+			var DDSLoader = new THREE.DDSLoader;
+			source = json.images[ textureExtensions[ EXTENSIONS.MSFT_TEXTURE_DDS ].source ];


I want to display warning or error if DDSLoader isn't imported.

takahirox · 2018-04-10T12:44:56Z

examples/js/loaders/GLTFLoader.js

+
+				var loader = THREE.Loader.Handlers.get( sourceURI ) || textureLoader;
+
+			}


Let's move var loader out from if else for the consistency with other parts.

var loader; if ( textureExtensions[ EXTENSIONS.MSFT_TEXTURE_DDS ] ) { loader = DDSLoader; } else { loader = THREE.Loader.Handlers.get( sourceURI ) || textureLoader; }

takahirox · 2018-04-10T13:17:59Z

examples/webgl_loader_gltf_extensions.html

 		<script src="js/loaders/GLTFLoader.js"></script>
-
+		


No need to add tab here.

takahirox · 2018-04-10T13:21:57Z

examples/js/loaders/GLTFLoader.js

+		var source;
+		if ( textureExtensions[ EXTENSIONS.MSFT_TEXTURE_DDS ] ) {
+
+			var DDSLoader = new THREE.DDSLoader;


Can we reuse one DDSLoader for all dds textures like textureLoader?

takahirox · 2018-04-10T13:22:29Z

examples/js/loaders/GLTFLoader.js

@@ -120,7 +120,6 @@ THREE.GLTFLoader = ( function () {
 				return;

 			}
-


No need to remove an empty line here.

takahirox · 2018-04-10T13:23:44Z

examples/webgl_loader_gltf_extensions.html

@@ -71,7 +72,7 @@
 					cameraPos: new THREE.Vector3( 0.02, 0.01, 0.03 ),
 					objectRotation: new THREE.Euler( 0, Math.PI, 0 ),
 					addLights: true,
-					extensions: [ 'glTF', 'glTF-pbrSpecularGlossiness', 'glTF-Binary' ],
+					extensions: [ 'glTF', 'glTF-pbrSpecularGlossiness', 'glTF-Binary', 'glTF-dds'],


You need a space after 'glTF-dds'

takahirox · 2018-04-10T13:32:21Z

examples/js/loaders/GLTFLoader.js

@@ -147,6 +146,12 @@ THREE.GLTFLoader = ( function () {

 				}

+				if ( json.extensionsUsed.indexOf( EXTENSIONS.MSFT_TEXTURE_DDS ) >= 0 ) {
+
+					extensions[ EXTENSIONS.MSFT_TEXTURE_DDS ] = "MSFT_texture_dds";


How about making GLTFTextureDDSExtension() for the consistency with other extensions? And if you instantiate DDSLoader there, it can be reused.

function GLTFTextureDDSExtension() { this.name = EXTENSIONS.MSFT_TEXTURE_DDS; this.loader = new THREE.DDSLoader(); }

+1. I would also throw the error at this point in the constructor if DDSLoader doesn't exist.

Now initializing THREE.DDSLoader in GLTFTextureDDSExtension, also added throwing an error if it does not exist

takahirox · 2018-04-10T13:34:15Z

Nice work! I commented mainly for Three.js / GLTFLoader code style.

donmccurdy · 2018-04-10T14:21:51Z

examples/js/loaders/GLTFLoader.js

+
+				loader = THREE.Loader.Handlers.get( sourceURI ) || textureLoader;
+
+			}


If THREE.Loader.Handlers.get( sourceURI ) returns a loader, it should override even the DDSLoader extension. Loader.Handlers is an escape hatch for getting images from drag-and-drop events, workers, or other custom sources. You can assume that it will return a DDSLoader for .dds files, if the developer is using it properly.

I changed it to

if ( textureExtensions[ EXTENSIONS.MSFT_TEXTURE_DDS ] ) { loader = THREE.Loader.Handlers.get( sourceURI ) || parser.extensions[EXTENSIONS.MSFT_TEXTURE_DDS].ddsLoader }

Please verify if correct or if I misunderstood. :)

The logic looks right, yes. Might trim it a little:

var loader = THREE.Loader.Handlers.get( sourceURI ); if ( ! loader ) { loader = textureExtensions[ EXTENSIONS.MSFT_TEXTURE_DDS ] ? parser.extensions[ EXTENSIONS.MSFT_TEXTURE_DDS ].ddsLoader : textureLoader; }

donmccurdy · 2018-04-10T14:23:57Z

examples/js/loaders/GLTFLoader.js

+
+			}
+
+			var DDSLoader = new THREE.DDSLoader;


nit: we use () on constructors.

Bergmam · 2018-04-11T07:35:30Z

All above comments should be fixed now.

donmccurdy · 2018-04-12T00:43:00Z

examples/js/loaders/GLTFLoader.js

+
+				throw new Error( 'THREE.GLTFLoader: Attempting to load .dds texture without importing THREE.DDSLoader' );
+
+			}


This error would need to be thrown in the constructor of GLTFTextureDDSExtension, I think it will error out before it gets here otherwise.

My bad, forgot to move it after I implemented GLTFTextureDDSExtention function. Fixed now :)

donmccurdy · 2018-04-12T00:47:54Z

examples/js/loaders/GLTFLoader.js

+
+				loader = THREE.Loader.Handlers.get( sourceURI ) || textureLoader;
+
+			}


The logic looks right, yes. Might trim it a little:

var loader = THREE.Loader.Handlers.get( sourceURI ); if ( ! loader ) { loader = textureExtensions[ EXTENSIONS.MSFT_TEXTURE_DDS ] ? parser.extensions[ EXTENSIONS.MSFT_TEXTURE_DDS ].ddsLoader : textureLoader; }

donmccurdy · 2018-04-12T00:48:18Z

examples/js/loaders/GLTFLoader.js

+	function GLTFTextureDDSExtension() {
+
+		this.name = EXTENSIONS.MSFT_TEXTURE_DDS;
+    this.ddsLoader = new THREE.DDSLoader();


nit: spaces -> tabs

donmccurdy · 2018-04-12T19:48:15Z

Looks good! Demo.

This is in the category of extensions that are harder to do outside the loader, because without it the loader will still request the (backup) uncompressed versions of textures, so I am OK with merging it into three.js despite its being a vendor extension. We will not necessarily support other MSFT_ extensions, and may remove this one when a Khronos or EXT_ alternative becomes available.

The only remaining issue from my perspective is this: with this change, the loader will always request compressed textures when the extension is present. On devices that don't support DDS (mobile) we should be falling back to uncompressed textures or alternative compression instead. I believe the way to test for this would be renderer.extensions.get('WEBGL_compressed_texture_s3tc')...

Do other three.js loaders have an API for this? If not, I'd propose:

var useDDS = !! renderer.extensions.get( 'WEBGL_compressed_texture_s3tc' );
loader
  .setExtensionEnabled( 'MSFT_texture_dds', useDDS )
  .load( 'foo.glb', () => {...} );

EDIT: The above can likely be addressed in another PR, too.

donmccurdy

Actually, I think I'd rather deal with enable/disable flags in a later PR. Approved!

mrdoob · 2018-04-12T23:46:45Z

Thanks!

donmccurdy · 2021-02-13T03:50:38Z

As an FYI, I'm recommending that we remove DDS support from future versions of GLTFLoader — we have cross-platform compressed texture support available through the KHR_texture_basisu extension now, and would like to steer users toward that for greater compatibility. Thanks again for this PR, bridging the 2.5 year gap to this point.

See: #21271

takahirox · 2021-02-13T03:58:20Z

Kudos to @Bergmam

Add .dds support to GLTFLoader. Add new Boombox gltf with .dds image …

21c4210

…files. Add dds boombox to gltf extensions example.

takahirox reviewed Apr 10, 2018

View reviewed changes

examples/webgl_loader_gltf_extensions.html Outdated

<script src="js/loaders/GLTFLoader.js"></script>

Copy link

Collaborator

takahirox Apr 10, 2018

Choose a reason for hiding this comment

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

No need to add tab here.

takahirox reviewed Apr 10, 2018

View reviewed changes

Add error if DDSLoader is not present, remove unnecessary newrow

7bdba71

takahirox reviewed Apr 10, 2018

View reviewed changes

moved var outside if/else

d3409a8

donmccurdy reviewed Apr 10, 2018

View reviewed changes

examples/js/loaders/GLTFLoader.js Outdated

}

var DDSLoader = new THREE.DDSLoader;

Copy link

Collaborator

donmccurdy Apr 10, 2018

Choose a reason for hiding this comment

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

nit: we use () on constructors.

Anton Bergman added 2 commits April 11, 2018 07:54

Fix spaces and tabs

f41987b

Add extension init function, fixes for pr comments

a7a8767

donmccurdy reviewed Apr 12, 2018

View reviewed changes

Moved error throwing to constructor, added suggested changes from pr

2a0c1c0

donmccurdy approved these changes Apr 12, 2018

View reviewed changes

mrdoob added this to the r92 milestone Apr 12, 2018

mrdoob merged commit 83ec0a1 into mrdoob:dev Apr 12, 2018

donmccurdy mentioned this pull request Sep 20, 2018

MSFT texture and packing extensions DDS requirements KhronosGroup/glTF#1359

Open

donmccurdy mentioned this pull request Feb 13, 2021

GLTFLoader: Remove support for MSFT_texture_dds. #21271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add .dds support to GLTFLoader #13820

Add .dds support to GLTFLoader #13820

Bergmam commented Apr 10, 2018 •

edited

Loading

olleman42 commented Apr 10, 2018

takahirox Apr 10, 2018

takahirox Apr 10, 2018

takahirox Apr 10, 2018

takahirox Apr 10, 2018

takahirox Apr 10, 2018

takahirox Apr 10, 2018

takahirox Apr 10, 2018

takahirox Apr 10, 2018

donmccurdy Apr 10, 2018

Bergmam Apr 11, 2018

takahirox commented Apr 10, 2018

donmccurdy Apr 10, 2018 •

edited

Loading

Bergmam Apr 11, 2018

donmccurdy Apr 12, 2018

donmccurdy Apr 10, 2018

Bergmam commented Apr 11, 2018

donmccurdy Apr 12, 2018

Bergmam Apr 12, 2018

donmccurdy Apr 12, 2018

donmccurdy Apr 12, 2018

donmccurdy commented Apr 12, 2018 •

edited

Loading

donmccurdy left a comment

mrdoob commented Apr 12, 2018

donmccurdy commented Feb 13, 2021

takahirox commented Feb 13, 2021


		var loader = THREE.Loader.Handlers.get( sourceURI ) \|\| textureLoader;

		}

		@@ -120,7 +120,6 @@ THREE.GLTFLoader = ( function () {
		return;

		}


		loader = THREE.Loader.Handlers.get( sourceURI ) \|\| textureLoader;

		}


		throw new Error( 'THREE.GLTFLoader: Attempting to load .dds texture without importing THREE.DDSLoader' );

		}

Add .dds support to GLTFLoader #13820

Add .dds support to GLTFLoader #13820

Conversation

Bergmam commented Apr 10, 2018 • edited Loading

olleman42 commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox commented Apr 10, 2018

donmccurdy Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bergmam commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy commented Apr 12, 2018 • edited Loading

donmccurdy left a comment

Choose a reason for hiding this comment

mrdoob commented Apr 12, 2018

donmccurdy commented Feb 13, 2021

takahirox commented Feb 13, 2021

Bergmam commented Apr 10, 2018 •

edited

Loading

donmccurdy Apr 10, 2018 •

edited

Loading

donmccurdy commented Apr 12, 2018 •

edited

Loading