Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

garyo
Copy link
Collaborator

@garyo garyo commented Jan 26, 2022

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 to StructureComponent 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 in trackball-controls.ts. That seemed a lot simpler (and a lot less code change) than introducing a new viewer.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.

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.
Copy link
Collaborator

@fredludlow fredludlow left a 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...)

src/controls/trackball-controls.ts Outdated Show resolved Hide resolved
src/component/structure-component.ts Outdated Show resolved Hide resolved
src/component/structure-component.ts Outdated Show resolved Hide resolved
@fredludlow
Copy link
Collaborator

btw - it builds fine and tests pass etc:
https://fredludlow.com/ngl/pr-909/examples/webapp.html?script=test/pivot

@garyo
Copy link
Collaborator Author

garyo commented Feb 4, 2022

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 setPivot, ideally you want both changing the pivot and changing the selection to avoid jumping around. I don't know how to do both of those other than the way it is now (without huge refactoring of the matrix code)... but I'm open to alternatives.

Fix signature of `getCenterUntransformed()` and add comments.
@garyo garyo requested a review from fredludlow February 4, 2022 18:33
@fredludlow
Copy link
Collaborator

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 component.transform such that the final position is unchanged (counteracting the change in center+pivot).

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...

@fredludlow
Copy link
Collaborator

As for doing it in setPivot, ideally you want both changing the pivot and changing the selection to avoid jumping around.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants