KTX2Decoder: Use user options over defaults #17061
Conversation
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/17061/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/17061/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/17061/merge#BCU1XR#0 |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
| @@ -43,7 +43,7 @@ export class KTX2Decoder { | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/naming-convention | |||
| public async decode(data: Uint8Array, caps: KTX2.ICompressedFormatCapabilities, options?: KTX2.IKTX2DecoderOptions): Promise<KTX2.IDecodedData> { | |||
| const finalOptions = { ...options, ...KTX2Decoder.DefaultDecoderOptions }; | |||
| const finalOptions = { ...KTX2Decoder.DefaultDecoderOptions, ...options }; | |||
There was a problem hiding this comment.
Actually, ...options, ...KTX2Decoder.DefaultDecoderOptions was done on purpose.
The idea is that if a setting is defined in KTX2Decoder.DefaultDecoderOptions, it should apply no matter what. That's why KTX2Decoder.DefaultDecoderOptions is empty by default, which means that user-defined options will prevail, but if you define something in DefaultDecoderOptions, it will always take precedence (DefaultDecoderOptions is probably poorly worded, and we probably need a comment in the code!).
There was a problem hiding this comment.
Oh, interesting... didn't know this.
My motivation for this change was that even undefined values from KTX2Decoder.DefaultDecoderOptions would override anything set in options.
For example, if I wanted to forceRGBA for a particular texture, the undefined value from KTX2Decoder.DefaultDecoderOptions would take precedence, which I wouldn't expect.
Is this the intended behavior?
There was a problem hiding this comment.
From an API standpoint, this behavior seems like the opposite of everything else, but maybe that is because of the "default" name. But it seems weird to pass options in with a specific call to the function and those options not take precedence over more broadly scoped options.
There was a problem hiding this comment.
For example, if I wanted to forceRGBA for a particular texture, the undefined value from KTX2Decoder.DefaultDecoderOptions would take precedence, which I wouldn't expect.
No, it's not expected, but by default KTX2Decoder.DefaultDecoderOptions == {}, there's no undefined values. We could remove undefined values from DefaultDecoderOptions to be extra safe, though.
From an API standpoint, this behavior seems like the opposite of everything else, but maybe that is because of the "default" name. But it seems weird to pass options in with a specific call to the function and those options not take precedence over more broadly scoped options.
Yes, the problem is that the name should be something like ForcedDecoderOptions instead... It allows the user to easily apply some options when decoding KTX textures, something very difficult to do otherwise (these options are passed through the Texture constructor, something the user may not have access to, like when reading a glb file for example).
The comment in KhronosTextureContainer2.DefaultDecoderOptions explains it:
This reverts commit e04e218.
Take 2 of #17061. This function is what eventually sets KTX2Decoder.DefaultDecoderOptions, so let's stop this from happening: ```typescript const table = {...{ foo: true }, ...{}} // result: { foo: true } const table = {...{ foo: true }, ...{ foo: undefined }} // result: { foo: undefined } ```
No description provided.