Fix unexplained shader issue (glsl compiler bug??)

[Rogier-5]

For some reason, in the original code, the clamp() operations seem
to be ignored on my system (e.g. erroneously optimized away by the
glsl compiler ? driver bug ?)

The changed code, which should be 100% equivalent, does not suffer
from the problem.

It is not known if more than one person is affected, or if this issue
is already fixed in a more recent version of the software (driver,
glsl compiler ... ?)

See also the discussion in #4691

Screenshots without and with this patch:

Note: as so far I'm the only one experiencing this problem, I don't expect
this to be merged soon. However, this PR may be of use if and when other
people report this problem as well.

[Fixer-007]

On my ATI HD6870 it works the same as before, performance also good. 👍

[paramat]

1.0 / 0.6 can be simplified to 1.66667, i think 5 decimal places is enough, that's still accurate for a view range of 1000.

[Rogier-5]

@paramat: Changed to 1.7, which is probably just as good as 1.66667.

The code now reads: 1.7 - 1.7 * length(eyeVec) / fogDistance

Is that OK with you ?

IMO the exact value is not paramount. 1.0 / 0.6 makes the first fog start at 40% of the fog distance, 1.7 makes it start at 41%. Nobody would notice.

[paramat]

I would prefer to keep it fairly precise at 40% even if not visually noticeable.
How about 1.667?

[kwolekr]

paramat: Does it really matter?

[paramat]

It's no more complex so +1 once simplified.

[nerzhul]

maybe the problem is just 1 instead of 1.0 i think this can be the cause. Can you verify ?

[Rogier-5]

@Nerzul: I tried 1.0, but that does not make a difference.

Maybe that was caused by something else. Can't reproduce it now.

[Rogier-5]

The following code also works:
float d = clamp(0.01 + 1.7 * (1 - length(eyeVec) / fogDistance), 0.0, 1.0);
while the following code doesn't:
float d = clamp(0.00 + 1.7 * (1 - length(eyeVec) / fogDistance), 0.0, 1.0);

So apparently the last operation must be an addition or substraction (of a non-zero value), or else the clamp() seems to be ignored.

[lhofhansl]

Generally, do we want to support old/buggy glsl compilers? The new formula looks again like magic, and what if we want to do exponential fog?

It seems at some place we'd have to draw a line and say it's not supported.

[Rogier-5]

@lhofhansl: My intention was primarily to document this, and make a patch available in case other people also start to report the issue. As long as it is not known exactly which version(s) of which software suffer from the problem, it's hard to foretell whether that will happen.

If the new formula is too much like magic: the problem is also solved for example by adding 0.001 to your original formula:
float d = clamp(0.001 + (fogDistance - length(eyeVec)) / (fogDistance * 0.6), 0.0, 1.0);
Of course this addition of 0.001 needs a clarifying comment.
If this formula is preferred, I'll update the patch

[lhofhansl]

float d = clamp(0.001 + (fogDistance - length(eyeVec)) / (fogDistance * 0.6), 0.0, 1.0); with an explanation is super cool! Maybe 0.0001 is better (not sure about the actual scales)

What does 0.001 - 0.001 + ... do? :)

[Rogier-5]

not sure about the actual scales

?? How do you mean ??

A difference of 0.01 for instance would make a difference of 1 node at a viewing range of about 167. I doubt anyone would notice, much less complain :-)

[Rogier-5]

Pushed a new version with an updated formula.

@lhofhansl That doesn't work. I assume the compiler is smart enough to optimize that away - even though it (assuming it's the compiler's fault) is not smart enough to compute the formula correctly.

[lhofhansl]

👍

[paramat]

Would this be better since it's simpler, more precise and avoids a magic 0.001?
float d = clamp(1.667 - 1.667 * length(eyeVec) / fogDistance, 0.0, 1.0);

[lhofhansl]

@paramat I think it would just be magic numbers and a magic formula again. As long as the comment explain it doesn't matter, I guess.

[paramat]

I request float d = clamp(1.667 - 1.667 * length(eyeVec) / fogDistance, 0.0, 1.0); since it's equivalent, simpler and faster. +1 with that.

[Rogier-5]

@lhofhansl @paramat how about the following formula, which replaces the magic constants with a symbolic constant.

const float fogStart = 0.4;    // fraction of fogDistance at which the fog starts to appear
[...]
float clarity = clamp(1 / (1 - fogStart) + 1 / (1 - fogStart) * (fogDistance - length(eyeVec)) / fogDistance, 0.0, 1.0);

[paramat]

Nope because that's more complex and therefore slower.
My request has no unnecessary magic number at all.

[lhofhansl]

Any formula is good as long as we put the "canonical" formula (fogMax - distance)/(fogMax - fogMin) in a comment somewhere. 1.667 is magic, unless there's a comment about how we got to that.

[paramat]

Yes that comment is needed.

[Rogier-5]

