-
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
Be able to select atoms and move them around #665
base: master
Are you sure you want to change the base?
Changes from all commits
8659f99
5c4c238
597b143
3c73779
2dd7c45
a8ccc7b
c645b04
a63c03b
4a70f4b
07da327
6583f28
c14a744
fb2968d
165e0ba
ed230c7
52750f0
3a11153
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ | |
|
||
import { Signal } from 'signals' | ||
|
||
import { ComponentRegistry, MeasurementDefaultParams } from '../globals' | ||
import { ComponentRegistry, MeasurementDefaultParams, SelectionDefaultParams } from '../globals' | ||
import { | ||
defaults, /*deepEqual, */createRingBuffer, RingBuffer, createSimpleDict, SimpleDict | ||
} from '../utils' | ||
|
@@ -25,11 +25,12 @@ import Stage from '../stage/stage' | |
import StructureRepresentation from '../representation/structure-representation' | ||
import AtomProxy from '../proxy/atom-proxy' | ||
import { Vector3, Box3 } from 'three'; | ||
import { createToggleSet, ToggleSet } from '../utils/toggle-set'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please consider using BitArray objects that you can get from structure.getAtomSet(), instead. |
||
|
||
export type StructureRepresentationType = ( | ||
'angle'|'axes'|'backbone'|'ball+stick'|'base'|'cartoon'|'contact'|'dihedral'| | ||
'distance'|'helixorient'|'hyperball'|'label'|'licorice'|'line'|'surface'| | ||
'ribbon'|'rocket'|'rope'|'spacefill'|'trace'|'tube'|'unitcell' | ||
'ribbon'|'rocket'|'rope'|'selected'|'spacefill'|'trace'|'tube'|'unitcell' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why 'selected' should be a new type of representation, as it's a space fill representation? |
||
) | ||
|
||
export const StructureComponentDefaultParameters = Object.assign({ | ||
|
@@ -66,11 +67,13 @@ class StructureComponent extends Component { | |
pickBuffer: RingBuffer<number> | ||
pickDict: SimpleDict<number[], number[]> | ||
lastPick?: number | ||
selectedAtomIndices: ToggleSet<number> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a BitAttay |
||
|
||
spacefillRepresentation: RepresentationElement | ||
distanceRepresentation: RepresentationElement | ||
angleRepresentation: RepresentationElement | ||
dihedralRepresentation: RepresentationElement | ||
selectedRepresentation: RepresentationElement | ||
|
||
measureRepresentations: RepresentationCollection | ||
|
||
|
@@ -89,6 +92,7 @@ class StructureComponent extends Component { | |
|
||
this.pickBuffer = createRingBuffer(4) | ||
this.pickDict = createSimpleDict() | ||
this.selectedAtomIndices = createToggleSet() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
this.spacefillRepresentation = this.addRepresentation('spacefill', { | ||
sele: 'none', | ||
|
@@ -108,6 +112,15 @@ class StructureComponent extends Component { | |
'dihedral', MeasurementDefaultParams, true | ||
) | ||
|
||
this.selectedRepresentation = this.addRepresentation('spacefill', { | ||
sele: 'none', | ||
color: SelectionDefaultParams.color, | ||
opacity: 0.4, | ||
disablePicking: true, | ||
radiusType: 'data' | ||
}, true) | ||
|
||
|
||
this.measureRepresentations = new RepresentationCollection([ | ||
this.spacefillRepresentation, | ||
this.distanceRepresentation, | ||
|
@@ -173,14 +186,15 @@ class StructureComponent extends Component { | |
* @param {String} value - assembly name | ||
* @return {undefined} | ||
*/ | ||
setDefaultAssembly (value:string) { | ||
// filter out non-exsisting assemblies | ||
setDefaultAssembly (value: string) { | ||
// filter out non-existing assemblies | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks! |
||
if (this.structure.biomolDict[value] === undefined) value = '' | ||
// only set default assembly when changed | ||
if (this.parameters.defaultAssembly !== value) { | ||
const reprParams = { defaultAssembly: value } | ||
this.reprList.forEach(repr => repr.setParameters(reprParams)) | ||
this.measureRepresentations.setParameters(reprParams) | ||
this.selectedRepresentation.setParameters(reprParams) | ||
this.parameters.defaultAssembly = value | ||
this.signals.defaultAssemblyChanged.dispatch(value) | ||
} | ||
|
@@ -196,6 +210,7 @@ class StructureComponent extends Component { | |
repr.build() | ||
}) | ||
this.measureRepresentations.build() | ||
this.selectedRepresentation.build() | ||
} | ||
|
||
/** | ||
|
@@ -211,6 +226,7 @@ class StructureComponent extends Component { | |
updateRepresentations (what: any) { | ||
super.updateRepresentations(what) | ||
this.measureRepresentations.update(what) | ||
this.selectedRepresentation.update(what) | ||
} | ||
|
||
addRepresentation (type: StructureRepresentationType, params: { [k: string]: any } = {}, hidden = false) { | ||
|
@@ -258,6 +274,7 @@ class StructureComponent extends Component { | |
this.trajList.length = 0 | ||
this.structure.dispose() | ||
this.measureRepresentations.dispose() | ||
this.selectedRepresentation.dispose() | ||
|
||
super.dispose() | ||
} | ||
|
@@ -431,6 +448,41 @@ class StructureComponent extends Component { | |
} | ||
this.measureBuild() | ||
} | ||
|
||
selectedPick (atoms: AtomProxy[]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An array of atomProxies would be inefficient: each atomProxy is a new object. In fact it's more convenient to use an atomSet (BitArray) or an array of indices and update a unique atomProxy with this value within a loop. const ap = structure.getAtomProxy();
atomSet.forEach((atomidx) => {
ap.index = atomidx;
console.log(ap.isProtein())
}) |
||
let atomIndices: number[] = [] | ||
atoms.forEach(function (x) { | ||
atomIndices.push(x.index) | ||
}) | ||
this.selectedPickIndices(atomIndices) | ||
} | ||
|
||
selectedPickIndices (atomIndices: number[]) { | ||
this.selectedAtomIndices.toggleAny(atomIndices) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with atomSets, this would require:
|
||
this.selectedUpdate() | ||
} | ||
|
||
setSelectedIndices (atomIndices: number[]) { | ||
this.selectedAtomIndices.clear() | ||
for (var i = 0; i < atomIndices.length; i++) { | ||
this.selectedAtomIndices.add(atomIndices[i]) | ||
} | ||
this.selectedUpdate() | ||
} | ||
|
||
|
||
selectedUpdate () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function duplicates the code from The second observation is that it looks quite heavy, given that selections might be large. The |
||
const pickData = this.selectedAtomIndices.list | ||
const radiusData: { [k: number]: number } = {} | ||
pickData.forEach(ai => { | ||
const r = Math.max(0.1, this.getMaxRepresentationRadius(ai)) | ||
radiusData[ai] = r * (2.3 - smoothstep(0.1, 2, r)) | ||
}) | ||
this.selectedRepresentation.setSelection( | ||
pickData.length ? ('@' + pickData.join(',')) : 'none' | ||
) | ||
this.selectedRepresentation.setParameters({ radiusData }) | ||
} | ||
} | ||
|
||
export const enum MeasurementFlags { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -176,6 +176,29 @@ class MouseActions { | |
stage.trackballControls.rotateComponent(dx, dy) | ||
} | ||
|
||
/** | ||
* Move picked component based on mouse coordinate changes | ||
* @param {Stage} stage | ||
* @param {Number} dx - amount to move in x direction | ||
* @param {Number} dy - amount to move in y direction | ||
* @return {undefined} | ||
*/ | ||
static moveComponentDrag (stage: Stage, dx: number, dy: number) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming is confusing to me here: there is already a |
||
stage.trackballControls.translateAtoms(dx, dy) | ||
} | ||
|
||
/** | ||
* Rotate picked component based on mouse coordinate changes. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rotate picked atoms (a component is something else) |
||
* Doesn't work very well; not implemented in mouse presets. | ||
* @param {Stage} stage | ||
* @param {Number} dx - amount to move in x direction | ||
* @param {Number} dy - amount to move in y direction | ||
Comment on lines
+194
to
+195
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment should be updated to describe a rotation, not a translation. |
||
* @return {undefined} | ||
*/ | ||
static rotateSelectionDrag (stage: Stage, dx: number, dy: number) { | ||
stage.trackballControls.rotateAtoms(dx, dy) | ||
} | ||
|
||
/** | ||
* Move picked element to the center of the screen | ||
* @param {Stage} stage - the stage | ||
|
@@ -217,6 +240,27 @@ class MouseActions { | |
stage.measureClear() | ||
} | ||
} | ||
|
||
/** | ||
* Add pick | ||
* @param {Stage} stage - the stage | ||
* @param {PickingProxy} pickingProxy - the picking data object | ||
* @return {undefined} | ||
*/ | ||
static selectPick (stage: Stage, pickingProxy: PickingProxy) { | ||
if (pickingProxy && (pickingProxy.atom || pickingProxy.bond)) { | ||
const atoms = [pickingProxy.atom.index] || [pickingProxy.bond.atom1.index, pickingProxy.bond.atom2.index] | ||
const sc = pickingProxy.component as StructureComponent | ||
sc.selectedPickIndices(atoms) | ||
} | ||
} | ||
|
||
static clearSelect (stage: Stage) { | ||
stage.eachComponent(function (sc: StructureComponent) { | ||
sc.selectedAtomIndices.clear() | ||
sc.selectedUpdate() | ||
}, "structure") | ||
} | ||
} | ||
|
||
type MouseActionPreset = [ string, MouseActionCallback ][] | ||
|
@@ -231,11 +275,14 @@ export const MouseActionPresets = { | |
[ 'drag-right', MouseActions.panDrag ], | ||
[ 'drag-ctrl-left', MouseActions.panDrag ], | ||
[ 'drag-ctrl-right', MouseActions.zRotateDrag ], | ||
[ 'drag-shift-right', MouseActions.moveComponentDrag ], | ||
[ 'drag-shift-left', MouseActions.zoomDrag ], | ||
[ 'drag-middle', MouseActions.zoomFocusDrag ], | ||
|
||
[ 'drag-ctrl-shift-right', MouseActions.panComponentDrag ], | ||
[ 'drag-ctrl-shift-left', MouseActions.rotateComponentDrag ], | ||
[ 'clickPick-shift-left', MouseActions.selectPick ], | ||
[ 'doubleClick-shift-left', MouseActions.clearSelect ], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am wondering wether clicking on the background my be more intuitive for de-selecting? |
||
|
||
[ 'clickPick-right', MouseActions.measurePick ], | ||
[ 'clickPick-ctrl-left', MouseActions.measurePick ], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ import Viewer from '../viewer/viewer' | |
import ViewerControls from './viewer-controls' | ||
import AtomProxy from '../proxy/atom-proxy'; | ||
import Component from '../component/component'; | ||
import StructureComponent from '../component/structure-component'; | ||
|
||
const tmpRotateXMatrix = new Matrix4() | ||
const tmpRotateYMatrix = new Matrix4() | ||
|
@@ -118,6 +119,45 @@ class TrackballControls { | |
this.component.updateRepresentations({ 'position': true }) | ||
} | ||
|
||
private _translateAtom (panMatrix: Matrix4, atom: AtomProxy, x: number, y: number) { | ||
let _tmpAtomVector = new Vector3(); | ||
let _tmpPanVector = new Vector3(); | ||
atom.positionToVector3(_tmpAtomVector) | ||
_tmpAtomVector.add(this.viewer.translationGroup.position) | ||
_tmpAtomVector.applyMatrix4(this.viewer.rotationGroup.matrix) | ||
|
||
let scaleFactor = this.controls.getCanvasScaleFactor(_tmpAtomVector.z) | ||
_tmpPanVector.set(x, y, 0) | ||
_tmpPanVector.multiplyScalar(this.panSpeed * scaleFactor) | ||
_tmpPanVector.applyMatrix4(panMatrix) | ||
|
||
|
||
atom.positionAdd(_tmpPanVector) | ||
} | ||
|
||
private _translateComponent (comp: StructureComponent, x: number, y: number) { | ||
|
||
let _tmpPanMatrix = new Matrix4(); | ||
_tmpPanMatrix.extractRotation(comp.transform) | ||
_tmpPanMatrix.premultiply(this.viewer.rotationGroup.matrix) | ||
_tmpPanMatrix.getInverse(_tmpPanMatrix) | ||
Comment on lines
+140
to
+143
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not need to be executed if the current component does not have active selection. I would suggest to add an early return before these statements. |
||
|
||
for (var j = 0; j < comp.selectedAtomIndices.list.length; j++) { | ||
let i = comp.selectedAtomIndices.list[j] | ||
let atom = comp.structure.getAtomProxy(i) | ||
this._translateAtom(_tmpPanMatrix, atom, x, y) | ||
} | ||
Comment on lines
+145
to
+149
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. executing 6 vector based manipulations on each atom of the selection seems to be quite heavy. As we are only doing a translation, I would assume that the same translation vector is applied to every atom in the selection, so that it can be computed only once. |
||
|
||
comp.updateRepresentations({'position': true}) | ||
} | ||
|
||
translateAtoms (x: number, y: number) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be renamed to |
||
let comps = this.stage.structureComponents | ||
for (var i = 0; i < comps.length; i++) { | ||
this._translateComponent(comps[i], x, y) | ||
} | ||
} | ||
|
||
rotate (x: number, y: number) { | ||
const [ dx, dy ] = this._getRotateXY(x, y) | ||
|
||
|
@@ -153,6 +193,51 @@ class TrackballControls { | |
this.component.quaternion.premultiply(tmpRotateQuaternion) | ||
this.component.updateMatrix() | ||
} | ||
|
||
_rotateAtoms (comp: StructureComponent, x: number, y: number) { | ||
// TODO | ||
// Fix rotation | ||
const [ dx, dy ] = this._getRotateXY(x, y) | ||
|
||
let rot: Matrix4 = this.viewer.rotationGroup.matrix | ||
let rotInv: Matrix4 = this.viewer.rotationGroup.matrix | ||
rotInv.getInverse(rotInv) | ||
|
||
tmpRotateMatrix.extractRotation(comp.transform) | ||
tmpRotateMatrix.premultiply(rot) | ||
tmpRotateMatrix.getInverse(tmpRotateMatrix) | ||
tmpRotateVector.set(1, 0, 0) | ||
tmpRotateVector.applyMatrix4(tmpRotateMatrix) | ||
tmpRotateXMatrix.makeRotationAxis(tmpRotateVector, dy) | ||
tmpRotateVector.set(0, 1, 0) | ||
tmpRotateVector.applyMatrix4(tmpRotateMatrix) | ||
tmpRotateYMatrix.makeRotationAxis(tmpRotateVector, dx) | ||
tmpRotateXMatrix.multiply(tmpRotateYMatrix) | ||
tmpRotateQuaternion.setFromRotationMatrix(tmpRotateXMatrix) | ||
|
||
for (var j = 0; j < comp.selectedAtomIndices.list.length; j++) { | ||
let i = comp.selectedAtomIndices.list[j] | ||
let atom = comp.structure.getAtomProxy(i) | ||
let _tmpAtomVector = new Vector3(); | ||
atom.positionToVector3(_tmpAtomVector) | ||
|
||
_tmpAtomVector.add(this.viewer.translationGroup.position) | ||
_tmpAtomVector.applyMatrix4(rot) | ||
_tmpAtomVector.applyQuaternion(tmpRotateQuaternion) | ||
_tmpAtomVector.applyMatrix4(rotInv) | ||
_tmpAtomVector.sub(this.viewer.translationGroup.position) | ||
atom.positionFromVector3(_tmpAtomVector) | ||
} | ||
|
||
comp.updateRepresentations({'position': true}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same goes for the translate method: after such a transformation has been done, we need to call "refreshPositions" to update the boundingBoxes and the spatialHash used for coordinates based selections. |
||
} | ||
|
||
rotateAtoms (x: number, y: number) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer a name such as |
||
let comps = this.stage.structureComponents | ||
for (var i = 0; i < comps.length; i++) { | ||
this._rotateAtoms(comps[i], x, y) | ||
} | ||
} | ||
} | ||
|
||
export default TrackballControls |
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.
NGL uses the concept of BitArray to express a list of atom indices with many advantages (compact, very fast manipulations such as intersections, etc...).
Unfortunately these BitArrays are not used ubiquitously where selections are made. For example, you can use them in
getAtomSetWithinSelection
but not ineachAtom(fn, Sele)
. In that case, we can't use them in rear.selection, but that would probably be the most efficient thing to do.