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

ImageUtils.getDataURL: Added warning when saving image as jpg. #21386

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Mar 2, 2021

Description

Log a warning when converting a image to jpeg due to its dimensions.
Seems like I forgot what issue/pr discussed this a few days ago. @Mugen87 do you remember?

@mrdoob mrdoob added this to the r127 milestone Mar 2, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 2, 2021

Seems like I forgot what issue/pr discussed this a few days ago. @Mugen87 do you remember?

No sorry. But the warning does make sense.

@mrdoob mrdoob merged commit 78c2e35 into dev Mar 2, 2021
@mrdoob mrdoob deleted the mrdoob-patch-1 branch March 2, 2021 11:49
@gsimone
Copy link
Contributor

gsimone commented Mar 22, 2021

Hey 👋 I just stumbled onto this, should we think about making this optimization skippable?

This lets users select the output format. Of course one can just patch this function away, but it's weird how opinionated the default is – I'm also missing the context here, so forgive me 🙏

getDataURL: function ( image, format ) {

    ...
    if (typeof format === "undefined") {

      if ( canvas.width > 2048 || canvas.height > 2048 ) {

        console.warn( 'THREE.ImageUtils.getDataURL: Image converted to jpg for performance reasons', image );

        return canvas.toDataURL( 'image/jpeg', 0.6 );

      } else {

        return canvas.toDataURL( 'image/png' );

      }
      
    } else {

      if (format === "png") {

         return canvas.toDataURL( 'image/png' );
         
      } else {
        
        return canvas.toDataURL( 'image/jpeg', 0.6 );
        
      }
  }
  ...

}

ADD 1: Also the fact that jpeg is always saved with quality 0.6, we could even expose toDataUrl arguments like this:

(from https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/toDataURL )

getDataURL(image, type, encoderOptions) {
  ...
}

This could also be reflect in Texture.toJSON, with additional type and encoderOptions arguments to allow more control to users

@mrdoob
Copy link
Owner Author

mrdoob commented Mar 22, 2021

@gsimone do you mind detailing a bit what your use case is?

@gsimone
Copy link
Contributor

gsimone commented Mar 23, 2021

We use Texture.toJson to serialize user-uploaded textures that might exceed 2048x2048, so a user uploaded a transparent png that was like 5k * 1k and it lost alpha information when it was serialized. We can of course work around this, but it's something that people might stumble into, might be worth at least documenting — although I'm still not sure why it's necessary

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.

3 participants