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

SSRPass: Correct Reflector depth. #21403

Merged
merged 6 commits into from
Mar 27, 2021

Conversation

gonnavis
Copy link
Contributor

@gonnavis gonnavis commented Mar 3, 2021

Previously, a hack was used to adjust maxDistance to let tDepth at the default camera distance has a relatively stable value. However, after changing the camera distance, the value of tDepth will also change.

Now the correct way to calculate tDepth is used, so that there is a stable value in any situation.
But at the trade off of turned off Oblique Near-Plane Clipping feature, because of it distort tDepth severely, and I feel need more long time to learn how to compensate it.
Any way, the Oblique Near-Plane Clipping is just used to fix the issue when something in bettwen virtualCamera and Reflector, for this demo scene and many groundReflectors, I think it's not a problem and correct tDepth is more important. I'll keep research how to compensate.

Before:
image

After:
image

@gonnavis gonnavis marked this pull request as draft March 3, 2021 19:31
@gonnavis gonnavis force-pushed the SSRPassCorrectReflectorDepth branch 2 times, most recently from 70f9e4c to 084c80c Compare March 4, 2021 18:14
@gonnavis gonnavis force-pushed the SSRPassCorrectReflectorDepth branch from 084c80c to c11f194 Compare March 4, 2021 18:31
@gonnavis gonnavis marked this pull request as ready for review March 4, 2021 19:21
scope.resolution = options.resolution || new Vector2( window.innerWidth, window.innerHeight );

scope._isDistanceAttenuation = ReflectorForSSRPass.ReflectorShader.defines.isDistanceAttenuation
Object.defineProperty(scope, 'isDistanceAttenuation', {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is isDistanceAttenuation used?

Copy link
Contributor Author

@gonnavis gonnavis Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If on, the reflection's opacity will has fade effect, otherwise all same opacity.
And because of I used it as defines, so don't want set it and update material in render loop, thus used defineProperty to watch it's state change.

isDistanceAttenuation on:
image
isDistanceAttenuation off:
image

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I think the property should be called distanceAttenuation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Shall I change all variables prefixed with is: isOtherMeshes, isGroundReflector, isPerspectiveCamera, isFresnel, isDistanceAttenuation, isOtherMeshes, isBouncing, isInfiniteThick, isBlur, isSelecctive to their none-is-prefixed version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at several examples with gui, found really no setting name prefixed with 'is', and feel need ensure consistency, so I renamed all words prefixed with is.
In addition, renamed all defines to uppercase.

@mrdoob mrdoob added this to the r127 milestone Mar 5, 2021
@gonnavis gonnavis force-pushed the SSRPassCorrectReflectorDepth branch 4 times, most recently from a867a86 to a3b9ea2 Compare March 15, 2021 04:50
Make `defines` all uppercase.
@gonnavis gonnavis force-pushed the SSRPassCorrectReflectorDepth branch from a3b9ea2 to c83b90f Compare March 22, 2021 17:50
@mrdoob
Copy link
Owner

mrdoob commented Mar 26, 2021

Sorry, seems like we introduced conflicts.
Do you mind resolving them?

@gonnavis
Copy link
Contributor Author

Resolved.

@mrdoob mrdoob merged commit 07e5802 into mrdoob:dev Mar 27, 2021
@mrdoob
Copy link
Owner

mrdoob commented Mar 27, 2021

Thanks!

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.

2 participants