The last circular dependency in the library is:
WebGLCubeRenderTarget -> CubeCamera -> WebGLCubeRenderTarget
This is caused by WebGLCubeRenderTarget.fromEquirectangularTexture
, which was added in #16671 following discussion in https://github.com/mrdoob/three.js/pull/15331#issuecomment-442908452.
This is the last blocker for closing #6241.
To remove this, we could extract .fromEquirectangularTexture
to an external utility (or maybe move it to CubeTexture
?). I'm not sure if it's worth doing so just to avoid a circular dependency though.
@WestLangley, @Mugen87 and @mrdoob - you guys were involved in adding this method. Do you think we need to fix this circular dependency? If not, then we can probably just mark as "won't fix" and close #6241.
Do you think we need to fix this circular dependency?
As far as I understand, this circular dependency is not harmful. However, circular dependency is always an indicator that something with the class design is _not right_. This issue can only be solved by refactoring.
I've already though about this in the past and my preferred option is to inject the internal render target of CubeCamera
to solve this issue. So instead of:
var camera = new THREE.CubeCamera( 1, 1000, 256, { format: THREE.RGBAFormat, magFilter: THREE.LinearFilter, minFilter: THREE.LinearFilter } );
we do this:
var renderTarget = new THREE.WebGLCubeRenderTarget( 256, { format: THREE.RGBAFormat, magFilter: THREE.LinearFilter, minFilter: THREE.LinearFilter } );
var camera = new THREE.CubeCamera( 1, 1000, renderTarget );
I like latter one since the last two parameters of CubeCamera
are obviously intended for WebGLCubeRenderTarget
. And since users usually use cubeCamera.renderTarget
to access the internal render target, they could instead refer to the variable created on app level (so just renderTarget
).
Yeah, that looks better to me. Passing in parameters intended for the render target to the CubeCamera
constructor doesn't feel right.
If @mrdoob approves (and consider no other objections), I can make a PR with the change.
I've already made the PR so it's easier to see the changes.
@Mugen87 Thank you! I'll review this week.
@mrdoob /bump
Most helpful comment
If @mrdoob approves (and consider no other objections), I can make a PR with the change.