3Dickulus / FragM

Derived from https://github.com/Syntopia/Fragmentarium/
GNU General Public License v3.0
349 stars 30 forks source link

bright artifacts with ToneMapping=4 #135

Closed claudeha closed 4 years ago

claudeha commented 4 years ago

Describe the bug My frag displays bright white pixels in certain areas, only with ToneMapping=4.

Expected behavior No bright white pixels.

Suggested fix

in BufferShader.frag:

     colorHSV.x += Hue;
     vec3 c = hsv2rgb(vec3(colorHSV));   
     c=c/tex.a;
+    c = max(c, 0.0); // avoid bright white artifacts with ToneMapping=4

    if (ToneMapping==1) {
        // Linear

I suppose rounding errors in RGB -> HSV -> RGB made it go negative?

Screenshots broken, white regions are bad: broken fixed by adding the max line: fixed

Desktop (please complete the following information):

Additional context I could make a pull request, I guess that would now be to the FragM-Examples repository? Should I have filed this issue there instead?

3Dickulus commented 4 years ago

looking at those images suggests the fix just turns the white pixels black

I think it's a raytracer.frag problem not a buffershader.frag problem ? DE() or Color() functions?

edit: looks like a hairy beast ;)

3Dickulus commented 4 years ago

The Examples is kinda up in the air until I move to subtrees, have to think about it some more... wondering if it should be completely separate, maybe bring the basic Examples and tutorials back into the main repo and provide a script to install/update the additional user folders or figure out libgit2 and give FragM the ability to grab them and keep them updated along with being able to commit and push to the user's own repo.

3Dickulus commented 4 years ago

the original buffershader.frag? have you tried BufferShader-1.0.1.frag or BufferShader-1.0.2.frag?

3Dickulus commented 4 years ago

reviewing the code in question, I think the commented line should replace the following 4 HSV BS lines

//  vec3 c = tex.xyz/tex.a;
    vec3 colorHSV = rgb2hsv(tex.rgb);  //based on ased on VB_overflows answer on https://stackoverflow.com/questions/32080747/gpuimage-add-hue-color-adjustments-per-rgb-channel-adjust-reds-to-be-more-pink
    colorHSV.x += Hue;
    vec3 c = hsv2rgb(vec3(colorHSV));   
    c=c/tex.a;

first off, how can the red channel "be more pink", it's R, pink requires more than R second, converting rgb to hsv and back is more likely to introduce things that may not be helpful like bit rot from twiddling floats. third, if there is too much "pink" make adjustments in color selections... I could go on.

the BufferShader-1.0.n.frags don't have that code, reverted back to the original because it's better? not sure if that was the case, they have been around since the early days.

so rather than covering up the white pixels with black ones how about removing the offending code from BufferShader.frag

claudeha commented 4 years ago

On 02/05/2020 08:18, 3Dickulus wrote:

so rather than covering up the white pixels with black ones how about removing the offending code from BufferShader.frag

I should test that it is really the HSV/RGB stuff! Maybe the colours are already negative before that?  Thanks for the idea!

claudeha commented 4 years ago

Right, the HSV stuff is herring (get the white artifacts even without it), cause must be bad input (negetive? NaN?, 0-alpha?) colours coming from the back buffer... My guess is negative because the other tone mapping modes work fine...

3Dickulus commented 4 years ago

hm, ok, is turning them black a fix?, same issue, different color.

if failure is due to precision (NaN) loss not much can be done? if failure is due to exceeding the range 0.0 - 1.0 as in <0.0 || >1.0 can the input be normalized ? or the range scaled ?

3Dickulus commented 4 years ago

conditions that cause NaN would be c=0.0 or Exposure=0.0 ? c*=Exposure; or where c=-1 ? c = c/(1.+c);

would this be a fix or just covering it ? if c>1-epsilon c=1 if c<epsilon c=epsilon

should it figure out a new range or scale factor somehow ?

