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

Fix fog to use view distance #17316

Closed
wants to merge 1 commit into from
Closed

Conversation

supereggbert
Copy link
Contributor

@mrdoob
Copy link
Owner

mrdoob commented Aug 22, 2019

@greggman did you try this?

@WestLangley
Copy link
Collaborator

What happened to #14633?

@EliasHasle
Copy link
Contributor

EliasHasle commented Aug 22, 2019

What happened to #14633?

These two PRs are identical, so they will face exactly the same problems (unless other changes are relevant). I found this explanation for what happened to the previous one: 63f6d20

@WestLangley
Copy link
Collaborator

Oh, now I remember suggesting #14740 (comment). Something similar was subsequently tried, but I have lost track as to why even that did not work...

@EliasHasle
Copy link
Contributor

It was apparently reverted because @titansoftime showed a screenshot of fog that did not work with the update, but without providing a way to verify that the root cause of the problem was fog. #14786 (comment)

Notably, the problem was with medium precision and FogExp2, which may be a bad combination, since FogExp2 squares the depth. ("My" proposed FogExp would perhaps be a better match for medium precision, although it has different characteristics.)

@supereggbert
Copy link
Contributor Author

Eekk! Looks like this has a lot of history. To prevent any issues with lowerp, and at least get it usable where it does work, I've limited using view distance method to highp. So, only highp should see this change, and it looks to be working across the board with the limitation in place. Once @EliasHasle implements the new FogExp method it can be revisited.

@WestLangley
Copy link
Collaborator

I'd suggest something like #14752 (comment).

Until someone can demonstrate that it does not work. Claiming is not demonstrating.

@titansoftime
Copy link
Contributor

If y'all bug out fog again, I will be sure to provide a jsfiddle demonstrating the issue in addition to screenshots.

@supereggbert
Copy link
Contributor Author

@WestLangley I entirely agree that's where things should be heading, but I fear history will simply repeat itself. At least if there is somewhere to revert back to, it doesn't mean losing distance fog entirely again.

@WestLangley
Copy link
Collaborator

@supereggbert Personally, I am not a fan of your proposed implementation which has completely different rendering results based on precision / device.

#ifdef HIGHP_PRECISION
	float fogDepth = length( fogPosition );
#else
	float fogDepth = - fogPosition.z;
#endif

I think that will surprise devs.

@supereggbert
Copy link
Contributor Author

supereggbert commented Aug 22, 2019

@WestLangley I agree it's far from optimal, but the result is far from completely different. Considering the issues are arising from lowerp, mixed with the fact the actually difference is very subtle, it's only really evident with a wide viewing angle on desktop/xr (see screenshots). I'd be willing to bet the precision issues will, more or less, be completely wipe out any difference on lowerp anyway. I'm sure, 99.99% of devs will not notice it's absence on mobile but it can be annoyingly evident on desktop/xr with camera rotation. Plus, it's hopefully only a stopgap/placeholder until a more global solution is found to the length function precision problems, which is probably better suited to it's own PR, as it potentially covers more then just fog.
mobile mediump
desktop highp

@EliasHasle
Copy link
Contributor

EliasHasle commented Aug 22, 2019

I agree with @WestLangley .

@titansoftime Is there a chance that your FogExp2 problems on mediump could be caused by the order of multiplication in the shader?

float fogFactor = 1.0 - exp( - fogDensity * fogDensity * fogDepth * fogDepth );

Wouldn't this be safer against mediump overflow, considering that fogDensity is usually a really small number and fogDepth is typically a large number in the problematic cases?

		float fogFactor = 1.0 - exp( - (fogDensity * fogDepth) * (fogDensity * fogDepth) );

@titansoftime
Copy link
Contributor

@titansoftime Is there a chance that your FogExp2 problems on mediump could be caused by the order of multiplication in the shader?

I'm not sure. That PR was reverted a while ago and re-implementing an old version of three.js to test in my app is not something that I want to spend time on.

@EliasHasle
Copy link
Contributor

EliasHasle commented Aug 23, 2019

In C, multiplication is left-associative: https://stackoverflow.com/a/31630985/4071801

