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

Added RGBMLoader. #21145

Merged
merged 1 commit into from
Jan 26, 2021
Merged

Added RGBMLoader. #21145

merged 1 commit into from
Jan 26, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jan 25, 2021

Related issue: Fixed #20786

Description

Added RGBMLoader so RGBM encoded textures also work on iOS. The loader achieves this by parsing PNG files based on UPNG.js.

@WestLangley
Copy link
Collaborator

As in RGBELoader, I think there should be an onLoadCallback() where the following are explicitly set to appropriate values for the RGBM loader. Just for clarity.

texture.encoding
texture.minFilter
texture.magFilter
texture.generateMipmaps
texture.flipY

I am not sure if PMREM handles RGBM correctly or not...

@WestLangley
Copy link
Collaborator

WestLangley commented Jan 25, 2021

webgl_loader_texture_rgbm.html is rendering upside down in this PR.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 25, 2021

webgl_loader_texture_rgbm.html is rendering upside down in this PR.

Have you rebuild three.module.js? Without it, it won't work.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 25, 2021

As in RGBELoader, I think there should be an onLoadCallback() where the following are explicitly set to appropriate values for the RGBM loader. Just for clarity.

I wanted to avoid the additional code since DataTextureLoader.parse() already configures the texture properties correctly. Besides, I personally prefer to only set values which differ from the default value. I've realized in the past that this approach is easier to read and understand.

@WestLangley
Copy link
Collaborator

I disagree 100%. Appropriate values must be set by the loader so the user does not have to worry about it.

Please advise:

  1. Should RGBM textures have generateMipmaps set?

  2. What should be the min/max filtering?

  3. Why does the RGBM loader set the encoding to linear when linear is clearly incorrect?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 25, 2021

Why does the RGBM loader set the encoding to linear when linear is clearly incorrect?

Why was linear filtering configured in the existing code?

@@ -44,14 +46,10 @@

camera = new THREE.OrthographicCamera( - aspect, aspect, 1, - 1, 0, 1 );

new THREE.TextureLoader().load( 'textures/memorial.png', function ( texture ) {
new RGBMLoader().load( 'textures/memorial.png', function ( texture ) {

texture.encoding = THREE.RGBM16Encoding;
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't RGBMLoader set this encoding by default?

Copy link
Collaborator Author

@Mugen87 Mugen87 Jan 25, 2021

Choose a reason for hiding this comment

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

There is also RGBM7Encoding. I don't think it's possible to detect which encoding is used in a given texture so I thought it's better to set it in the example code.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, do we need RGBM7Encoding? 😅

Considering this was not working on iOS, I suspect this doesn't have't that much real usage...

@vjevremovic do we need both RGBM7 and RGBM16? Or can we just pick one?

export const RGBEEncoding = 3002;
export const LogLuvEncoding = 3003;
export const RGBM7Encoding = 3004;
export const RGBM16Encoding = 3005;
export const RGBDEncoding = 3006;

I'm not sure we need that many encodings...

/cc @bhouston

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd default the encoding to RGBM7Encoding. At least it will render correctly up to an exposure factor. Defaulting the encoding to Linear is just wrong.

Copy link
Collaborator Author

@Mugen87 Mugen87 Jan 26, 2021

Choose a reason for hiding this comment

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

Although technically not correct, I've read that the resulting artifacts due to linear filtering with RGBM or RGBD can be considered as acceptable. This explains why you see the usage of linear filtering with both approaches.

But I understand that it depends on the texture data and the personal preference what kind of filtering is appropriate.

@elalish @MiiBond What is your recommendation? Should we keep the usage of linear filtering? Or default to nearest?

Copy link
Collaborator

@WestLangley WestLangley Jan 26, 2021

Choose a reason for hiding this comment

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

@Mugen87 I, too, think Linear filtering is acceptable, even if not correct. I am not sure about mip generation, though. I guess defaulting to false is OK.

Copy link
Owner

@mrdoob mrdoob Jan 26, 2021

Choose a reason for hiding this comment

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

So how do users other than @bhouston create RGBM files? Do they use a Blender add-on or something?

I would like to know that too...

Copy link
Contributor

Choose a reason for hiding this comment

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

Three.js has the ability to take a RGBE/HDR texture and encode it in RGBM just rendering from one texture to another using an encoding.

To make it easier one could make a Three.js example that could transcode between all these image formats pretty quickly. webgl_texture_encodings.html maybe? I seem to remember this suggested like 4 years ago.

Copy link
Collaborator Author

@Mugen87 Mugen87 Jan 28, 2021

Choose a reason for hiding this comment

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

I've hacked something together to test the RGBE to RGBM conversion for webgl_texture_encodings. The following fiddle loads a RGBE texture, converts it to RGBM and outputs the PNG on the website.

https://jsfiddle.net/94d5ymwj/

However, I've encountered an issue. It seems the encoding of the resulting RGBM texture is not correct. If I use the result in the RBGM example, it looks wrong, see https://jsfiddle.net/jg2so8mn/

I wonder what bit is missing 🤔 . Maybe a precision related issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Never mind! I've resolved the issue my manually encoding the PNG via UPNG.js and not using canvas. Updated fiddle:

https://jsfiddle.net/2do9arLf/1/

Not sure but it seems when applying the image data to the canvas, a color space conversion was applied on the texels.

@mrdoob mrdoob added this to the r125 milestone Jan 25, 2021
@vjevremovic
Copy link

@mrdoob Yes. We need both, as it determines precision people can use. We use RGBM7 as we have more need for darker areas to have enough data points, but RGBM16 is legitimate, too. I would suggest we keep both as they already exist.

@mrdoob
Copy link
Owner

mrdoob commented Jan 26, 2021

@vjevremovic Okay. What about LogLuv?

@mrdoob mrdoob merged commit 16eb3ec into mrdoob:dev Jan 26, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 26, 2021

Thanks!

@MiiBond
Copy link
Contributor

MiiBond commented Jan 26, 2021

@elalish @MiiBond What is your recommendation? Should we keep the usage of linear filtering? Or default to nearest?

Yeah, linear usually looks okay for RGBD and RGBM. But not RGBE or LogLuv.

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.

RGBM texture banding on iOS
6 participants