Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Aug 25, 2020

This PR makes WebGLCubeRenderTarget use a CubeTexture.

I've realized this allows a nice refactoring in WebGLTextures. setTextureCubeDynamic() becomes obsolete.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 25, 2020

It seems when applying an instance of WebGLCubeRenderTarget (or its CubeTexture) to Scene.background, the background is now flipped (webgl_materials_cubemap_dynamic). Investigating...

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 25, 2020

It seems these checks are problematic:

uniforms.flipEnvMap.value = envMap.isCubeTexture ? - 1 : 1;

The engine should not set flipEnvMap to -1 for CubeTextures referring to a WebGLCubeRenderTarget.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 25, 2020

Ah, the removed comments from #18438 explain the need for further refactoring^^.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 25, 2020

@mrdoob @WestLangley Do you have already a preferred solution for this use case? Maybe a flag CubeTextures.needsFlip that controls this operation?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 27, 2020

Well, at least it seems the flag approach works^^.

@WestLangley WestLangley added this to the r121 milestone Aug 28, 2020
@WestLangley
Copy link
Collaborator

This is an excellent refactoring, IMHO. Everything seems to be working.

TBH, I am not a fan of ._needsFlipEnvMap, but I do not have a better suggestion.

CubeTexture extends Texture

	this._needsFlipEnvMap = true;

WebGLCubeRenderTarget extends WebGLRenderTarget

	this.texture = new CubeTexture()

	this.texture._needsFlipEnvMap = false;

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 8, 2020

@mrdoob What do you think about this PR? I would love to see setTextureCubeDynamic() and isWebGLCubeRenderTargetTexture go away 😇 . And using CubeTexture in WebGLCubeRenderTarget is good for #20111 (comment), too.

@mrdoob
Copy link
Owner

mrdoob commented Sep 23, 2020

Yes! Lets give this a try!

@mrdoob mrdoob merged commit 800d6b3 into mrdoob:dev Sep 23, 2020
@mrdoob
Copy link
Owner

mrdoob commented Sep 23, 2020

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Sep 23, 2020

TBH, I am not a fan of ._needsFlipEnvMap, but I do not have a better suggestion.

Would flipXZ be better? (Considering we already have flipY) 🤔

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 23, 2020

BTW: I would like to add a comment to the property that explains why it's there. I understand that it's necessary to flip the env map when just using CubeTexture and not cube render target. However, I'm not sure I fully understand why this detail could not be implemented elsewhere.

@mrdoob
Copy link
Owner

mrdoob commented Sep 23, 2020

I used to know, but I forgot...

@WestLangley
Copy link
Collaborator

Would flipXZ be better?

I do not think so. What is in the PR is better.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 8, 2020

I used to know, but I forgot...

Well, it seems the flipping happens since the early days^^.

946cc6a#diff-c0e88b98497597a015ecf238e91ac3a0R890

@WestLangley
Copy link
Collaborator

@WestLangey said:

By convention -- likely based on the RenderMan spec from the 1990's -- cube maps are specified by WebGL (and three.js) in a coordinate system in which positive-x is to the right when looking up the positive-z axis -- in other words, in a left-handed coordinate system. By continuing this convention, preexisting cube maps continued to render correctly.

three.js uses a right-handed coordinate system. So environment maps used in three.js appear to have px and nx swapped. (This is the case for every three.js cube map example.)

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 8, 2020

I hope you don't mind if I copy this into the code base for clarification 😇 .

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.

3 participants