@paramat, @lhofhansl Pushed a new version.

@paramat: I used your formula, and I tried to make it self-documenting, so I made the number a symbolic constant. OK ?

[Zeno-]

👍

[sofar]

is this relevant, perhaps?

https://www.opengl.org/discussion_boards/showthread.php/176167-ATI-GLSL-compiler-bug

[Zeno-]

@sofar it might be relevant yes. I was talking about these difficult to find "issues" the other day in a related PR

[nerzhul]

@paramat can you profile it to prove what you said ? Compiler will solve those calculs itself

[paramat]

Ok i was asking for too much simplification above.

+const float fogStart = 0.4;
+const float fogShadingParameter = 1 / ( 1 - fogStart);

+       float clarity = clamp(fogShadingParameter
+           + fogShadingParameter * (fogDistance - length(eyeVec)) / fogDistance, 0.0, 1.0);

I don't object to the const floats, but the form of the clarity equation can be simpler.
The original equation is
float d = clamp((fogDistance - length(eyeVec)) / (fogDistance * 0.6), 0.0, 1.0);

fogShadingParameter is 1.0 / 0.6.
So we can have this instead:

+const float fogStart = 0.4;
+const float fogShadingParameter = 1 / ( 1 - fogStart);

+       float clarity = clamp(fogShadingParameter - 
                    fogShadingParameter * length(eyeVec) / fogDistance, 0.0, 1.0);`

+1 for this. One less term and one less operator.
It's good practice to reduce to the simplest equation. Compared to this, the PR as it is currently is no closer to the standard form fog equation, and is no more readable.

[paramat]

Zeno this is not trivial and has only one approval, you almost impulsively merged against the rules again :) [EDIT i was wrong, see below].
As you can see your complaint was justified and i have changed my request, so waiting and discussing it instead of merging it worked out well.

[Zeno-]

Zeno this is not trivial and has only one approval, you almost impulsively merged against the rules again :)

I did not almost impulsively do anything at all! Do not attribute actions to me that never even existed, please.

<Zeno`> Rogier-5's change (using a constant) is the correct way
<Zeno`> and I disagree with paramat. Premature optimisation is the root of all evil
<Zeno`> Look, I'm the first person to push optimisations, I think you all know that
<Zeno`> but this is silly
<Zeno`> the bugfix is the more important of the two issues!
<Zeno`> by far
<Zeno`> there is no evidence that it even needs optimisation. I suggest it be merged
<nore> +1

You do NOT block a PR because you want someone to use a literal constant instead of a symbolic constant without a very good reason, and proof, to explain.

[paramat]

<Zeno> I'll merge in 15 minutes then if there are no objections
Oh but nore +1 just before, ok.
However i was in discussion and the author had not responded for a while, the discussion was ongoing, so it was a little irritating.

[Zeno-]

<Zeno> I'll merge in 15 minutes then if there are no objections`

Yes, because at that time there was two approvals. My approval, and nore's approval.

When sfan5 raised an objection I said I would not merge. How did I act inappropriately?

[paramat]

My mistake, see my edited comment above.

[Zeno-]

Please don't update previous comments in cases like this... it confuses things.

[paramat]

You do NOT block a PR because you want someone to use a literal constant instead of a symbolic constant without a very good reason, and proof, to explain.

Yes that's fair, i changed my request and gave reasons in my long comment above.

I didn't see nore's +1 in IRC chat at first.

[paramat]

Sorry i missed that.

[paramat]

Often i will edit a comment i have just posted, if someone then comments while i'm editing it can look like i edited after that someone's comment. That happened here once.

[Rogier-5]

@paramat: I implemented and tested your formula and it works the way it should.

I did some reading about efficient graphics processor programming, and I even think this patch is also a performance improvement, as additions are essentially zero-cost after performing a multiplication.

I pushed a new version.

[paramat]

Thanks 👍

[Zeno-]

Paramat, I do expect a proper apology by the way. I know it's petty, but I did nothing wrong. At all.

[paramat]

I did apologise above, but i do feel bad about not seeing that +1.

I think i felt a little annoyed because 2 devs (nore too, not just yourself) were pushing for a merge when the discussion was ongoing, when i had an objection and when the author had not responded for a while. I would have preferrred that someone commented and taken issue with my objection, as you can see in the end i realised i was being unreasonable and i changed my request.

Essentially if there's controversy and ongoing discussion it's often best (and advised in MT dev) to wait and not rush a merge.
It also seemed to be similar to yesterday, disagreement with my objection and a rush to merge without discussion. It became more annoying because of yesterday.
So i do think you (and nore) made a minor mistake too, but only very minor.

[lhofhansl]

As an outsider here, let me just note that everybody wants to do the right thing here and there was simply a communication problem. (I would have preferred the common range-fog formula be mentioned in the comment, but that's a nit). So let me thank all of you for keeping MT alive and kicking and being passionate about doing so.