-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/rotate selection and pivot #909
base: master
Are you sure you want to change the base?
Conversation
This adds a `pivot` property to Component, with a `setPivot` method. It also arranges for StructureComponents to optionally pass the current selection to Component.updateMatrix when it gets the component's center. StructureComponents now can set their centering mode to "selection" or "component". This makes components able to rotate around their selection's center, not just the complete structure's center. To change the centering mode, call new method `StructureComponent.setCenterMode()`. The default is set to "selection", which is new & improved behavior. Set it to "component" to get the old behavior. This also adds a pivot.js example to demonstrate the pivot feature in a couple of different ways, as well as demonstrate selection-based centering, showing how to update the transform when changing selection to avoid molecules jumping around. (We could do that in the core code, but since it uses Component.transform, users who are already using that would be impacted -- so probably best to just show it in an example and users can adapt it to suit.)
This allows users to scale the overall scene by applying the scale factor to the rotationGroup. The only real change is to extract the rotation of the rotationGroup in the trackball controls, so the trackball doesn't go to fast or slow when the scale changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @garyo. I have one major request which is that we preserve the function signature getCenterUntransformed(sele: string)
so that we can continue to pass selections through (though happy to have it handle additional types of arguments if needed).
I spent a while wondering if there was a simpler way to achieve this that avoids updateMatrix
calls when selection changes (and then the handling you need to do to avoid the structure jumping around when selection changes). I think the issue is that you need to know the pivot point every time the overall matrix gets updated, and changing the pivot point requires the pre-transform you do in the example). i.e. in this model, the pivot is a fundamental part of describing the final position.
I then wondered if it is possible to use the pivot point in the TrackballControl code, so that the rotateComponent
function actually updates both rotation and translation of the component (according to the position of the pivot) - i.e. the component's updateMatrix
code would no longer need to worry about the pivot, the pre-transform is done in TrackballControl
The problem with that way is that it makes it hard to do a rotation around an arbitrary point from anywhere other than trackball controls code (you'd have to reimplement your pretransform logic).
So then I wondered, should the pre-transform from your example go into the setPivot
method? i.e. should setPivot be written such that the overall matrix does not change? (I've only just thought of that last one, so it's quite possible it's also a bad idea...)
btw - it builds fine and tests pass etc: |
I did try moving some of this into the trackball code, but since my main application for this is animated motion tracks for the molecules, having it there makes that harder. I wish the pre-transform to compensate for selection changes wasn't needed but I don't think there's any good way to avoid it. As for doing it in |
Fix signature of `getCenterUntransformed()` and add comments.
Right, I think I understand now: As you said: When you update selection, the center changes. We still want to keep the pivot as a relative offset from the center (the clear case for this is that if it's at 0,0,0 for the whole structure and you select only one chain, you don't want pivot jumping to some offset and negating the nice behaviour of rotating around the center of the selection). Your example code recalculates Aside: matrix inversions get a bad rep but I think we're good here - it would be possible to calculate the new transform without the inversion from a combination of old and new centers, but this is shorter (otherwise you need to handle scale too) I tried moving the selection change code from your pivot example into the selection.signals.stringChanged handler in structure-component.ts, i.e: this.selection.signals.stringChanged.add(() => {
this.structureView.setSelection(this.selection)
this.rebuildRepresentations()
this.rebuildTrajectories()
if (this.centerMode === 'selection') {
// Selection's center point has changed, and in center mode we use
// the selection center in the matrix (via getCenterUntransformed),
// so we need to recalculate the matrix.
// Additionally:
// When the selection changes, we don't want the molecule to move
// around. So compute a pre-transform that will take the new matrix,
// based on the new center point, back to the old matrix. At this
// point, it's important that the matrix hasn't yet been updated to
// reflect the new selection's center point.
const oldMatrix = this.matrix.clone()
this.updateMatrix(true) // get matrix w/ new selection-center (silently, no signals)
// Update pre-transform to make final result same as m0 (old matrix)
// T' = m0 * m1^-1 * T
tmpMat4.getInverse(this.matrix)
this.transform.premultiply(tmpMat4).premultiply(oldMatrix)
this.updateMatrix()
}
}) That seems to work for me, though as always I'm probably overlooking something... |
Yes, this would also be nice, I guess a similar fudge to transform could be done in the setPivot code? That said, if you're playing around with the pivot at least you would understand (or could make an educated guess) why your molecule has moved, but jumping around on selection change is definitely more surprising. |
This PR adds a pivot point around which rotations happen to a component. This makes it easier to rotate a component interactively around an atom, the component's center, or any other point.
Based on that, it adds a
CenterMode
toStructureComponent
to allow interactively rotating around the current selection or the structure's center.This PR also allows the
viewer.rotationGroup
to have a non-unit scale factor to scale the whole scene, for when it is included in a larger three.js scene. It simply extracts the rotation part only intrackball-controls.ts
. That seemed a lot simpler (and a lot less code change) than introducing a newviewer.scaleGroup
.Adds an example to show how it works. There is some complexity to handling selection-changes so it seemed worth creating the fully worked example.