Skip to content

Conversation

@Mcgode
Copy link
Contributor

@Mcgode Mcgode commented Oct 30, 2020

Related issues: GLTFExporter and TextureData

Description

Adds support of DataTexture images for GLTFExporter.

The DataTexture image data will be converted to RGBA, downscaled linearly if needed (due to maxTextureSize or forcePowerOfTwoTextures options), then put into an ImageData object and then proceed to be put to the canvas.

The example file has also been edited to test the feature: a linear gradient DataTexture is generated, added as a map to the sphere mesh.

So far, the supported DataTexture formats are:

  • RGBAIntegerFormat
  • RGBAFormat
  • RGBEFormat (note: linear downscale may have issues)
  • RGBIntegrerFormat
  • RGBFormat
  • RGIntegerFormat
  • RGFormat
  • RedIntegerFormat
  • RedFormat
    For now, other formats will result in an explicit error

This contribution is funded by Dioxygen Software for Dualbox.com

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 30, 2020

I don't think it makes sense to support anything except of RGB(A). If users create data texture let's say with RedFormat, the probably expect to have this format again after the import. We should also not promote to let users export HDR textures.

@donmccurdy
Copy link
Collaborator

^Probably not necessary to support more than RGB(A) within GLTFExporter, anyway, to keep this simple.

That said, a full getImageData method could arguably be a fit for ImageUtils? Is there a particular reason ImageUtils is in core and not examples/*?

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 30, 2020

ImageUtils is actually required in Texture.toJSON().

Somewhat related to this PR: There is actually another one for serializing/deserializing data textures in general: #17745

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 2, 2020

I don't think it's necessary to support maxTextureSize and forcePowerOfTwoTextures like mentioned here. Besides, the integer format support is also not required. TBH, I think it's totally sufficient if the PR just adds:

if ( ( typeof HTMLImageElement !== 'undefined' && image instanceof HTMLImageElement ) ||
	( typeof HTMLCanvasElement !== 'undefined' && image instanceof HTMLCanvasElement ) ||
	( typeof ImageBitmap !== 'undefined' && image instanceof ImageBitmap ) ) {

	ctx.drawImage( image, 0, 0, canvas.width, canvas.height );

} else {

        // data texture must be of type RGBA

	var imageData = new ImageData( new Uint8ClampedArray( image.data.buffer ), image.width, image.height );
	ctx.putImageData( imageData, 0, 0 );

}

@Mcgode
Copy link
Contributor Author

Mcgode commented Nov 2, 2020

@Mugen87 For now, my opinion is that we should keep the maxTextureSize and forcePowerOfTwo option support until it is discontinued globally, otherwise it might lead to weird issues due to image cropping.

I also think RGB support should be kept as a feature as well. It is a widely used format, and the lack of this feature could be infuriating to users who have to do the conversion manually.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 2, 2020

It is a widely used format, and the lack of this feature could be infuriating to users who have to do the conversion manually.

I'm not sure I understand this argument. Data textures are generated by the user. If they generate textures for glTF assets, they just have to adjust an existing routine. No "conversion" is required. Hence, maxTextureSize and forcePowerOfTwo are also redundant in this context.

@Mcgode
Copy link
Contributor Author

Mcgode commented Nov 2, 2020

I'm not sure I understand this argument. Data textures are generated by the user. If they generate textures for glTF assets, they just have to adjust an existing routine.

My issue with this approach is that it imposes an arbitrary constraint sooner in the pipeline that might a pain to work with.
A user might prefer to work with an RGB data structure rather than an RGBA data structure because it fits his needs better.
Forcing him to use RGBA will force him to find a workaround, by implementing another image generation algorithm, or by having him do the conversion himself.

This is, in my opinion, unecessary added complexity. Using an efficient embedded conversion function to the exporter process removes that complexity for the user. To me, imposing this to the user also goes against the "easy-to-use" motto of three.js

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 2, 2020

Another motto of three.js is to avoid unnecessary complexity and over-engineering in the engine. Hence, I do not vote to merge the PR in its current state and suggest a more simple solution like proposed in #20588 (comment).

@Mcgode
Copy link
Contributor Author

Mcgode commented Nov 2, 2020

Hence, maxTextureSize and forcePowerOfTwo are also redundant in this context.

I'm not sure I follow your logic here.

The issue is that if those options are ignored for DataTexture objects, but are enabled nonetheless by the exporter, the image will be cropped.
ctx.putImageData( imageData, 0, 0 ); does not downscale the image to fit it into the canvas, which size is overriden by those options.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 2, 2020

Like I said before, color data textures are generated by the user like you did it in your example. Since the user is in full control over this logic, he can already ensure proper texture sizes. It does not make sense to generate a larger texture and then ask the exporter to shrink it down.

@Mcgode
Copy link
Contributor Author

Mcgode commented Nov 2, 2020

Another motto of three.js is to avoid unnecessary complexity and over-engineering in the engine. Hence, I do not vote to merge the PR in its current state and suggest a more simple solution like proposed in #20588 (comment).

If the issue is to keep internal code to a minimum for maintenance, I can compact the conversion code to something like this :

if ( ( typeof HTMLImageElement !== 'undefined' && image instanceof HTMLImageElement ) ||
	( typeof HTMLCanvasElement !== 'undefined' && image instanceof HTMLCanvasElement ) ||
	( typeof ImageBitmap !== 'undefined' && image instanceof ImageBitmap ) ) {

	ctx.drawImage( image, 0, 0, canvas.width, canvas.height );

} else {

        // data texture must be of type RGBA or RGB

	let data = image.data;
        if ( format === THREE.RGBFormat ) {

            data = new Uint8ClampedArray( image.height * image.width * 4 );
            data.forEach( function ( _, i ) { data[ i ] = i % 4 === 3 ? 255 : image.data[ 3 * Math.floor( i / 4 ) + i % 4 ] } );

        }
	ctx.putImageData( data, 0, 0 );

}

Would that be acceptable ?

@Mcgode
Copy link
Contributor Author

Mcgode commented Nov 2, 2020

Like I said before, color data textures are generated by the user like you did it in your example. Since the user is in full control over this logic, he can already ensure proper texture sizes. It does not make sense to generate a larger texture and then ask the exporter to shrink it down.

Okay, I can agree on that.
Though I think we should probably have some explicit warning in cases where the texture is going to be cropped. What's your opinion?

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 2, 2020

It's best when @mrdoob decides about this. Your new code is definitely more appropriate 👍 .

@Mcgode
Copy link
Contributor Author

Mcgode commented Nov 2, 2020

I'll push the new code right away then

@mrdoob mrdoob added this to the r123 milestone Nov 21, 2020
@mrdoob mrdoob merged commit 66e4d92 into mrdoob:dev Nov 22, 2020
@mrdoob
Copy link
Owner

mrdoob commented Nov 22, 2020

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Nov 22, 2020

@Mcgode I've done some clean up 6910a7a

Do you mind testing the code?

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 23, 2020

There is a minor bug in the stride computation for the RGB texture. I'll file a PR.

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