CSS2D/3DRenderer: Ensure Object3D.visible is correctly evaluated in object hierarchies.

[lafflan]

…stors are visible

Related issue: #XXXX

Description

A clear and concise description of what the problem was and how this pull request solves it.

This contribution is funded by Example

[Mugen87]

For reproduction: https://jsfiddle.net/nhg8durq/

[Mugen87]

Um, I'm not sure why the CSS renderers behave differently compared to WebGLRenderer. Normally, when Object3D.visible is set to false neither the object nor its descendant nodes should be rendered.

It's also better to fix this issue in renderObject(). Maybe just adding the following line at the top of the method in CSS3DRenderer:

if ( object.visible === false ) return;

For CSS2DRenderer it should be:

if ( object.visible === false || ( _vector.z < - 1 || _vector.z > 1 )) return;

The local visible property in renderObject() only depends from the layers tests like in WebGLRenderer.

@yomotsu Do you know why the CSS renderers handle children of invisible objects differently than WebGLRenderer?

[yomotsu]

Actually, I'm not entirely sure, and it seems that the potential problem might have gone unnoticed.
The issue is attributed to the following reasons:

• Setting visible = false will set display: none on the CSS3DObject internally, and its DOM children will not be rendered either.
  see:

  three.js/examples/jsm/renderers/CSS3DRenderer.js

  Line 259 in 79c3490

  object.element.style.display = ( visible === true ) ? '' : 'none';

• However, nested CSS3DObjects are not actually nested in the DOM scene graph but are flattened using a global matrix.
• Therefore, setting visible = false on a parent CSS3DObject does not affect its children.

I thought nested CSS3DObjects are also nested in the DOM scene graph and parents display: none should affect its children, but not.

Given these points, I think each CSS3DObject needs to check its ancestors' visible as this PR does.
Alternatively, CSS3DObjects could have a .visible setter that changes the CSS display property ('none' or 'block') of the element and its descendants (no need to check ancestors at every render).

[Mugen87]

Given these points, I think each CSS3DObject needs to check its ancestors' visible as this PR does.

The solution would be more consistent when the same coding patterns are applied like in WebGLRenderer. So if the code from #28293 (comment) would also work, I would prefer that approach.

[yomotsu]

That makes sense and I tested adding that.

function renderObject( object, scene, camera, cameraCSSMatrix ) {

	if ( object.visible === false ) return;	

The above will stop the rendering process of children, However, CSS3DObjects will remain rendered without matrix updates because DOM elements are rendered because HTML is rendered in "retained mode", unlike WebGL or Canvas2D.
If we take the above approach, I think all CSS3DObjects must be hidden before rendering, then, show them if visible === true

e.g.

this.render = function ( scene, camera ) {

  // snip
  hideAllObjects( scene ); // traverse and hide all objects regardless its visible state
  renderObject( scene, camera, cameraCSSMatrix ); // will render if object is visible. the process will be stopped if visible === false

};

[Mugen87]

https://rawcdn.githack.com/lafflan/three.js/patch-1/examples/index.html

[lafflan]

Do we still need to change the Object3D core code like this:

class Object3D{
  constructor(){
    ...
    // this.visible=true;
    this._visible=true;
    ...
  }
  set visible(val) {
    this._visible = val;
    // no need to traverse children's children,  becasuse they also have visible setter
    const children = this.children;
    for (let i = 0, l = children.length; i < l; i++) {
        children[i].visible = val; 
     }
  }

  get visible() {
    return this._visible;
  }
  ...
}

[Mugen87]

Let's not apply any changes to Object3D and focus on the CSS renderers instead.

[Mugen87]

The problem with ancestorIsVisible() is it performs a lot of redundant hierarchy traversal.

Do you mind implementing the PR differently based on #28293 (comment)?

[lafflan]

Done. @Mugen87 @yomotsu

[Mugen87]

It seems the ( object.visible === true ) && check further below can be removed now, right?

[yomotsu]

That's right. The object has to be visible === true below this line, so there's no need to check again.
Thus, line 138 can be

- const visible = ( object.visible === true ) && ( _vector.z >= - 1 && _vector.z <= 1 ) && ( object.layers.test( camera.layers ) === true );
+ const visible = ( _vector.z >= - 1 && _vector.z <= 1 ) && ( object.layers.test( camera.layers ) === true );

[yomotsu]

Additionally, the line that sets the CSS display value can look like this,
because all elements are already set to display: none due to hideObject(object)

- object.element.style.display = ( visible === true ) ? '' : 'none';

if ( visible === true ) {

 	object.onBeforeRender( _this, scene, camera );

	const element = object.element;

+	element.style.display = '';

[Mugen87]

Beautiful!

[lafflan]

@Mugen87 There is another problem in CSS2DRenderer.js 's zOrder method:

function zOrder(scene) {
  ...
  const distanceA = cache.objects.get(a).distanceToCameraSquared;
  const distanceB = cache.objects.get(b).distanceToCameraSquared;
  ...
}

Due to the cache setting condition that objects must be visible (including their ancestors), when an object is initially invisible ,the cache.objects.get(object) will return undefined, and the above code will report an error:

So it is necessary to check if the cache is empty and set a default value , some code like this:

const cacheDataA = cache.objects.get(a);
const distanceA = cacheDataA ? cacheDataA.distanceToCameraSquared : 0;
const cacheDataB = cache.objects.get(b);
const distanceB = cacheDataB ? cacheDataB.distanceToCameraSquared : 0;

[Mugen87]

Good catch! Would you file another PR with your fix?

[lafflan]

Done. New PR is #28330 @Mugen87