-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support RGBP encoding #4414
Support RGBP encoding #4414
Conversation
@@ -984,6 +984,13 @@ export const TEXTURETYPE_RGBM = 'rgbm'; | |||
*/ | |||
export const TEXTURETYPE_RGBE = 'rgbe'; | |||
|
|||
/** | |||
* Texture stores high dynamic range data in RGBP encoding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if some detail on the format is helpful here. Same goes for RGBM and RGBE I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, will add something to the engine in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super-cool. Any benchmark stats covering quality and size would be good though before merging, just to give us confidence this is genuinely superior.
I created an RGBP version of a particularly problematic lightmap originally taken from https://playcanvas.com/project/345310/overview/orange-room. The result running on iphone 7: And comparison of the two lightmaps:
|
Wow. I'd say that's a massive win! 😮 |
@@ -10,6 +10,7 @@ import { | |||
import { DebugGraphics } from './debug-graphics.js'; | |||
|
|||
const fixCubemapSeams = true; | |||
const RGBA8_TYPE = TEXTURETYPE_RGBM; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- local variables should not be all capital in
RGBA8_TYPE
- should this default to RGBP now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah this actually is a global constant .. ignore the first point :)
srgb: 'decodeGamma', | ||
rgbm: 'decodeRGBM', | ||
rgbe: 'decodeRGBE' | ||
'linear': 'decodeLinear', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these tables should use TEXTURETYPE_RGBP and similar constants, instead of stating the strings again?
for example:
TEXTURETYPE_RGBP: 'decodeRGBP'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this table is actually based on texture encodings not texture types, see https://github.com/playcanvas/engine/blob/main/src/graphics/texture.js#L652
The difference between texture encoding and type is subtle and I don't like it.
Texture encoding is based on texture type and pixel format:
- texture with type 'default' and RGB8 pixels has encoding 'srgb'
- texture with type 'default' and RGB16F pixels has encoding 'linear'
- texture with type 'rgbm/rgbe/rgbp' is always encoding 'rgbm/rgbe/rgbp' no matter the pixel format.
Ideally we'd have just one enum, since it's confusing have two concepts.
(I actually added 'encoding' to texture as a helper, but it's getting used more and more).
This looks really awesome, simple solution that solves the problem, and likely even faster to encode to! |
This PR adds support for a new HDR texture encoding (similar to RGBM).
Instead of storing pixel intensity in alpha, RGBP stores roughly
1.0 - max(1.0, pixel_intensity)
.This has some desirable properties, especially when stored as PNG:
For example, given this HDR image:
RGBM colour and alpha (3.6MB):
RGBP colour and alpha (3.1MB):