Opacity resetting always sets opacity to 1 instead of original value

[NiklasPor]

Describe the bug / question
I expected that the implementation resets the opacity when we assignentity.opacity = undefined, In reality it seems like setting it to undefined will always set it to 1.

Should we track the original opacity of the elements by ourself or is this unintended behavior?

To Reproduce
Steps to reproduce the behavior:

1. Go to https://jsfiddle.net/zfd3rqng/
2. Left click on a window => opacity goes to 0.5
3. Right click on a window => opacity should reset, but actually is set to 1

Xeokit 2.6.94

[NiklasPor]

I just read through the mesh opacity implementation, looks like this is not intended as also the original color opacity is being overwritten and not stored somewhere:

xeokit-sdk/src/viewer/scene/model/SceneModelMesh.js

Lines 189 to 204 in e9c8daf

	_setOpacity(opacity, entityFlags) {
	const newTransparent = (opacity < 255);
	const lastTransparent = this._transparent;
	const changingTransparency = (lastTransparent !== newTransparent);
	this._color[3] = opacity;
	this._colorize[3] = opacity;
	this._transparent = newTransparent;
	if (this._colorizing) {
	this.layer.setColor(this.portionId, this._colorize);
	} else {
	this.layer.setColor(this.portionId, this._color);
	}
	if (changingTransparency) {
	this.layer.setTransparent(this.portionId, entityFlags, newTransparent);
	}
	}

I'd expect this to be a common use-case after completely coloring a transparent model to reset it without having the windows loose there transparency. Is there a recommended existing solution?

[NiklasPor]

Custom workaround for the meantime for SceneModelEntity & SceneModelMesh, this also works for objects which are only partially transparent:

/**
 * Sets the opacity of the given objects.
 * @param viewer - The viewer to set the opacity of.
 * @param objectIds - The ids of the objects to set the opacity of.
 * @param opacity - The opacity to set. Undefined to reset to the base opacity.
 */
function setObjectsOpacity(
  viewer: XEO.Viewer,
  objectIds: (string | number)[],
  opacity: number | undefined,
) {
  const models = new Set<XEO.SceneModel>();
  const quantizedOpacity = opacity == null ? undefined : opacity * 255;
  const opacityUpdated = quantizedOpacity != null;

  for (const objectId of objectIds) {
    const object = viewer.scene.objects[objectId];
    const anyObject = object as any;
    if (!object || anyObject.meshes.length === 0) {
      continue;
    }

    (viewer.scene as any)._objectOpacityUpdated(anyObject, quantizedOpacity);
    anyObject._opacityUpdated = opacityUpdated;
    models.add(anyObject.model);

    for (const mesh of anyObject.meshes) {
      setMeshOpacity(mesh, quantizedOpacity, anyObject._flags);
    }
  }

  for (const model of models) {
    (model as any).glRedraw();
  }
}

function setMeshOpacity(mesh: any, quantizedOpacity: number | undefined, entityFlags: unknown) {
  if (mesh._baseOpacity == null) {
    mesh._baseOpacity = mesh._color?.[3] ?? mesh._colorize?.[3];
  }

  const newOpacity = quantizedOpacity == null ? (mesh._baseOpacity ?? 255) : quantizedOpacity;
  mesh._setOpacity(newOpacity, entityFlags);
}

[paireks]

Hey @NiklasPor,

I was thinking about this one for a while, I think it is connected to this as well: #1971

Here is my personal opinion:

Generally it looks like there are some inconsistencies in "reverting" the original values of color and opacity.

One way to solve it on our side would be to change the existing behaviour, which is e.g. changing opacity behaviour to set it back to original values when null/undefined is assigned. However I'm not sure if such breaking change makes sense at this point, because if someone already depends on the existing behaviour (that setting the opacity to null makes it = 1.0), then we will break the user-space for such person. That's why I'm not sure if we should go this way.

On the other hand we discussed some time ago, that generally the topic of colorizing / setting the opacities / materials etc. should be probably redesigned to make it more flexible and more consistent. We don't have it in our roadmap yet, but have it in the backlog.

I'm happy you were able to find these workarounds, and you're right it should be more straight-forward. I hope in the future it would be designed in a more consistent and flexible way. If you have any comments and questions: please let us know.

Wojciech