I will have to assume it works the same in GLSL, since they are so similar.

For this case, that means the current code will first square the density, then multiply with depth two times. If it were the other way around, I would expect to see overflow on the exponent bits if the precision were low. But I suppose there is equally a potential for "underflow" on the exponent when squaring the density too, so I will submit a PR: #17324

I hope this can pave the way for a new attempt at introducing @bhouston's safePrecisionLength and enable working distance-based fog.

@titansoftime
Copy link
Contributor

It was apparently reverted because @titansoftime showed a screenshot of fog that did not work with the update, but without providing a way to verify that the root cause of the problem was fog. #14786 (comment)

Ya know, I actually did make a fiddle back then =P

#14736

@EliasHasle
Copy link
Contributor

Ya know, I actually did make a fiddle back then =P

But not in the second report, after the fix, right?

I didn't want to be harsh against you, anyway. I understand that stuff like this cannot always be a priority. :-) Pinged you to get your (current) perspective/explanation.

@titansoftime
Copy link
Contributor

But not in the second report, after the fix, right?

Incorrect. #14786

No offense taken =]

@EliasHasle
Copy link
Contributor

EliasHasle commented Aug 25, 2019

FOG_EXP2 could avoid squaring a square root with a few modifications. Something like:

vec3 scaledFogPos = fogDensity*fogPosition; //reducing potential for overflow/underflow problems
float fogFactor = 1.0 - exp( -dot( scaledFogPos, scaledFogPos ) );

This would replace the need for safePrecisionLength for FOG_EXP2, but safePrecisionLength will still be needed for FOG_EXP and linear fog. It will also make my PR #17324 obsolete. I don't know whether it is faster in practice, though. But in theory one avoids an expensive square root and makes a simpler code to optimize than the safePrecisionLength path for mediump.

@supereggbert Will you do this change?

(dot( scaledFogPos, scaledFogPos ) could be wrapped in a common lengthSq function, if that is interesting.)

EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Aug 26, 2019
Also correct TS docstring and misspelled name in test file for FogExp2. Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Aug 26, 2019
Also correct TS docstring and misspelled name in test file for FogExp2. Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.
@WestLangley
Copy link
Collaborator

Be aware that this PR, as currently-written, is going to significantly change fog behavior for users who use linear fog with and an OrthographicCamera and a negative value for near plane.

I will not be surprised if there are complaints.

EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Aug 26, 2019
Also correct TS docstring and misspelled name in test file for FogExp2, and doc for standard Fog. Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Aug 26, 2019
Also correct TS docstring and misspelled name in test file for FogExp2, and doc for standard Fog. Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.
@EliasHasle
Copy link
Contributor

EliasHasle commented Aug 26, 2019

I have incorporated the functionality of this PR as part of the larger PR #17355. It was the most practical way to do it, since I both added FogExp support and modified the GLSL for FogExp2. @supereggbert Will you support my PR instead? :)

Be aware that this PR, as currently-written, is going to significantly change fog behavior for users who use linear fog with and an OrthographicCamera and a negative value for near plane.

@WestLangley The last time I checked, OrthographicCamera did not support near<0. The docs are... I would say clear, but they may be interpreted such that far can be negative and near can be less negative. I have never tried that.

@WestLangley
Copy link
Collaborator

Be aware that this PR, as currently-written, is going to significantly change fog behavior for users who use linear fog with and an OrthographicCamera and a negative value for near plane.

I will not be surprised if there are complaints.

In my experience, It's quite common. @Mugen87 can decide if we want to break this. I am just raising the issue so everyone is aware of it.

Modify the misc_controls_orbit.html example like so:

scene.fog = new THREE.Fog( 0xcccccc, 100, 1000 );

var h = 120, aspect = window.innerWidth / window.innerHeight;
camera = new THREE.OrthographicCamera( - h * aspect, h * aspect, h, - h, - 1000, 1000 );
camera.position.set( 4, 1, - 4 );

@EliasHasle
Copy link
Contributor

EliasHasle commented Aug 26, 2019

Again, that is an undocumented feature (my emphasis):

