Linear to sRGB conversion

[keianhzo]

Blender stores colors in linear color space internally. Hubs expect sRGB. We need to convert linear to sRGB when exporting.

Fixes #42

[Exairnous]

I haven't tested it yet (although it's probably fine), but my initial instinct is that it would be nice if that return statement could be broken up and clarified a little, rather than increasing what's on that line.

[keianhzo]

Related to: #42 (comment)

I just tried COLOR_GAMMA and it seems to work. So I think we need to account for both cases before converting to sRGB. I'm going to update.

[keianhzo]

I've updated to account for both COLOR and COLOR_GAMMA color types

[Exairnous]

Okay. Well, I tested out your first commit a bit and it seemed fine, your second one errors out ('str' object has no attribute 'startsWith'), also I'm not sure passing in a string indicating the subtype is a good idea (if you're going to do that you might as well change the subtype to COLOR_GAMMA, imo). However I'm a little worried that it's being applied to everything with a color property (such as lights, particles, fog, etc.), but there are no bug reports about them previously having the wrong color (I haven't detected anything wrong so far, so maybe it's fine, but it's weird). Also, perhaps there should be a migration so that the resulting Hubs color ends up the same (because people have probably compensated for this bug, although if it wasn't affecting the other stuff before that maybe doesn't need migration?)? And, minor thing, I think the comment needs updating.

[keianhzo]

Other than background color, color differences in things like light or fog can be really hard to appreciate. My guess is that not many people have realized so far and that's why we only have one report related to the background color.

ThreeJS Color type internally expect sRGB values unless you specify another color space, Hubs go with the defaults so my understanding is that we should be providing sRGB for every color.

I'll need to check the importer though to see how this affects the import process in case the color properties are COLOR or COLOR_GAMMA.

[keianhzo]

Maybe @rawnsley also wants to take a look at this

[rawnsley]

I can confirm it works for me in the simple case of background color as described in the original issue.

I'm not entirely sure what to expect from fog, but an empty scene with dark red fog and a pure white background looked all white. I would have expected it to be a bit pink at least? Probably unrelated as this also doesn't work with the original exported.

[rawnsley]

I forgot that fog needs depth buffer entries to work. When I placed a pure white cube in the scene and backed away from it to the "far" distance of the fog definition, the color was much closer to the defined fog color in the new system vs the old one. Remaining small differences may have been due to tone mapping, but I haven't bothered to go into further detail.

[Exairnous]

Okay, I've reviewed this more thoroughly and it's looking good. I think you're right that people just didn't notice the colors were off. What you have here codewise looks good to me, but I really think the colors should be migrated (i.e. we shouldn't keep the current values, but replace them with what was actually being exported, otherwise anyone who exports an old file will get quite different colors/lighting from what they had before); the good news is that Blender will do that for us if we switch all the subtypes from 'COLOR' to 'COLOR_GAMMA'.

I also agree about the importer, I think we'll need to detect the subtype of the property and convert or not based on that.
You can do that by hubs_component.bl_rna.properties['the property name'].subtype
Edit: but apparently you're already familiar with getting the subtype via bl_rna.properties, apologies.

[Exairnous]

Hm, thinking about this more, I think it would be better if the subtype was retrieved in the gather_color_property function itself via component.bl_rna.properties[property_name].subtype, rather than having it passed in (that way you don't have to remember to specify it in things like the Environment Settings gather).

[keianhzo]

Nice that just changing to COLOR_GAMMA works for migrating. I've updated all colors to that and also updated the tests, we were using just black or white before and that won't surface color breakage in the future for things like gamma.

[Exairnous]

Yup, it's always nice when Blender will just handle it :)
And good point about the tests, having the colors set to white has tripped me up when manually testing.