MolecularViewer refactor and optimisations

[ChiCheng45]

Description of work
Refactored the molecule viewer code.

Improved bond and atom trace calculation performance.

Changed the bonds setting so that it can be calculated from the first, last, or every frame, or it can be taken from the file; the default is every frame.

Improved performance, larger systems should run faster. I think after this change the performances are probably good enough to be able implement #500 and not have it slow down too much. Here is the before and after on my old Mac with a 61K atom trajectory.

Before

After

To test
All aspects of the 3D viewer including atom selection, and atom tracing plotting. Test the different bond settings. 3D viewer should be much faster, test with a large trajectory.

[oerc0122]

Looks good. Couple of stylistic and defensive things.

[MBartkowiakSTFC]

@MBartkowiakSTFC any preference on the default choice for the bond calculation setting? I think it should be either first frame or every frame.

I think that "from file" also has a fairly good claim to be the default option. But if you want to make sure that the users see the bonds immediately, then it should be "every frame", because it avoids glitches with bonds stretching across the entire box.

Side note: should we extend the labels to say something like "calculate every frame", so the users realise that this will be a slower option, as opposed to "read from file"? If it doesn't fit, you could always change the grid layout to be:

empty | atom checkbox | cell checkbox
axes | combo box spans two columns
labels | combo box spans two columns
bonds | combo box spans two columns 

[MBartkowiakSTFC]

In this branch, manual selection is more tricky on my machine, since the atoms are pretty much invisible once I have deselected them:
Atoms selected:

Some atoms deselected:

In this case I could still manage to find the atoms by switching the bonds on. Do you think this needs improving?

[MBartkowiakSTFC]

Overall, the performance is greatly improved, as demonstrated in my tests yesterday with 4,000,000 atoms in a trajectory.

However, on my computer the performance becomes bad even for small trajectories if I turn on the labels, e.g. atom indices.

[ChiCheng45]

@MBartkowiakSTFC

I've change the bond calc description, increased the unpicked opacity from 0.20 to 0.30, and reverted the text label to use followers again. Not sure why it was so slow on the Mac, the performance of the newer implementation of the atom labels seemed similar to the follower version on windows. Let me know if the opacity change is better or if I needs to be increased further.

[MBartkowiakSTFC]

Thanks for the changes. The speed is back to normal when using labels. The opacity is better. Even though I find some atoms still hard to see, it can be easily fixed by changing the colour of the background.

I noticed one more problem that may need fixing.

1. I load a trajectory that does not contain atom labels or molecule definitions,
2. I start playing the animation,
3. It is possible to change different settings while playing the animation: cell/bonds/atoms visibility, axis type, etc.
4. If I set labels: to label or molecule, the atom positions stop updating in the 3D view, and every frame produces an error:

