Broken PR: RangeFog and DensityFog by EliasHasle · Pull Request #17355 · mrdoob/three.js

EliasHasle · 2019-08-26T14:22:26Z

Update: #17592 replaces this PR.

Step 2 of the plan in #17264, adapted to comply with #17420 (comment).

Outdated

Also correct typescript docstring and misspelled name in test file for FogExp2, as well as docs for standard Fog.

Includes the change to distance-based depth, as well as corrections to better support mediump and avoid unnecessary squaring of a square root. Makes #17316 and #17324 obsolete.

Requires new build, update to the library build used in the editor, doc build, examples build.

I added an example named webgl_geometry_terrain_fog_types, which is basically webgl_geometry_terrain_fog without flight and with a GUI to try different fog setups. Everything seems to work.

Outdated link... Will be updated soon

Running version here: https://eliashasle.github.io/testing/fog_types/ (I think FogExp looks much more natural than the others.)

I have added additional versions of the new example, that test legacy signatures, mediump and lowp, respectively. They should probably be removed before merging.

I am currently struggling with git. Hoping to get everything in order soon.

Resolved

Some examples give an apparently unrelated warning message about using `pow` with negative input. I would really appreciate pointers into what goes wrong, as I am not using `pow` in my code or, as far as I can see, outputting anything that can be negative.

aardgoose · 2019-08-26T15:33:49Z

See #4834 for discussion about the pow() warnings, it's a known issue, and actually a DirectX warning from the Angle translation of shaders.

EliasHasle · 2019-08-26T16:40:49Z

@aardgoose Good to know it is a known problem, at least. Not introduced by me now.

This is (properly) ready for review.

EliasHasle · 2019-08-26T17:24:50Z

@gogoend This PR modifies existing docs and adds a new class FogExp, but adds no corresponding Chinese doc page. I linked to the English version in the list, as a fallback.

EliasHasle

The PR introduces FogExp in a straightforward, copy-paste, search-replace way.

The additional changes to documentation are needed. They may need a couple more critical eyes on them, though.

It felt natural to include the GLSL optimizations in the same PR. Comment #17316 (comment) by @WestLangley (and the following discussion) applies to this PR too, to the extent that it is a valid point:

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.

I replied that the docs do not seem to allow negative near plane, and that multiplying sign(fogPosition.z) into fogDepth may be a simple solution if negative near is to be supported. I suggest continuing the discussion here, since @supereggbert has expressed support for pursuing my PR as first choice.

docs/api/en/scenes/Fog.html

docs/list.js

editor/js/Sidebar.Scene.js

examples/files.js

EliasHasle · 2019-08-27T06:26:34Z

src/renderers/shaders/ShaderChunk/fog_fragment.glsl.js

-
-		float fogFactor = 1.0 - exp( - fogDensity * fogDensity * fogDepth * fogDepth );
+
+		vec3 scaledFogPosition = fogDensity * fogPosition;


This and the following lines avoid computing a precisionSafeLength just to square it (which would invariably fail in exactly the same cases where length would fail). Proposed in #17316 (comment)

src/scenes/Fog.d.ts

EliasHasle · 2019-08-27T06:29:34Z

test/unit/src/scenes/FogExp2.tests.js

 import { FogExp2 } from '../../../../src/scenes/FogExp2';

