add support for setting the encoding of the main map #3547
Conversation
| else { | ||
| texture.encoding = newEncoding; | ||
| } | ||
| } |
There was a problem hiding this comment.
Will need to use ES5 var and match the if/else style of the code above here.
|
+1 from me for providing a way of setting sRGB here. It should apply to Despite the three.js terminology, I would suggest naming this option |
| @@ -30,7 +30,8 @@ module.exports.Component = registerComponent('material', { | |||
| side: {default: 'front', oneOf: ['front', 'back', 'double']}, | |||
| transparent: {default: false}, | |||
| vertexColors: {type: 'string', default: 'none', oneOf: ['face', 'vertex']}, | |||
| visible: {default: true} | |||
| visible: {default: true}, | |||
| encoding: {type: 'string', oneOf: ['sRGB', 'Linear']} | |||
| if( data.encoding) { | ||
| let newEncoding = THREE[ data.encoding] || THREE[ data.encoding + "Encoding"]; | ||
| if(newEncoding === undefined) { | ||
| error("Unrecognized encoding: " + data.encoding); |
There was a problem hiding this comment.
Probably better a warning than an error.
There was a problem hiding this comment.
I would do. warning + fallback to existing encoding if it's not valid
| @@ -346,6 +346,16 @@ function setTextureProperties (texture, data) { | |||
| if (offset.x !== 0 || offset.y !== 0) { | |||
| texture.offset.set(offset.x, offset.y); | |||
| } | |||
|
|
|||
| if( data.encoding) { | |||
| let newEncoding = THREE[ data.encoding] || THREE[ data.encoding + "Encoding"]; | |||
There was a problem hiding this comment.
var encoding define at top of function
|
Thank you. @ncode congrats for your first PR. A few changes and this is ready to go. |
|
Hey @ncode, want to make a few changes? Thanks! The linter is also throwing some errors. |
|
@donmccurdy does this approach still look good? i don't have much experience with colorspace and encodings. |
|
This is still correct, but I think i'd prefer to do something at the scene level (like #3757) rather than putting a setting on every component in the scene. If we merge the other PR we should be able to close this. |
|
Thanks @ncoder, we're going to go with a scene-level solution. |
|
Sweet! Thanks, guys. |
Description:
Allow specifying the encoding of the main map on the material. This is important for correct linear/gamma settings.
Looks like:
<a-entity id="sky" geometry="primitive:sphere; radius:30; phiLength:360; phiStart:0; thetaLength:180" material="src:#skymap; encoding:sRGB" rotation="30 0 0">Changes proposed:
I don't think this change is merge able as-is. But I wanted to get this out there to see if there was interest in doing something similar, or if there are plans to solve this some other way.