Proposal - changing structure.refreshed signal and logic

[fredludlow]

NGL supports updating atomic coordinates (there's an updatePosition method on structure, and the trajectory viewer works this way). Currently however, there's not really a consistent way to listen for coordinate changes. A StructureComponent can have a trajectory, and listens for frameChanged to updates representations.

This is problematic when you have multiple parts of your app that might update coordinates, but all of which need to know when coordinates change.

This PR assumes Structure.updatePosition is the public API for updating atomic coordinates. This method has a new refresh parameter (default true, set to false for old behaviour) which will call Structure.refreshPosition, this recalculates bounding boxes etc and fires the refreshed signal which triggers representation rebuilds and which other code (i.e. your application) can subscribe to.

This also means StructureComponent doesn't need to listen for frameChanged on trajectory any more.

The main problem I can foresee if developers are already doing something clever calling updatePosition and then manually choosing what gets updated - in this case passing in false as the second argument will restore old behaviour

const structure = stage.compList[0].structure
newCoords: number[] = calcNewCoords(structure)
structure.udpatePositions(newCoords, true) // All representations updated automatically

const structure = stage.compList[0].structure
newCoords: number[] = calcNewCoords(structure)
structure.udpatePositions(newCoords, false) 
// Manually choose which repr to rebuild etc.

Comments etc very welcome!

[ppillot]

Thanks Fred for this! What happens then in the case of a trajectory? If I understand correctly, the spatial hash would be recomputed which might hinder performance when playing a trajectory? This is a guess, I don't know what is actually the time penalty for this. Maybe this could be made an option for trajectories as there are limitations with the current situation which could be acceptable tradeoffs in most of the cases (e.g. contacts can not be computed consistently between trajectory frames if one does not explicitly trigger structure.refreshPosition() which is a limitation, but maybe there are few use cases).

As an aside, the secondary structure is only calculated once, at parse time, and it could be interesting to optionally "refresh" this information as well. Once again, this comes at a computational cost.

[fredludlow]

Let's talk about this with reference to editing as well, but one idea would be to upgrade the boolean refresh parameter to an object (cf. the what parameter in updateRepresentations)

type RefreshOptions {
  representations: boolean,
  secondary: boolean,
  hash: boolean
}
updatePosition (position: Float32Array|number[], refresh: boolean | RefreshOptions = true)

If it looks like it isn't a performance issue to refresh everything then can stick with simple boolean - but in that case secondary structure should probably be in there too..

[ppillot]

I've done some benchmarking using a trajectory on a medium sized system (400 residues, no water displayed) The re-computation of the spatial hash is only perceptible in the profiling tools if I throttle the CPU. Then it takes less that 1% of the rendering time. I take into account the whole refresh routine (incl. bounding box...) it's 3%. In absolute value it's 0.6ms out of 18ms rendering time. I've compared with the previous version (without refresh) and it's consistent with the measured timings.

So, I think it's fine to simply do the refresh as you suggested in this PR.

But, I've also tried to run the NGLviewer using the ?debug=y url parameter. In the console.log, I can see multiple calls to getBoundingBox (x3) and to StructureView.refresh (x8) for the same frame, see the comparison bellow (times should not be compared due to throttling)

I don't know if this should be investigated? Are StructureView.refresh() somehow also triggered by each representation?

[ppillot]

After some more digging in the code, a StructureView object is created for each representation and each one attaches to Structure.signals.refreshed. The listener is the StructureView.refresh() function which updates some properties of the StructureView such as the boundingBox, center and some atomSets. This refresh function was apparently previously only triggered when a change of selection was being made. It seems genuine to update the coordinates of the bounding boxes for each representation in the context of what this PR does, so I don't really think there is an issue here (except the unnecessary recomputation of the atoms sets).

I'll investigate what can be done for the secondary structure update. I also would like to check what is happening for the contact representations.