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

Be able to select atoms and move them around #665

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

Conversation

lilyminium
Copy link

This pull request adds the following:

  • Being able to select or deselect atoms with shift+click, which is indicated by a transparent white VdW structure
  • Being able to deselect all atoms with shift+double click
  • Being able to drag selected atoms with shift+right-drag to alter their coordinates

There is room for other transformations, e.g. rotation (which I started on but isn't working very well). Being able to select by clicking is very convenient.

@fredludlow
Copy link
Collaborator

Thanks @lilyminium, this sounds great! I'll have a proper look at it as soon as I can.

Copy link
Collaborator

@ppillot ppillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank-you for making this PR, which would provide a good starting point for adding some editing capabilities to NGL, a much demanded new set of features!

I am really sorry to start the review process almost 4 years after you had submitted the PR in the first place. I understand that since then, you might have moved to other subjects and might not want to spend more time on this feature.
Nonetheless, given the implications of adding this kind of capabilities to NGL, I think we should proceed cautiously.
I can see that there have been several PR made in the direction of modifying the coordinates of atoms (one by @fredludlow and one by @garyo are still pending).

From the code review, I also noticed that there is a need for some utility functions in the NGL core, or some improvements to existing functions parameters (such as the way selections could be passed as strings, selection objects or atom sets interchangeably).

Based on that, a possible roadmap would be:

  • I make some changes to NGL to make the necessary modifications to pass atom sets as selections to the various utility functions (repr.setSelection, structure.eachAtom,...)
  • In a first step, this PR can be divided to add the capability to make interactive selections
  • During that time, it would be nice to have @fredludlow 's and @garyo 's PR addressed.
  • In a second step, we make a second PR to add the capabilities to translate atoms from the selection

* @param {number[]} indices array of selected atom indices
* @return {RepresentationElement} this object
*/
setSelectedAtomIndices (indices: number[]) {
Copy link
Collaborator

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 in eachAtom(fn, Sele). In that case, we can't use them in rear.selection, but that would probably be the most efficient thing to do.

@@ -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';
Copy link
Collaborator

Choose a reason for hiding this comment

The 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'
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

@@ -66,11 +67,13 @@ class StructureComponent extends Component {
pickBuffer: RingBuffer<number>
pickDict: SimpleDict<number[], number[]>
lastPick?: number
selectedAtomIndices: ToggleSet<number>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a BitAttay

@@ -89,6 +92,7 @@ class StructureComponent extends Component {

this.pickBuffer = createRingBuffer(4)
this.pickDict = createSimpleDict()
this.selectedAtomIndices = createToggleSet()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.selectedAtomIndices = this.structure.getAtomSet()

Comment on lines +85 to +87
labelUnit: 'angstrom',
arcVisible: true,
planeVisible: false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are specific to distance, angle/dihedral representations

* @type {[AtomProxy, AtomProxy]}
*/
get atoms () {
return [this.atom1, this.atom2]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

return proxies
}

get structureComponents() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have compList for the list of all components. That one could be named in a similar way structureCompList?

let proxies: AtomProxy[] = [];
this.eachComponent(function (sc: StructureComponent) {
sc.selectedAtomIndices.list.forEach(function (i) {
let ap = sc.structure.getAtomProxy(i)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An atomProxy is a quite complex object, with many methods. I would discourage creating arrays of atom proxies, even if I understand it is convenient in that case because we can have in the same array reference to atoms from different components.
Another approach could be to return an iterator instead, something along the lines of eachAtom but for the user selections.

Comment on lines +7 to +16
export interface ToggleSet<T> {
has: (value: T) => boolean
add: (value: T) => void
del: (value: T) => void
toggle: (value: T) => void
toggleAny: (value: T[]) => void
count: number
list: T[]
clear: () => void
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted previously, the BitArray object should be sufficient to cover the needs of ToggleSet.

@lilyminium
Copy link
Author

Thanks for the thoughtful and comprehensive review, @ppillot! The roadmap you suggested looks great and practical. Unfortunately it has been a while since I looked at NGLViewer or Javascript in general, so it would take some time for me to get my head back into this. I don't want to hold up any plans -- would it be better to close this PR and eventually work towards the two divided functionalities?

@ppillot
Copy link
Collaborator

ppillot commented Jan 22, 2023

No worries! I'll leave it open for now as it's a useful starting point.
I've opened the discussion with @fredludlow and we'll see how we advance starting from there.
Since then, I've already started working on more flexible selections which will accept BitArrays as their definitions. Once this is in, we'll hopefully be in a better position for the next steps.

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.

3 participants