Support WebP in environmentTextureTools #11102
Conversation
|
@simonihmig This is huge !!! I ll review in a couple hours thanks a lot !!! |
| @@ -1,328 +1,329 @@ | |||
| # 5.0.0 | |||
There was a problem hiding this comment.
Can you check what is wrong with this file? We cannot compare them :(
There was a problem hiding this comment.
This is weird. The diff in my IDE looked all good. Will check...
There was a problem hiding this comment.
Seems my IDE changed line endings to CRLF, no idea why, which made every line change. Fixed this now!
src/Misc/environmentTextureTools.ts
Outdated
| @@ -218,6 +249,7 @@ export class EnvironmentTextureTools { | |||
| let info: EnvironmentTextureInfo = { | |||
| version: 1, | |||
There was a problem hiding this comment.
Should we bump the version ? like 1.1 as it is not a breaking change ?
There was a problem hiding this comment.
Yeah, my thinking was we can keep it as is, as nothing has been removed from the JSON data. But after thinking about this again, I am beginning to think that actually it's not only about the json, but also about what kind of data is in these array buffers.
Like when there is an .env files created with the changes here, using WebP, any application that still uses old BJS code without these changes will break reading it, as it will wrongly assume the array buffers contains png data, which they don't. And this will probably break in unexpected ways, basically with corrupted image data, instead of giving a meaningful error message.
So maybe we should bump that indeed to 2! What do you guys think?
There was a problem hiding this comment.
Yup sounds like a good plan to me :-)
There was a problem hiding this comment.
Did that now (bump version to 2)!
Changed the types a bit, EnvironmentTextureInfo is now a discriminated union of the now separate v1 and v2 interfaces. So by checking the version, TS will narrow the type to the appropriate version.
Added a normalize method, which takes any version and always returns the latest, so we don't have to care about differences between these versions in other parts of the codebase. This also removed the need for this Required<> type, as we now always return a EnvironmentTextureInfoV2, which is guaranteed to have imageType set. /cc @deltakosh
Tested again locally, so I think this should be fine. But please re-review the latest changes! :)
simonihmig
force-pushed
the
env-webp
branch
from
a76e2fe
to
f560f99
Compare
| if (info.version !== 1) { | ||
| Logger.Warn('Unsupported babylon environment map version "' + info.version + '"'); | ||
| } | ||
| info = EnvironmentTextureTools.normalizeEnvInfo(info); |
There was a problem hiding this comment.
Note that this could be seen as some kind of breaking change, as an unsupported version will now throw, while previously it only issued a warning. But I believe this was wrong, as the schema could have been totally different. At least it was inconsistent with the other places where an error was already thrown...
|
Thanks a ton !!! I ll merge it right after the build !!! |
|
@simonihmig were you able to validate we are not using values when alpha is high ? just to be sure :-) |
Not sure I understand what you mean here? What exactly (and how) should I validate? |
|
Basically I am wondering if for compression sake they are premultiplying alpha with the rest of the channels so when we would have high intensity in some places, we could lost the color info. like white + alpha of 0.1 would result in webp of 0.1, 0.1, 0.1, 0.1 This happens in IE with pngs for instance so I bet it is not the case but it is more of a sanity check |
|
I regularly contribute to other OSS projects for quite a while, but having a PR reviewed, re-reviewed and merged(!) by three maintainers within literally just a few hours, was a quite unique and enjoyable experience! Thanks folks! 😍 |
This adds support for encoding and decoding WebP images in .env files. See https://forum.babylonjs.com/t/support-webp-for-env-maps/23644
The default type remains PNG, so the changes should be fully backwards compatible. I tested this by locally changing the arguments to CreateEnvTextureAsync in the Inspector, exporting some sample environments with different settings, and dropping them again into the sandbox. Everything seems to work fine. Not sure if there is some additional test coverage we can add here?
The second commit adds an optional
qualityargument to a bunch of Blob-related tools, so that it can be passed finally tocanvas.toBlob(), which supports this argument for WebP images (at least in Chrome).The results are quite satisfying. The playground's original
environment.ddsgave these results when converted to .env:You can find these sample files here
I didn't update the Inspector to allow exporting .env files using WebP yet, but can do this in a separate PR when this is merged!