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

RangeFog and DensityFog #17592

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

EliasHasle
Copy link
Contributor

@EliasHasle EliasHasle commented Sep 27, 2019

Replaces #17355.

RangeFog(color, near, far) and DensityFog(color, density, squared), as requested in #17420 (comment)

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

DensityFog: To make sure a change to squared causes the necessary material update, a needsUpdate is used internally and handled by the renderer. (Does needsUpdate also have to be declared in DensityFog.d.ts?)

The fog types example is live on my website.
Test of legacy signatures
Test of mediump
Test of lowp
It looks suspiciously good with mediump and lowp on my desktop, probably because the precision specifiers are minimal requirements. These ones must be tested on actual mediump/lowp devices.

The editor works too. It is not perfectly user-friendly to only have an unlabeled checkbox to represent squared, though. (Unlabeled controls seems to be a global problem in the editor).

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 27, 2019

But I think it will be ready for r109 after all.

No rush. I don't think merging this in that short amount if time is good. Better to target a later release. There are some other things that need to be fixed and tested for R109 already.

@EliasHasle EliasHasle force-pushed the FogExp_and_safe_distance_fog branch from 2f3b28f to 1bd6e9d Compare September 27, 2019 16:04
@EliasHasle

This comment was marked as resolved.

@EliasHasle EliasHasle force-pushed the FogExp_and_safe_distance_fog branch from 1bd6e9d to 39030ff Compare September 27, 2019 16:09
@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 27, 2019

I think it's better to stick to the old style for now. I'm not sure how fast we move on with this topic.

@EliasHasle EliasHasle force-pushed the FogExp_and_safe_distance_fog branch 2 times, most recently from 82dd31e to 5eddba2 Compare September 27, 2019 16:31
@EliasHasle EliasHasle marked this pull request as ready for review September 27, 2019 17:01
@EliasHasle EliasHasle force-pushed the FogExp_and_safe_distance_fog branch 2 times, most recently from 42e49f4 to 0dbf102 Compare September 27, 2019 22:48
@EliasHasle

This comment was marked as outdated.

@EliasHasle

This comment was marked as outdated.

@EliasHasle

This comment was marked as outdated.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 30, 2019

I normally don't do that if files in the core change. In this case, it's best to download your branch and test locally.

@EliasHasle

This comment was marked as resolved.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 3, 2020

Read the tooltip 😊

image

@EliasHasle

This comment was marked as outdated.

@EliasHasle

This comment was marked as outdated.

@Feirell
Copy link

Feirell commented Mar 3, 2021

Well this could fix things for me, I really hope this gets some traction again and not vanish in the deep dark.

@Mugen87 Mugen87 mentioned this pull request Mar 15, 2021
2 tasks
@LeviPesin
Copy link
Contributor

@EliasHasle Can you please update this PR?

@EliasHasle
Copy link
Contributor Author

EliasHasle commented Mar 24, 2023

@EliasHasle Can you please update this PR?

Done! I used a reset and a Yoda move (force push). The docs are not updated yet, though.

EliasHasle referenced this pull request Mar 27, 2023
…5498)

* Nodes: Rework ShaderNodeElements and make some other refactorings

* Fixes

* fix add assign

* Fix circular dependency and clean up

* Rename addNode -> addNodeElement and LightsNode.setReference -> addLightNode, add addNodeClass and addNodeMaterial

---------

Co-authored-by: sunag <sunagbrasil@gmail.com>
@EliasHasle EliasHasle force-pushed the FogExp_and_safe_distance_fog branch from 2a938de to 36bcc71 Compare March 27, 2023 13:39
@github-actions
Copy link

github-actions bot commented Mar 27, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize Gzipped Diff from dev
634.8 kB 157.4 kB +1.13 kB

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize Gzipped Diff from dev
425.6 kB 103.2 kB +645 B

@EliasHasle EliasHasle marked this pull request as draft March 27, 2023 14:19
@EliasHasle EliasHasle force-pushed the FogExp_and_safe_distance_fog branch from 18e57d1 to 1dd9f2a Compare March 28, 2023 12:48
src/Three.Legacy.js Outdated Show resolved Hide resolved
@EliasHasle
Copy link
Contributor Author

EliasHasle commented Mar 31, 2023

The rationale for using only depth rather than Euclidean distance from a single point for fog with orthographic projection is that every pixel on the screen represents a parallel "eye beam", whereas with perspective projection, the camera position represents the focal point, ideally located at the position of the observer in front of the screen, where the screen is the projection plane (or "window" to the virtual world). (Nevermind that most demos and games inflate the FOV way beyond the typical eye-screen angle, for practical gameplay/immersion purposes.)

@EliasHasle EliasHasle marked this pull request as ready for review March 31, 2023 08:44
@EliasHasle EliasHasle force-pushed the FogExp_and_safe_distance_fog branch from 8eb6655 to 042568b Compare March 31, 2023 08:45
Neither builds, docs, manual nor the FogExp2Node are updated in this commit. Work in progress.

I think one should consider making `vViewPosition` a dependency for all shaders that use fog, instead of adding the new `vFogPosition`.

Fixed dirtiness logic in WebGLRenderer and the automatically detected errors

Also improved the example and added a thumbnail for it.

Update src/Three.Legacy.js

Shorter deprecation warning for `FogExp2`.

Co-authored-by: Levi Pesin <35454228+LeviPesin@users.noreply.github.com>

Fix JSON logic
@EliasHasle EliasHasle force-pushed the FogExp_and_safe_distance_fog branch from 042568b to 57eeb7f Compare April 12, 2023 09:52
None of the `exp`s can return >1 or <0 given the new, necessarily non-positive, arguments.
@EliasHasle EliasHasle force-pushed the FogExp_and_safe_distance_fog branch from 57eeb7f to 4bea924 Compare April 12, 2023 11:27
@EliasHasle
Copy link
Contributor Author

Well this could fix things for me, I really hope this gets some traction again and not vanish in the deep dark.

@Feirell Do you mind testing the newest version?

This PR should be ready for review now. @mrdoob @Mugen87 @LeviPesin

Given the parameterisation with an optional squared, I would prefer to have it false by default, because FOG_EXP retains at least a rudimentary physical realism that FOG_EXP2 sacrifices. (It could also simplify some logic for JSON etc.) But for now, I have left it true by default, as requested.

@Feirell
Copy link

Feirell commented Apr 20, 2023

@EliasHasle sorry to disappoint but the usecase I had has long moved on. I switched to a different approach entirely. Depending on if you believe that another tester is useful, I could cobble together a quick scene and test if it works as I would expect it to.

Non the less, thank you for your work and effort!

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.

4 participants