Various optimizations related to camera linking and mesh normals

[djhoese]

See #2530

CC @JintaoLee-Roger

This is a collection of optimizations uncovered in #2530 debugging by me or @JintaoLee-Roger. The main ideas are to not update camera properties unless the value has actually changed. This prevents expensive transform-updating methods from being called unnecessarily. This is especially noticeable when a single camera move updates 4+ properties of a linked property and each one tries to report that the view/transform has changed. It does this in two ways:

1. Don't execute a transform update if we're updating properties
2. Don't update a property (and signal a view change) if the property value is the same

This PR also adds some checks in the mesh light filter to see if vertex normals have actually changed before submitting/uploading the normals to the GPU. It also enables the use of a previously unused (yet defined for some reason?) MeshData._vertex_normal_indexed_by_faces. This allows the check in the light filter to actually work with a simple identity (is) check.

TODO:

• Testing of performance by @JintaoLee-Roger
• Adding tests for at least the normals caching in the meshdata.

[JintaoLee-Roger]

Modifications in two ways

1. Don't execute a transform update if we're updating properties

The hack comments should do some changes,
move if self._setting_state from _set_scene_transform function to view_changed function.

As follows:

diff --git a/vispy/scene/cameras/base_camera.py b/vispy/scene/cameras/base_camera.py
index c59f340d..882b6bfc 100644
--- a/vispy/scene/cameras/base_camera.py
+++ b/vispy/scene/cameras/base_camera.py
@@ -62,6 +62,7 @@ class BaseCamera(Node):
         # Linked cameras
         self._linked_cameras = {}
         self._linked_cameras_no_update = None
+        self._setting_state = False

         # Variables related to transforms
         self.transform = NullTransform()
@@ -356,6 +357,7 @@ class BaseCamera(Node):
         """
         state = state or {}
         state.update(kwargs)
+        self._setting_state = True

         # In first pass, process tuple keys which select subproperties. This
         # is an undocumented feature used for selective linking of camera state.
@@ -383,6 +385,8 @@ class BaseCamera(Node):
             if key not in self._state_props:
                 raise KeyError('Not a valid camera state property %r' % key)
             setattr(self, key, val)
+        self._setting_state = False
+        self.view_changed()

     def link(self, camera, props=None, axis=None):
         """Link this camera with another camera of the same type
@@ +443,8 @@ class BaseCamera(Node):

    def view_changed(self):
        """Called when this camera is changes its view. Also called
        when its associated with a viewbox.
        """
+       if self._setting_state:
+           return
        if self._resetting:
            return  # don't update anything while resetting (are in set_range)

2. Don't update a property (and signal a view change) if the property value is the same

diff --git a/vispy/scene/cameras/base_camera.py b/vispy/scene/cameras/base_camera.py
--- a/vispy/scene/cameras/base_camera.py
+++ b/vispy/scene/cameras/base_camera.py

@@ +377, +386 @@ class BaseCamera(Node):

        for key in list(state.keys()):
            if isinstance(key, tuple):
                key1 = key[0]
                if key1 not in state:
                    root_prop = getattr(self, key1)
                    # We make copies by passing the old object to the type's
                    # constructor. This needs to be supported as is the case in
                    # e.g. the geometry.Rect class.
                    state[key1] = root_prop.__class__(root_prop)
-              nested_setattr(state[key1], key[1:], state[key])
+              if nested_getattr(state[key1], key[1:]) != state[key]:
+                   nested_setattr(state[key1], key[1:], state[key])

        # In second pass, assign the new root properties.
        for key, val in state.items():
            if isinstance(key, tuple):
                continue
            if key not in self._state_props:
                raise KeyError('Not a valid camera state property %r' % key)
-           setattr(self, key, val)
+           if getattr(self, key) != val:
+               setattr(self, key, val)

Both of these methods seem to have similar performance; either one is fine. However, as for which one is more elegant, I'm not an expert in this.

[djhoese]

For 1, I wasn't sure if I liked it going it view_changed(). Did that make a difference in performance for you? I was nervous that view_changed may need to do other things besides updating the transform in some cases so didn't want to stop it...but I'm not sure I have any evidence or strong reasoning for doing it where/how I did.

For 2, the idea is that setting state may not be the only way people are setting these properties. It could be from the user (keyboard or user interface input) or part of some timer trigger. So even though it is more work I think it is more useful in the long run to have these checks per property.

[djhoese]

Most importantly I should ask, @JintaoLee-Roger do these changes make your application usable?

[JintaoLee-Roger]

Both of these two hacks satisfy my needs.

[djhoese]

Both of these methods seem to have similar performance; either one is fine. However, as for which one is more elegant, I'm not an expert in this.

So when you said the above, you mean that either this "debouncing" of the set state (the setting_state change) or the equality check give you decent performance. Ok. We'll see what @brisvag and other maintainers think of these changes when they get a chance to review.

[brisvag]

Looks good to me! Didn't test it myself, but the code looks ok.