.far : Float
Camera frustum far plane. Default is 2000.
The valid range is between the current value of the near plane and infinity.

Presumably positive infinity.

.near : Float
Camera frustum near plane. Default is 0.1.

The valid range is between 0 and the current value of the far plane. Note that, unlike for the PerspectiveCamera, 0 is a valid value for an OrthographicCamera's near plane.

I don't argue that placing near and far freely can never be useful, though. (However, the same view-projection can be achieved in other ways, such as modifying the camera position.)

@WestLangley How about multiplying in sign(fogPosition.z) in fogDepth? Would that be better?

EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Aug 27, 2019
Also correct TS docstring and misspelled name in test file for FogExp2, and doc for standard Fog. Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.
@supereggbert
Copy link
Contributor Author

@EliasHasle That's fine with me.

EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Sep 27, 2019
Also correct TS docstring and misspelled name in test file for FogExp2, and doc for standard Fog. Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Sep 27, 2019
author Elias Hasle <elias.hasle@gmail.com> 1566822048 +0200
committer Elias Hasle <elias.hasle@gmail.com> 1569597690 +0200

RangeFog(color, near, far) and DensityFog(color, density, squared)

Added fog types example.
Currently includes some extra examples that show functionality of legacy signatures, and mediump and lowp operation for all fog types.

Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.

Force update when squared is changed.

Fix legacy example and mediump example

Attempted fixes for editor
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Sep 27, 2019
Added fog types example.
Currently includes some extra examples that show functionality of legacy signatures, and mediump and lowp operation for all fog types.

Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.

Force update when squared is changed.

Fix legacy example and mediump example

Attempted fixes for editor

Formatting + typos
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Sep 27, 2019
Added fog types example.
Currently includes some extra examples that show functionality of legacy signatures, and mediump and lowp operation for all fog types.

Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.

Force update when squared is changed.

Fix legacy example and mediump example

Attempted fixes for editor

Formatting + typos
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Sep 27, 2019
Added fog types example.
Currently includes some extra examples that show functionality of legacy signatures, and mediump and lowp operation for all fog types.

Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.

Force update when squared is changed.

Fix legacy example and mediump example

Attempted fixes for editor

Formatting + typos
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Sep 27, 2019
Added fog types example.
Currently includes some extra examples that show functionality of legacy signatures, and mediump and lowp operation for all fog types.

Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.

Force update when squared is changed.

Fix legacy example and mediump example

Attempted fixes for editor

Formatting + typos

Fixed TS declarations (Not sure I am using IFog right)
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Sep 27, 2019
Added fog types example.
Currently includes some extra examples that show functionality of legacy signatures, and mediump and lowp operation for all fog types.

Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.

Force update when squared is changed.

Fix legacy example and mediump example

Attempted fixes for editor

Formatting + typos

Fixed TS declarations (Not sure I am using IFog right)

Fixed support for scene.fog=null
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Sep 27, 2019
Added fog types example.

Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.

Force update when squared is changed.

Fixed support for scene.fog=null

Fixed TS declarations

New best guess is `fog: IFog | null`
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Sep 27, 2019
Added fog types example.

Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.

Force update when squared is changed.

Fixed support for scene.fog=null

Fixed TS declarations

New best guess is `fog: IFog | null`
EliasHasle added a commit to EliasHasle/three.js that referenced this pull request Jan 3, 2020
Added fog types example.

Includes the change to distance-based depth, as well as corrections to better support mediump. Makes mrdoob#17316 and mrdoob#17324 obsolete.

Force update when squared is changed.

Fixed support for scene.fog=null

Fixed TS declarations

New best guess is `fog: IFog | null`
@EliasHasle
Copy link
Contributor

I see you closed this. #17592 is the up-to-date PR that incorporates the same idea, corrects some numerical issues, improves the OO logic and adds support for truly exponential fog (without squared exponent). It seems to be indefinitely stuck in the fog, though. 😕

@mrdoob
Copy link
Owner

mrdoob commented Dec 8, 2020

@EliasHasle do you mind writing a "/bump" comment in that PR so it goes back up on my list?

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

Successfully merging this pull request may close these issues.

None yet

5 participants