DaloLorn / Rising-Stars

A large-scale mod for Star Ruler 2.
http://steamcommunity.com/sharedfiles/filedetails/?id=812827373
Other
18 stars 7 forks source link

Ringworlds crash my video card and the game even w/ "Safe Ringworlds" #66

Closed l29ah closed 3 years ago

l29ah commented 5 years ago

Linux l29ah-x201 4.19.0-rc6+ #107 SMP PREEMPT Wed Oct 3 19:13:19 MSK 2018 x86_64 Intel(R) Core(TM) i5-3320M CPU @ 2.60GHz GenuineIntel GNU/Linux

l29ah commented 5 years ago

Removing the "atmoMask == 0" branch "fixes" it.

werman commented 5 years ago

I have commented in Mesa's bugzilla but will duplicate it here:

Compiling the offending shader "ringworld_procedural_ps.txt" produces warnings about uninitialized variable "atmoSteps".

int atmoSteps;
if (advancedProcedurals){
    atmoSteps = 8 - int(atmoSteps * 4.0);
}
else if (simpleProcedurals){        
    atmoSteps = 6 - int(atmoSteps * 3.0);
}
else{
    atmoSteps = 4 - int(atmoSteps * 2.0);
}

As could be seen it is used without being initialized.

Later it is passed to cloudMaker function where is used for the loop as iterations count.

float cloudAnimated = 1.0 - cloudMaker(uv * vec2(256.0, 8.0), vec2(time * 0.5, 0.0), atmoSteps, cloudDistort);

float cloudMaker(vec2 n, vec2 f, int iterations, float w) {
    float total = 0.0, amplitude = 1.0;
    for (int i = 0; i < iterations; i++) {
        total += noise(n + w + f + total * 0.5) * amplitude;
        n += n;
        w *= 0.75;
        f *= 1.5;
        amplitude *= 0.4;
    }
    return total;
}

Accessing uninitialized variable is a valid operation but yields undefined result, "atmoSteps" became a huge value in some of invocations thus hanging the GPU.

GLSL 4.4 spec says:

Reading a variable before writing (or initializing) it is legal, however the value is undefined.

I don't know what value it should be initialized to but initializing it resolves the hang.

DaloLorn commented 5 years ago

Interesting... I wonder what Jon has to say about it...

JonMicheelsen commented 5 years ago

Nice find! Probably one of those, where my compiler was "clever" enough to figure out what was intended and compiled without as much as a warning. We had a few of those.

sol-oriens commented 5 years ago

I tested this on a rig that had the issue and it works indeed. I think I should add this fix to the Community Patch? (Initialized to 0, unless Jon says otherwise :))

werman commented 5 years ago

Just my thoughts: That calculation of atmoSteps had some strange intentions so 0 may not be a good value, I would advise to look at the other variant where atmoSteps is initialized to 1, pick the best one and simplify these confusing branches.

DaloLorn commented 5 years ago

Definitely - initializing to 0 results in multiplication by 0, which renders the whole operation moot.

DaloLorn commented 4 years ago

@JonMicheelsen So what do you think your compiler defaulted to? Looking at the file again, I actually can't see any value that would be an obvious fit...

DaloLorn commented 3 years ago

I actually fixed this in the MP: OpenSRProject/OpenStarRuler-Modpack@42399ba

Soooo closing now.