161 -> 162 DragControls breaks for multiple groups

[jedleslie]

Description

I think this is related to the issue solved by PR #27791.

When enabling DragControls after 162, the Drag Controls onDrag handlers do not return the drag object inside the e.object. The drag object is one passed into the DragControls ctor, but when the events fire they are returning the parent object of that drag object.

Reproduction steps

1. Create a scene and add 2 group objects to the root of the scene
2. Enable just one of them in the DragControls ctor
3. View the onDrag event from the DragControls and see that it passses the object's parent as the "item being dragged" in the event.

Code

// code goes here

Live example

• jsfiddle-latest-release
• jsfiddle-dev

Screenshots

No response

Version

0.162

Device

Desktop

Browser

Chrome

OS

MacOS

[Mugen87]

Can you please demonstrate with a fiddle what's going wrong: https://jsfiddle.net/narenjcs/8ret1b9z/13/

It's currently not clear to me if this is a bug or if it is just the expected behavior.

[Mugen87]

I've created a fiddle by myself: https://jsfiddle.net/3cbrz8k4/3/

What we see is the expected behavior when enabling on transformGroup.

Objects organized in groups usually are logically connected so it makes sense when transformGroup is set to true, the entire group is affected. I understand there is the use case if just a single object of the hierarchy is added to the controls and transformGroup is set to true. In this case, users might expect the controls do not affect the hierarchy. But I'm unsure if this is right considering that enabling transformGroup should transform groups. That's the hole point of the property.

Nevertheless, we could basically check if the detected group is part of the objects array maintained by DragControls but that would potentially break other use cases which rely on the current behavior. As long as no other devs report the issue, let's keep the current default.

[Mugen87]

Nevertheless, we could basically check if the detected group is part of the objects array maintained by DragControls

Here is a custom version of DragControls that implements the suggested fix: https://jsfiddle.net/3bc8Lov7/

There is now a new helper function inObjects() that checks whether a found group belongs to the object hierarchies managed by DragControls.

[ctmbl]

Hi, I don't know if it's OK to re-open this closed issue, let me know if it isn't, I'll open another one 😉

I have a use case where the change implemented by #27791 is really counter-intuitive. Based on your example but updated with this code:

// Simple three.js example

import * as THREE from 'three';
import {
  OrbitControls
} from 'three/addons/controls/OrbitControls.js';
import {
  DragControls
} from 'three/addons/controls/DragControls.js';
let mesh, renderer, scene, camera, controls;

init();
animate();

function init() {

  // renderer
  renderer = new THREE.WebGLRenderer();
  renderer.setSize(window.innerWidth, window.innerHeight);
  renderer.setPixelRatio(window.devicePixelRatio);
  document.body.appendChild(renderer.domElement);

  // scene
  scene = new THREE.Scene();

  // camera
  camera = new THREE.PerspectiveCamera(40, window.innerWidth / window.innerHeight, 0.11, 100);
  camera.position.set(0, 10, 20);

  // controls
  controls = new OrbitControls(camera, renderer.domElement);

  // axes
  scene.add(new THREE.AxesHelper(20));

  const geometryPlane = new THREE.PlaneGeometry(15, 15);
  const materialPlane = new THREE.MeshBasicMaterial({
    color: 0x00aaaa
  });

  const geometryCylider = new THREE.CylinderGeometry();
  const materialCylider = new THREE.MeshBasicMaterial({
    color: 0xffff00
  });

  // There are 2 groups: 
  // physGroup: all physical objects of the group
  // draggableGroup: the draggable objects of the scene, child of physGroup
  const physGroup = new THREE.Group();
  const draggableGroup = new THREE.Group();
  scene.add(physGroup);
  physGroup.add(draggableGroup);

  // mesh
  const plane = new THREE.Mesh(geometryPlane, materialPlane);
  plane.position.set(0, -1, 0);
  plane.rotateX(-Math.PI / 2);

  const cubeA = new THREE.Mesh(geometryCylider, materialCylider);
  cubeA.position.set(5, 0, 0);

  const cubeB = new THREE.Mesh(geometryCylider, materialCylider);
  cubeB.position.set(-5, 0, 0);

  // Both cylinders are draggable: they go in the draggableGroup
  // The plane is a physical object but is not draggable: it goes in physGroup
  physGroup.add(plane);
  draggableGroup.add(cubeA)
  draggableGroup.add(cubeB)
  // Only the draggableGroup is included in the DragControl
  const dControl = new DragControls([draggableGroup], camera, renderer.domElement);
  dControl.transformGroup = true;

  dControl.addEventListener('dragstart', function(event) {

    controls.enabled = false
  })
  dControl.addEventListener('dragend', function(event) {

    controls.enabled = true
  })
}

function animate() {

  requestAnimationFrame(animate);

  //controls.update();

  renderer.render(scene, camera);

}

The most interesting parts being:

  // There are 2 groups: 
  // physGroup: all physical objects of the group
  // draggableGroup: the draggable objects of the scene, child of physGroup
  const physGroup = new THREE.Group();
  const draggableGroup = new THREE.Group();
  scene.add(physGroup);
  physGroup.add(draggableGroup);

  // Both cylinders are draggable: they go in the draggableGroup
  // The plane is a physical object but is not draggable: it goes in physGroup
  physGroup.add(plane);
  draggableGroup.add(cubeA)
  draggableGroup.add(cubeB)
  // Only the draggableGroup is included in the DragControl
  const dControl = new DragControls([draggableGroup], camera, renderer.domElement);
  dControl.transformGroup = true;

I'd expect that because draggableGroup is registered in DragControl only this group will transform as a group when dControl.transformGroup == true.
However since #27791 the outermost group is transform, here being physGroup that includes the plane which seems a bit counter-intuitive to me.

Moreover, apart from the custom implementation you proposed I don't see any workaround. Could we imagine making this behavior ("looking for the outermost group") configurable?