-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
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
Make equirectangular reflections / refractions backwards compatible. #19911
Conversation
…ngley/three.js into dev_equirect_refl_mapping
…onMapping removals.
Seems like many examples broke (background related). Will fix it tomorrow! |
@@ -25,7 +25,7 @@ function WebGLBackground( renderer, state, objects, premultipliedAlpha ) { | |||
|
|||
function render( renderList, scene, camera, forceClear ) { | |||
|
|||
let background = scene.isScene === true ? scene.background : null; | |||
let background = scene.isScene === true ? textures.getCubeTexture( scene.background ) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scene.background
can also be an instance of THREE.Color
. Besides, an instance of THREE.Texture
can represent and normal (flat) background texture. In both cases, getCubeTexture()
should not be called.
It seems this bit is responsible for breaking so many examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still an issue with this setup: https://jsfiddle.net/63cqshof/2/
Meaning you use a flat texture for the background (like a CSS background image). On dev_equirect_refl_mapping
nothing is rendered since getCubeTexture()
returns null.
src/renderers/webgl/WebGLTextures.js
Outdated
} else if ( envMap && envMap.isTexture ) { | ||
|
||
const mapping = envMap.mapping; | ||
const isDeprecatedEquirectangular = mapping === EquirectangularReflectionMapping || mapping === EquirectangularRefractionMapping; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe adding a warning when isDeprecatedEquirectangular
is true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
Just for the record: This PR would fix #9733. |
src/renderers/webgl/WebGLCubeMaps.js
Outdated
|
||
return texture; | ||
|
||
} else if ( texture.isEquirectangularTexture || isDeprecatedEquirectangular ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current PR, where are instance of EquirectangularTexture
actually created^^?
It seems right now the renderer only looks for EquirectangularReflectionMapping
and EquirectangularRefractionMapping
in order to determine an equirect texture.
If these constant are considered to be deprecated, how would the user define an equirectangular texture ( I thought this was the purpose of the respective loader)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current PR, where are instance of
EquirectangularTexture
actually created^^?
No example is using EquirectangularTexture
yet. I was waiting to get some consensus in #19845 before investing more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets leave the EquirectangularTexture
for another PR 😅
Two broken examples left. |
# Conflicts: # src/renderers/WebGLCubeRenderTarget.js
Beautiful! 😊 |
|
||
scene.add( mesh ); | ||
texture.minFilter = LinearFilter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned about this. The texture can be RGBE encoded, in which case linear filtering is inappropriate. See #19716 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I change it to this?
texture.minFilter = texture.encoding === RGBEEncoded ? NearestFilter : LinearFilter;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If linear filtering is appropriate for a given encoding, then the texture loader sets it as linear. There should be no reason to reset it -- at least for HDR/EXR.
PNG files can be RGBM16 encoded. It is the user's responsibility to set these properties in that case, since the loader won't.
Why are you using a filter different from the one the user specifies? Can you explain what you want to achieve here? Disabling mips?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what you want to achieve here? Disabling mips?
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not remove this code unless you can demonstrate a compelling reason to do so.
this.texture.generateMipmaps = texture.generateMipmaps;
this.texture.minFilter = texture.minFilter;
this.texture.magFilter = texture.magFilter;
Remember, not all equirectangular maps are JPGs.
If you want to create a special case to accommodate the default settings of TextureLoader
, that is up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to make a special case, that is up to you. I just can't support doing so since I do not have an explanation as to why mip maps are suddenly not a good thing.
I think it's the same reason we get a seam when mapping a equirectangular texture in a sphere geometry. When I first bumped into this issue I remember asking on Twitter about it and Alex Evans answered this:
So in the case of a Sphere, because the GPU is dealing with 2x2 blocks, it finds a block that goes from U 1.0 to 0.0 so it picks the smallest mip. That results in a seam when the U wraps.
We don't have seams in this case because the shader in fromEquirectangularTexture
does not use UVs.
However, when the fragment shader is drawing the poles we're seeing this issue again. In a equirectangular projection the higher or lower the V is the further apart the U samples are and... because of the 2x2 block approach the further apart in the texture the pixels are in the 2x2 block the smallest the mip is, so we end up with blurred poles.
By disabling mipmapping we force the GPU skip that step.
For more research on the subject, I think this is the best query:
https://www.google.com/search?q=graphics+cards+2x2+pixel+blocks
Do you think my conjecture makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. I just don't know at this point, unfortunately...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, not sure I understand.
Do you mean that you don't know if doing this is right?
texture.minFilter = texture.encoding === RGBEEncoded ? NearestFilter : LinearFilter;
Or that I was unsuccessful at explaining the issue and I confused you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would write it like this:
if ( texture.minFilter === LinearMipmapLinearFilter ) texture.minFilter = LinearFilter;
I feel uncomfortable about rendering with different settings than the user set, and I have not seen more than one texture that has this problem, so I would be reluctant to modify the library to accommodate it.
However, if you want a special case, then that is fine with me. After all, you may be right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would write it like this:
if ( texture.minFilter === LinearMipmapLinearFilter ) texture.minFilter = LinearFilter;
That sounds good to me! 👍
This looks good. ( edit: except for the changes you made to Unless I am missing something, I am not seeing support for assigning an equirectangular texture directly to |
It should work already: three.js/src/renderers/webgl/WebGLBackground.js Lines 30 to 34 in f25ccec
|
Yep, |
@Mugen87 Just to confirm, in scene.background = texture; // equirectangular
scene.environment = texture; |
I've just tried a HDR equirect texture and realized that ...should be something like: const image = texture.image;
if ( texture.isDataTexture === true || image.complete === true ) {
const renderTarget = new WebGLCubeRenderTarget( image.height / 2 );
renderTarget.fromEquirectangularTexture( renderer, texture );
cubemaps.set( texture, renderTarget );
return mapTextureMapping( renderTarget.texture, texture.mapping );
} |
@Mugen87 Done! Thanks! |
this.texture.generateMipmaps = texture.generateMipmaps;
this.texture.minFilter = texture.minFilter;
this.texture.magFilter = texture.magFilter; is also back. |
src/renderers/webgl/WebGLCubeMaps.js
Outdated
|
||
const image = texture.image; | ||
|
||
if ( texture.isDataTexture === true || image.complete === true ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On last question: Do we have an example where the code uses a compressed equirectangular texture (e.g. S3TC)? Right now, CompressedTexture
is not supported by WebGLCubeMaps.get()
. How about adding texture.isCompressedTexture === true
to the if
statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I guess that should be safe to add.
I'll add it. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up changing the check to just if ( image.height > 0 ) {
.
I double checked that dds textures work.
@WestLangley Squashed and merged. |
|
||
const image = texture.image; | ||
|
||
if ( image.height > 0 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That indeed produces a runtime error. I'll file a PR.
I know this was merged and closed a while ago so I don't really expect any action here, but just wanted to note that this change caused some issues with my project which I'm not sure how to best resolve. I updated Three.js versions for JanusXR a few months ago, and some users started noting that their rooms which used equirect video textures for environment maps had stopped working. I only just now got around to debugging that and traced it to this change. I guess in order to fix this I'm going to have to manually set up a Kind of hoping someone here might know of a more direct method for using equirect video envmaps which could save me from having to do the heavy lifting here... |
Do you mind elaborating? |
Sure. Each time In this particular case, I'm using video textures which could be rendering at 25, 30, or even 60fps, and I'll have to call A more optimized implementation would involve allocating those objects once, and then calling All of this is of course less optimal than being able to pass the |
Did you find a solution @jbaicoianu ? |
Not a cheap one, no. I ended up adding a wrapper around CubeCamera in my engine which then let's you place a reflection probe in your scene, and it does realtime reflections by using that as the envMap on specified objects, for example, here: Way more expensive than just sampling the equirect skybox, but at least it includes all objects that way |
Continued from #19845
This turned out to be much harder than I thought but ended up in a good place.
We no longer break user's code and, in the process, I updated
fromEquirectangularTexture()
to userenderer.renderBufferDirect()
and that way we can generate cubemaps within a render without messing with the state inrenderer.render()
This PR also adds
EquirectangularReflectionMapping
support toscene.background
.I'll leave this is a draft for now as I want to take a second look tomorrow.