-export default QUnit.module( 'FoxExp2', () => {
+export default QUnit.module( 'FogExp2', () => {


This was a typo that I discovered when doing search&replace. It should be fixed whether or not the PR is merged.

EliasHasle · 2019-08-27T07:23:49Z

@titansoftime Would you like to test this? Considering how #14786 ended.

titansoftime · 2019-08-27T15:38:58Z

I can test once merged.

EliasHasle · 2019-08-31T16:12:47Z

@mrdoob Any chance of getting a review/merge soon? I try not to nag people too much, but FogExp was accepted by you as a good addition, so I went ahead with implementing it for common good. #17264 (comment) #17264 (comment)

Are any of the additional changes problematic? I think I have justified them well, but I am sure more perspectives may improve them further. @Mugen87 @WestLangley

EliasHasle · 2019-09-26T10:11:46Z

@titansoftime

I can test once merged.

Would you like to test the https://eliashasle.github.io/testing/fog_types/ demo on your mobile device before I start refactoring to the new API?

I guess part of the refactoring update will also be to make examples that explicitly set precision mediump and lowp for testing.

titansoftime · 2019-09-26T18:45:30Z

Would you like to test the https://eliashasle.github.io/testing/fog_types/ demo on your mobile device before I start refactoring to the new API?

Looks good. Though without an orbit cam, it's kinda difficult to be sure. Previously I would rotate the camera to reveal "fail areas" in the fog.

EliasHasle · 2019-09-26T22:13:12Z

Thanks!

Did you get to test all three fog types and different param values? (dat.gui isn't the most mobile-friendly) And are you certain that you were running in mediump?

without an orbit cam, it's kinda difficult to be sure.

I think I disabled it to avoid some event-handling conflicts.

Mugen87 · 2019-09-27T11:55:00Z

Um, the diff from the PR looks strange.

Can you please rebase it so it is based on the latest commit in dev?

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 · 2019-09-27T15:32:51Z

This gets worse and worse. Until a few minutes ago there was a monster diff, and now there is no diff at all, and the PR was "merged" and automatically closed. 😛

I will just have to go on...

EliasHasle · 2019-09-27T15:39:44Z

#17592 is a new try... I will clean it up a bit before I mark it as ready for review.

EliasHasle force-pushed the dev branch 2 times, most recently from 0e922a8 to 41c0bdb Compare August 26, 2019 16:37

This was referenced Aug 26, 2019

FOG_EXP2: Avoid possible over-/underflow when squaring on mediump #17324

Closed

Fix fog to use view distance #17316

Closed

Introduce FogExp and consider refactoring+FogLinear #17264

Closed

EliasHasle force-pushed the dev branch from 41c0bdb to f1b0911 Compare August 26, 2019 17:07

EliasHasle force-pushed the dev branch from f1b0911 to b480b79 Compare August 26, 2019 22:15

EliasHasle commented Aug 27, 2019

View reviewed changes

EliasHasle force-pushed the dev branch from bf1546b to 36b784e Compare August 27, 2019 12:59

mrdoob mentioned this pull request Sep 3, 2019

Scene: background, fog and shadow. #17420

Open

EliasHasle changed the title ~~Introduce FogExp with class, GLSL, docs and current applications~~ RangeFog and DensityFog Sep 27, 2019

EliasHasle closed this Sep 27, 2019

EliasHasle force-pushed the dev branch from dddb6dc to ab18028 Compare September 27, 2019 15:30

EliasHasle reopened this Sep 27, 2019

EliasHasle changed the title ~~RangeFog and DensityFog~~ Broken PR: RangeFog and DensityFog Sep 27, 2019

EliasHasle mentioned this pull request Sep 27, 2019

RangeFog and DensityFog #17592

Open

EliasHasle closed this Sep 27, 2019


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

		vec3 scaledFogPosition = fogDensity * fogPosition;

Uh oh!

Conversation

EliasHasle commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aardgoose commented Aug 26, 2019

Uh oh!

EliasHasle commented Aug 26, 2019

Uh oh!

EliasHasle commented Aug 26, 2019

Uh oh!

EliasHasle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EliasHasle Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EliasHasle Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EliasHasle commented Aug 27, 2019

Uh oh!

titansoftime commented Aug 27, 2019

Uh oh!

EliasHasle commented Aug 31, 2019

Uh oh!

EliasHasle commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

titansoftime commented Sep 26, 2019

Uh oh!

EliasHasle commented Sep 26, 2019

Uh oh!

Mugen87 commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EliasHasle commented Sep 27, 2019

Uh oh!

EliasHasle commented Sep 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

EliasHasle commented Aug 26, 2019 •

edited

Loading

EliasHasle left a comment •

edited

Loading

EliasHasle Aug 27, 2019 •

edited

Loading

EliasHasle Aug 27, 2019 •

edited

Loading

EliasHasle commented Sep 26, 2019 •

edited

Loading

Mugen87 commented Sep 27, 2019 •

edited

Loading