I note that the issue shows up with very few subframes... specular settings can cause bright spots on sharp edges and small features due to sampling radius, try specular settings @ 0 ?

I also note that with "Menger's Inferno" there were a few bright pixels showing up that seemed to be tied to background color, assumed they were very small holes in the object, adjusted background color and they blended in but still there.

3Dickulus commented 4 years ago

The fix is in DE-Raytracer.frag color() function

changed from this...

vec3 color(vec3 from, vec3 dir) {
    vec3 hit = vec3(0.0);
    vec3 hitNormal = vec3(0.0);
    depthFlag=true; // do depth on the first hit not on reflections
    vec3 first =  trace(from,dir,hit,hitNormal);

    if (Reflection==0. || hitNormal == vec3(0.0)) {
        return first;
    } else {
        vec3 d = reflect(dir, hitNormal);
        return mix(first,trace(hit+d*minDist,d,hit, hitNormal),Reflection);
    }
}

...to this...

vec3 color(vec3 from, vec3 dir) {
    vec3 hit = vec3(0.0);
    vec3 hitNormal = vec3(0.0);
    depthFlag=true; // do depth on the first hit not on reflections
    vec3 first =  trace(from,dir,hit,hitNormal);

    if (Reflection!=0. && hitNormal != vec3(0.0)) {
        vec3 d = reflect(dir, hitNormal);
        first = mix(first,trace(hit+d*minDist,d,hit, hitNormal),Reflection);
    }

    return clamp( vec3(0.0), first, vec3(1.0));
}

de-raytracer-bug de-raytracer-bug-fixed

claudeha commented 4 years ago

On 03/05/2020 07:38, 3Dickulus wrote:

|return clamp(vec3(0.0), first, vec3(1.0));| I think it's fine to have light > 1.0, that's how physics works, but negative is unphysical. So I would write

    return max(vec3(0.0), first);

But I'll keep investigating, the real fault is probably in my code somewhere and not the DE-Raytracer.frag...

3Dickulus commented 4 years ago

I think it's fine to have light > 1.0, that's how physics works, but negative is unphysical.

mathematically, physically, yes yes, however, this is limited by float precision and the confines and restrictions imposed by the hardware.

I think its the raytracer, have a look at DE-Kncr11.frag, way down at the bottom, almost last line

//Sometimes col<BigNegativeValue ->black dots. Why? I don't know :-/. Solved by Eiffie & Syntopia See: http://www.fractalforums.com/fragmentarium/updating-of-de-raytracer/msg81003/#msg81003 . I keep it just in case .

...just did a little test, returning different values from the color() function using default tracer....

Returned Value Color Yield
-2 white
-1 random black and white resolving to black with white spots
0 black
1 grey
2 grey++
... "
20 white (almost)
128 white (254,,,)
148 white (255,,,)

it seems the range for the color function is 0 > 148 ??? ( edit: at least under the default settings on that frag) yes, agree, return max(vec3(0.0), first);

No body has ever really maintained the frag collection, I'm sure many improvements are still stuck in the threads of FF.com that have been implemented in only a few personal collections, I'm not terribly good at math or GLSL (and a bit lazy) so I have generally left it up to users to dig in and figure it out (part of the fun I think)

3Dickulus commented 4 years ago

Having a repo for the Examples and with users maintaining their own folders should make it easier to propagate improvements while integrating them in the distribution.

You should make a repo for your frags ;) subfolders in user repos are fine too. I've setup a cron job that pulls user folders once a day so submodule HEADs are always pointing at the most recent commit on master branch of user repos ( but subtrees should fix that ) ..hm.. just wondering the effect of someone committing to their tree or making a series of changes to their subfolder structure while I'm in the middle of building a release or developing something on another branch?

claudeha commented 4 years ago

I get the white spots when I define providesNormal and use my own analytic (dual number derivatives) normal calculation - with the default numerical-delta normals they are not there... still investigating...

3Dickulus commented 4 years ago

resolved by https://github.com/3Dickulus/FragM/issues/135#issuecomment-623062878