Change behaviour of setPackState when passed undefined options to avoid confusion

[jthistle]

Hi. This is a change I propose to the behaviour of setPackState.

I believe the current behaviour of setPackState is obscure and can lead to hard-to-debug issues. Here is an example (from the real world - I had been struggling with it for the past hour):

• There are two types of texture in my project: 2D textures called 'X', and 3D textures called 'Y'.
• Both types of textures are loaded asynchronously.
• In my project, all type X textures use the option flipY: 1, which leads to UNPACK_FLIP_Y_WEBGL being set to 1 in setPackState.
• Type Y textures are 3D, and so are not allowed to have the UNPACK_FLIP_Y_WEBGL flag set to 1. Because of this, in my project the flipY option is left empty when creating the 3D textures (under the incorrect assumption that it defaults to 0).
  • In Firefox, setting the flag to 1 is a soft error and the texture still loads (with weird undefined behaviour following). In Chrome, the texture will not be loaded at all.
• As a result of all of this, any type Y textures which load before the type X textures are fine, and any that load after are broken.
• Confusion entails...

For this reason, I think it would be more intuitive to have all of the options handled in setPackState default to 0 if left unspecified, rather than just retaining the previous value of whatever texture was last created.

As this could potentially break functionality in some applications, I think this change would require at least a bump of minor version number.

Please let me know your thoughts.

[greggman]

Thank you for the PR. This is definitely a bug.

I ended up fixing it by saving and restoring the state.

This is because, the way it was before is you can do this

gl.pixelStorei(gl.UNPACK_PREMULTIPLY_ALPHA_WEBGL, true);
gl.pixelStorei(gl.UNPACK_FLIP_Y_WEBGL, true);

// Load a bunch of textures flipped with premultiplied alpha
t1 = twgl.createTexture(gl, { src: 'https://p.com/some1.jpg' });
t2 = twgl.createTexture(gl, { src: 'https://p.com/some2.jpg' });
t3 = twgl.createTexture(gl, { src: 'https://p.com/some3.jpg' });
t4 = twgl.createTexture(gl, { src: 'https://p.com/some3.jpg' });

And I didn't want to break that behavior.

The new fix is a breaking change though because before it was

t1 = twgl.createTexture(gl, { src: 'https://p.com/some1.jpg', flipY: true });  // flipped
t2 = twgl.createTexture(gl, { src: 'https://p.com/some1.jpg' });  // also flipped

but now it's

t1 = twgl.createTexture(gl, { src: 'https://p.com/some1.jpg', flipY: true });  // flipped
t2 = twgl.createTexture(gl, { src: 'https://p.com/some1.jpg' });  // not flipped

[jthistle]

I see, makes a lot of sense. I didn't consider that pattern of usage. Your changes look like they should indeed fix the issue, thanks - I'll give them a test when I'm back at work and let you know if it improves things.

[jthistle]

BTW, funnily enough, I think the 5.x behaviour actually is the complete opposite of what the documentation explicitly says: https://twgljs.org/docs/module-twgl.html#.createTexture

[greggman]

I'm not sure that you're saying. The current version is 6.1

The behavior is tested here. Are you saying there's a bug? The docs seem correct for 6.1 unless I'm missing something

[jthistle]

Sorry yeah, was just saying that I think it was correct to say there was a bug in 5.x because the docs said the complete opposite of what the behaviour was. In 6.x, the docs now do match the behaviour.

edit: I'm being stupid, I didn't realise the online docs had been updated to 6.x, because I saw that the npm version was still at 5.5.4. Sorry. Ignore me.

[jthistle]

OK, I've tested with 6.1.0. Can confirm that the changes fix the issue. Thanks again.