Three.js: Circular dependency in WebGLCubeRenderTarget.fromEquirectangularTexture

Created on 15 Apr 2020  路  6Comments  路  Source: mrdoob/three.js

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.

Suggestion

Most helpful comment

If @mrdoob approves (and consider no other objections), I can make a PR with the change.

All 6 comments

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

Was this page helpful?
0 / 5 - 0 ratings

Related issues

ghost picture ghost  路  3Comments

danieljack picture danieljack  路  3Comments

boyravikumar picture boyravikumar  路  3Comments

donmccurdy picture donmccurdy  路  3Comments

Horray picture Horray  路  3Comments