<class 'ValueError'>
zip() argument 2 is longer than argument 1
<traceback object at 0x314e3ee80>
Traceback (most recent call last):
  File "MDANSE_GUI/MolecularViewer/MolecularViewer.py", line 1094, in set_coordinates
    self.update_atom_labels()
  File "MDANSE_GUI/MolecularViewer/MolecularViewer.py", line 374, in update_atom_labels
    for follower, coord in zip(
ValueError: zip() argument 2 is longer than argument 1

At the same time, the animation progress bar is still moving and changing the frame numbers.

[oerc0122]

Thanks for the changes. The speed is back to normal when using labels. The opacity is better. Even though I find some atoms still hard to see, it can be easily fixed by changing the colour of the background.

I noticed one more problem that may need fixing.

1. I load a trajectory that does not contain atom labels or molecule definitions,
2. I start playing the animation,
3. It is possible to change different settings while playing the animation: cell/bonds/atoms visibility, axis type, etc.
4. If I set labels: to label or molecule, the atom positions stop updating in the 3D view, and every frame produces an error:

<class 'ValueError'>
zip() argument 2 is longer than argument 1
<traceback object at 0x314e3ee80>
Traceback (most recent call last):
  File "MDANSE_GUI/MolecularViewer/MolecularViewer.py", line 1094, in set_coordinates
    self.update_atom_labels()
  File "MDANSE_GUI/MolecularViewer/MolecularViewer.py", line 374, in update_atom_labels
    for follower, coord in zip(
ValueError: zip() argument 2 is longer than argument 1

At the same time, the animation progress bar is still moving and changing the frame numbers.

Probably want itertools.zip_longest(..., fillvalue=None) or similar

[ChiCheng45]

Thanks for the changes. The speed is back to normal when using labels. The opacity is better. Even though I find some atoms still hard to see, it can be easily fixed by changing the colour of the background.

I noticed one more problem that may need fixing.

1. I load a trajectory that does not contain atom labels or molecule definitions,
2. I start playing the animation,
3. It is possible to change different settings while playing the animation: cell/bonds/atoms visibility, axis type, etc.
4. If I set labels: to label or molecule, the atom positions stop updating in the 3D view, and every frame produces an error:

<class 'ValueError'>
zip() argument 2 is longer than argument 1
<traceback object at 0x314e3ee80>
Traceback (most recent call last):
  File "MDANSE_GUI/MolecularViewer/MolecularViewer.py", line 1094, in set_coordinates
    self.update_atom_labels()
  File "MDANSE_GUI/MolecularViewer/MolecularViewer.py", line 374, in update_atom_labels
    for follower, coord in zip(
ValueError: zip() argument 2 is longer than argument 1

At the same time, the animation progress bar is still moving and changing the frame numbers.

Thanks, this should be fixed now.

For the opacity, 0.2 was the value on protos for unpicked. It was set that way because so that it was easier to see picked atoms, if unpicked opacity becomes too large then it can be more difficult to determine what is and isn't picked. Probably need to add some opacity setting, but this will have to be done in another PR I think.

[ChiCheng45]

@MBartkowiakSTFC

In the molecular viewer, there are a few unused functions that I have chosen to leave in the code. Did you still want these here? Do you have some plans to update the molecule viewer to use them at some point?

    def change_atom_trace_opacity(self, surface_index: int, opacity: float):
        """This method should allow changing the opacity of an already existing
        isosurface. Updates the renderer.

        Currently not connected to any widgets.

        Parameters
        ----------
        surface_index : int
            index of the isosurface in self._surfaces
        opacity : float
            new opacity value for the isosurface
        """

        if surface_index >= len(self.surface_actors):
            return

        self.surface_actors[surface_index].GetProperty().SetOpacity(opacity)
        self.update_renderer()

    def change_atom_trace_isocontour_level(self, surface_index: int, level: float):
        """This method should allow changing the isocontour level for an already existing
        isosurface. Updates the renderer.

        Currently not connected to any widgets.

        Parameters
        ----------
        surface_index : int
            index of the isosurface in self._surfaces
        level : float
            new value of isocontour level
        """

        if surface_index >= len(self.surface_actors):
            return

        self.isocontours[surface_index].SetValue(0, level)
        self.isocontours[surface_index].Update()
        self.update_renderer()

    def change_atom_trace_rendering_type(self, surface_index: int, rendering_type: str):
        """Method for changing the rendering style of an existing isosurface.
        Updates the renderer.

        Currently not connected to any widgets.

        Parameters
        ----------
        surface_index : int
            index of the isosurface in self._surfaces
        rendering_type : str
            one of the following: wireframe, surface, points
        """

        if surface_index >= len(self.surface_actors):
            return

        surface = self.surface_actors[surface_index]

        if rendering_type == "wireframe":
            surface.GetProperty().SetRepresentationToWireframe()
        elif rendering_type == "surface":
            surface.GetProperty().SetRepresentationToSurface()
        elif rendering_type == "points":
            surface.GetProperty().SetRepresentationToPoints()
            surface.GetProperty().SetPointSize(3)
        else:
            return

        self.update_renderer()

[MBartkowiakSTFC]

For the opacity, 0.2 was the value on protos for unpicked. It was set that way because so that it was easier to see picked atoms, if unpicked opacity becomes too large then it can be more difficult to determine what is and isn't picked. Probably need to add some opacity setting, but this will have to be done in another PR I think.

We don't have to deal with this now, but definitely this branch produces different level of transparency than protos. I saved a screenshot of the same atoms using the main branch and then using this branch. Then I checked the RGB values of the brightest spot on the carbon atom. This branch is still darker.

I agree that this can be addressed in a separate PR.

[MBartkowiakSTFC]

In the molecular viewer, there are a few unused functions that I have chosen to leave in the code. Did you still want these here? Do you have some plans to update the molecule viewer to use them at some point?

I think that it is becoming increasingly unlikely that we will use these methods. If they are not used at the moment, you are welcome to remove them.

[ChiCheng45]

For the opacity, 0.2 was the value on protos for unpicked. It was set that way because so that it was easier to see picked atoms, if unpicked opacity becomes too large then it can be more difficult to determine what is and isn't picked. Probably need to add some opacity setting, but this will have to be done in another PR I think.

We don't have to deal with this now, but definitely this branch produces different level of transparency than protos. I saved a screenshot of the same atoms using the main branch and then using this branch. Then I checked the RGB values of the brightest spot on the carbon atom. This branch is still darker.

I agree that this can be addressed in a separate PR.

I see, I guess the difference is because I used a different actor type for the atoms. I've increased atoms and bond opacity to 0.4, so unpicked bonds will be slightly more opaque.

Protos

This branch

I've also removed the unused code for the surfaces, we can always get this back from the git history if we need it again.

[oerc0122]

Looks good, just one instance of == where there should be is then I'm happy.