Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exporting wrong colors #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

set-killer
Copy link
Contributor

@set-killer set-killer commented Sep 12, 2016

As you might know currently BCE exports wrong colors. Instead of real RGBA color it exports only RGB color with "precalculated" alpha channel. This is wrong. The reason for the current hacky implementation is that Godot does not support alpha channel for some of the colors. I assume this will be improved in the near future. Also the current implementation does not work vary well when you change the ambient light or any other light in the environment.

By definition the collada format requires 4 float values for some of the colors (page 375).

This patch fixes the alpha channel for the colors. Removes the function numarr because it is not used anywhere. Renames strarr to strfloat3 and numarr_alpha to strfloat4.

I've taken some samples:
samples

1 - The object rendered into blender
2 - The current situation with BCE
3 - Exported with this patch - Godot does not support alpha channel for the emission and specular colors (does only the diffuse color supports alpha?)
4 - Exported with this patch - changed the emission color to black

The reason for the difference between blender and godot is that blender applies gamma correction of 2.2 (whatever that means).

I guess it is not practical to merge this patch right now because the godot users should remove the emission color after they import a collada file. Nobody uses material colors anyway. I assume it is a known problem.
Also if you don't plan to merge it, it is not rude to close this PR.

@ghost
Copy link

ghost commented Sep 29, 2016

Technically your patch seems to be right - what was the original purpose of multiplying the color by mult, I wonder? Of course using the right colors and the alpha channel too is a better solution.

Godot does not support alpha channel for the emission and specular colors (does only the diffuse color supports alpha?)

Isn't this a Godot bug? Even if nobody uses material colors, it's Godot that isn't playing by the rules, right?

@set-killer
Copy link
Contributor Author

Yes, its a Godot problem. I hope that version 3 will fix the missing alpha channel for some of the colours.

To determine the problem more clearly try to add environmental light into the godot's scene. Then a totally different colour appears.

The reason for the current implementation is that originally Godot supports only RGB colours, and the RGBA support for the diffuse colour was introduced in version 2.0 (as far as I remember).

Another thing we need to discuss is: Should we apply that gamma correction when exporting the colours? In that way there wouldn't be any differences between that what you see in Blender and what you export.

@akien-mga
Copy link
Member

CC'ing @reduz who's working on the new renderer right now and would know what can and should be supported.

@ghost
Copy link

ghost commented Sep 29, 2016

Another thing we need to discuss is: Should we apply that gamma correction when exporting the colours? In that way there wouldn't be any differences between that what you see in Blender and what you export.

Turns out the gamma correction of 2.2 is actually standardized in sRGB. So, in short, it would not be wrong to apply gamma correction (so that Blender users won't accuse Godot of getting the colors wrong), but I'd put a checkbox to allow disabling it just in case some advanced user wants to.

@set-killer
Copy link
Contributor Author

Okey, i've added the Apply Gamma Correction to the exporting options.
So currently the result is the following:

gamma correction

1 - The object rendered in Blender
2 - The object imported in Godot with gamma correction
3 - The object imported in Godot with gamma correction and fixed specular and emission colors.
4 - The object imported in Godot without gamma correction and fixed specular and emission colors.

When we compare the hex colors from blender (which is gamma corrected already) and the hex color in Godot we still see difference:

  • #A5E7DB in Blender
  • #A3E6DA in Godot

I guess this difference comes from the rounding of the floats. Anyway, it is relatively small difference.

With the second patch I move the functions strfloat3 and strfloat4 inside the class so we can access the self.config variable. Adding new function apply_gamma which calculates the gamma correction.
The formula for the gamma correction is something like:

gamma = 2.2
for C in [R, G, B]:
  C = pow(C, 1/gamma)

The alpha channel does not receive gamma correction 😃

@ghost
Copy link

ghost commented Oct 1, 2016

@set-killer excellent work! Please explain one thing, when Godot users export the model from Blender using the default setting (gamma correction on) and load it straight into Godot do they see #2 or #3?

@set-killer
Copy link
Contributor Author

set-killer commented Oct 1, 2016

@paper-pauper Currently they see #3 from the first pic or #2 from the second image because the emission color in Godot does not support alpha blending. I hope the things will change with Godot 3.0

@ghost
Copy link

ghost commented Oct 1, 2016

I think your patch is good. There could be an improvement to match the colors with more precision, however as you noted it probably depends on the rounding (or some other precision issue) and I think nobody expects you to fix it (but if you do, props for your coding skillz 😎).

On the other hand, here we have a dilemma:

  1. This plugin is aimed at all Collada users, not just Godot users (the plan is to get it into upstream Blender), so it wouldn't be fair to adapt it to Godot's shortcomings
  2. If your PR is merged, people will complain about the plugin

In short, perhaps right now (until the issue is fixed) the best solution is to add yet another checkbox to "precalculate" the alpha channel (using the same formula as before), and leave it enabled for the time being. This way is fair to Godot users, to Blender users and to Collada users, I think. What do you think about it?

@set-killer
Copy link
Contributor Author

I think that would not be necessary.

Even if we submit this addon for merging into the mainline Blender now, i guess they will publish it with the version 2.80 which is not coming soon. This means that we have to distribute Godot 2.2 with BCE.

In this case we should merge this code and then submit it for merging into the Blenders repository. After that we should apply a patch into our repository which changes the algorithm in the strfloat4 function, and then we can distribute Godot 2.2 with BCE.

If nobody volunteers to submit this addon for merging into Blenders mainline code, we may ship Godot 2.2 without this patch.

(* by we i mean reduz, akien-mga and the other guys who are organizing the releases. not me)

@ghost
Copy link

ghost commented Oct 3, 2016

Now if we only had someone for #31...a whole month was wasted because nobody wants to do it. There are so many improvements which could benefit both BCE and Godot, such as this one.

This post says that Blender 2.8 will replace their Collada exporter with a Python one. Is it true? If so, that increases the chance they will merge BCE or at least use some code from it (unless their exporter is significantly better).

@set-killer
Copy link
Contributor Author

Oh, that's bad. It feels like our work is wasted...
We have missed the Blender's 2.78 milestone. Don't know if there are gonna be 2.78a and 2.78b releases, but it looks like our best chance is 2.79 (if happen at all) 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants