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

the mouse dragging inside the canvas it is not responsive when using 'Share Camera Views' #2530

Open
JintaoLee-Roger opened this issue Sep 22, 2023 · 17 comments

Comments

@JintaoLee-Roger
Copy link

Hi!

I referred to examples Share Camera Views and Shading a Mesh to display multiple meshes and share camera parameters. However, I noticed that when dragging the mouse, the mouse dragging is smooth only in the first canvas, while it feels very sluggish in the other canvases. How can I resolve this issue?

I need to emphasize that I must use 'on_transform_change' to ensure that the mesh has proper lighting when dragged.

An example:

import numpy as np
from vispy.scene.visuals import SurfacePlot
from vispy import app, scene
from vispy.visuals.filters import ShadingFilter
from vispy.scene.cameras import TurntableCamera

x1, y1 = np.meshgrid(np.linspace(-5, 100, 300), np.linspace(-5, 100, 300))
z1 = (x1 / 20)**3 + (y1 / 20)**3

x2, y2 = np.meshgrid(np.linspace(-5, 80, 256), np.linspace(-5, 80, 256))
z2 = (x2 / 20)**3 + (y2 / 20)**3 + 100

canvas = scene.SceneCanvas(keys='interactive', bgcolor='white')

vb1 = scene.widgets.ViewBox(parent=canvas.scene)
vb2 = scene.widgets.ViewBox(parent=canvas.scene)
vb3 = scene.widgets.ViewBox(parent=canvas.scene)
vb4 = scene.widgets.ViewBox(parent=canvas.scene)

grid = canvas.central_widget.add_grid()
grid.padding = 6
grid.add_widget(vb1, 0, 0)
grid.add_widget(vb2, 0, 1)
grid.add_widget(vb3, 1, 0)
grid.add_widget(vb4, 1, 1)

vb1.camera = TurntableCamera(fov=45, azimuth=0, elevation=0)
vb2.camera = TurntableCamera(fov=45, azimuth=0, elevation=0)
vb3.camera = TurntableCamera(fov=45, azimuth=0, elevation=0)
vb4.camera = TurntableCamera(fov=45, azimuth=0, elevation=0)

vb1sf1 = SurfacePlot(x1, y1, z1, shading=None, parent=vb1.scene)
vb1sf2 = SurfacePlot(x2, y2, z2, shading=None, parent=vb1.scene)
shading1vb1 = ShadingFilter(shininess=200)
shading2vb1 = ShadingFilter(shininess=200)
vb1sf1.attach(shading1vb1)
vb1sf2.attach(shading2vb1)

vb2sf1 = SurfacePlot(x1, y1, z1, shading=None, parent=vb2.scene)
vb2sf2 = SurfacePlot(x2, y2, z2, shading=None, parent=vb2.scene)
shading1vb2 = ShadingFilter(shininess=200)
shading2vb2 = ShadingFilter(shininess=200)
vb2sf1.attach(shading1vb2)
vb2sf2.attach(shading2vb2)

vb3sf1 = SurfacePlot(x1, y1, z1, shading=None, parent=vb3.scene)
vb3sf2 = SurfacePlot(x2, y2, z2, shading=None, parent=vb3.scene)
shading1vb3 = ShadingFilter(shininess=200)
shading2vb3 = ShadingFilter(shininess=200)
vb3sf1.attach(shading1vb3)
vb3sf2.attach(shading2vb3)

vb4sf1 = SurfacePlot(x1, y1, z1, shading=None, parent=vb4.scene)
vb4sf2 = SurfacePlot(x2, y2, z2, shading=None, parent=vb4.scene)
shading1vb4 = ShadingFilter(shininess=200)
shading2vb4 = ShadingFilter(shininess=200)
vb4sf1.attach(shading1vb4)
vb4sf2.attach(shading2vb4)


def attach_headlight(view, shadings):
    light_dir = (0, 1, 0, 0)
    for shading in shadings:
        shading.light_dir = light_dir[:3]
    initial_light_dir = view.camera.transform.imap(light_dir)
    view.camera.azimuth = 0
    view.camera.elevation = 0
    view.camera.set_range()

    @view.scene.transform.changed.connect
    def on_transform_change(event):
        transform = view.camera.transform
        for shading in shadings:
            shading.light_dir = transform.map(initial_light_dir)[:3]


# Only need attach one camera, as cameras are linked
# but all shadingfilters must be attached
attach_headlight(vb1, [
    shading1vb1, shading2vb1, shading1vb2, shading2vb2, shading1vb3,
    shading2vb3, shading1vb4, shading2vb4
])

vb1.camera.link(vb2.camera)
vb1.camera.link(vb3.camera)
vb1.camera.link(vb4.camera)

canvas.show()

if __name__ == "__main__":
    app.run()

Is my usage correct? Is there a way to solve this problem?

Thanks!

@JintaoLee-Roger JintaoLee-Roger changed the title the mouse dragging inside the canvas behind it is not responsive when using 'Share Camera Views' the mouse dragging inside the canvas it is not responsive when using 'Share Camera Views' Sep 22, 2023
@djhoese
Copy link
Member

djhoese commented Sep 22, 2023

Overall I think this is a bug or at least something that needs to be optimized. It is a limitation of the camera linking code done here:

for cam in self._linked_cameras:
if cam is self._linked_cameras_no_update:
continue
try:
cam._linked_cameras_no_update = self
linked_props = self._linked_cameras[cam]
cam.set_state(self.get_state(linked_props))
finally:
cam._linked_cameras_no_update = None

The code is structured in a way so it stops the linked camera from updated the camera being moved/manipulated. So if camera 1 is moved then it's properties are updated and then those properties are assigned to camera 2. For each property camera 2's transform is changed and it tries to update its linked cameras. That's for every property that is copied from camera 1 to camera 2. Overall this isn't that bad in the camera 1 case (still not great) because camera 2 only has one linked camera (camera 1) and it is told not to update it.

But in the case where you move camera 2, you now have camera 1 trying to update every linked camera for every property that is getting updated.

Theoretically this should be fixable. Let's see...

@djhoese
Copy link
Member

djhoese commented Sep 22, 2023

Hm I thought this would help but it really doesn't do much at all:

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
@@ -508,6 +512,8 @@ class BaseCamera(Node):

     def _set_scene_transform(self, tr):
         """Called by subclasses to configure the viewbox scene transform."""
+        if self._setting_state:
+            return
         # todo: check whether transform has changed, connect to
         # transform.changed event
         pre_tr = self.pre_transform

I can see with print messages that it is doing less work/updates, but it is still very slow.

I should also note that this number of visuals with this many filters seems to really kill performance (https://vispy.org/faq.html#why-is-my-visualization-slower-when-i-add-more-visual-objects).

I'm not really sure where to go from here.

@JintaoLee-Roger
Copy link
Author

I believe the issue should be related to on_transform_change, i.e., the ShadingFilters, because once I comment on_transform_change all camera movements work fine. However, when I add on_transform_change it makes camera movements sluggish except for the first camera. So, should optimize the code of filters?

@djhoese
Copy link
Member

djhoese commented Sep 22, 2023

When I did:

attach_headlight(vb1, [shading1vb1, shading2vb1])
attach_headlight(vb2, [shading1vb2, shading2vb2])
attach_headlight(vb3, [shading1vb3, shading2vb3])
attach_headlight(vb4, [shading1vb4, shading2vb4])

They all perform equally bad I think.

@djhoese
Copy link
Member

djhoese commented Sep 22, 2023

As a hack I did this:

diff --git a/vispy/visuals/filters/mesh.py b/vispy/visuals/filters/mesh.py
index eb4e3326..e48c335d 100644
--- a/vispy/visuals/filters/mesh.py
+++ b/vispy/visuals/filters/mesh.py
@@ -549,8 +549,10 @@ class ShadingFilter(Filter):
             1 if self._enabled and self._shading is not None else 0
         )

-        normals = self._visual.mesh_data.get_vertex_normals(indexed='faces')
-        self._normals.set_data(normals, convert=True)
+        if not hasattr(self, "_normals_cache"):
+            normals = self._visual.mesh_data.get_vertex_normals(indexed='faces')
+            self._normals_cache = normals
+        self._normals.set_data(self._normals_cache, convert=True)

     def on_mesh_data_updated(self, event):
         self._update_data()

And that seems to be decently fast. This basically tells us that computing the normals and/or uploading them to the GPU (.set_data) is the slow part.

@JintaoLee-Roger
Copy link
Author

Yes, the speed of the cameras seems to have improved now, although there is still some difference in responsiveness between the rear cameras and the first one.

@djhoese
Copy link
Member

djhoese commented Sep 22, 2023

Do you have both my hacks in your local environment? The camera one and the light filter? I'm not noticing anything too bad.

@JintaoLee-Roger
Copy link
Author

Well, it depends on the number of meshes. As the number of meshes increases, the issue persists with smooth movement for the first camera and lag for the other cameras.

@djhoese
Copy link
Member

djhoese commented Sep 25, 2023

Do you have a good mini-example that has a lot of meshes that show this? Keep in mind that it is expected that having a lot of Visuals slows down the visualization. The fact that you have separate meshes for each element and then have to repeat that across the 4 views means you're going to hit performance issues pretty quickly.

@JintaoLee-Roger
Copy link
Author

Just Increase the number points of the surfaces:

x1, y1 = np.meshgrid(np.linspace(0, 1000, 1000), np.linspace(0, 1000, 1000))
z1 = (x1 / 100)**3 + (y1 / 100)**3

x2, y2 = np.meshgrid(np.linspace(0, 800, 800), np.linspace(0, 800, 800))
z2 = (x2 / 100)**3 + (y2 / 100)**3 + 100

This will reproduce the initial problem. (I need to display surfaces in three-dimensional seismic data. In three-dimensional data, a size of (x, y) = (1000, 1000) is quite common.)

@JintaoLee-Roger
Copy link
Author

I noticed that when I change the state of camera 1, the cam.set_state(self.get_state(linked_props)) line in BaseCamera is called three times in total, and on_transform_change is called once. However, when I change the state of the other cameras, the cam.set_state(self.get_state(linked_props)) is called 13 times, and on_transform_change is called 6 times.

To reproduce:

diff --git a/vispy/visuals/filters/mesh.py b/vispy/visuals/filters/mesh.py
+++ b/vispy/visuals/filters/mesh.py
@@ -62,6 +62,7 @@ class BaseCamera(Node):
# The viewbox for which this camera is active
self._viewbox = None
+ self.name = None

@@ +541,7 @@ class BaseCamera(Node):
try:
    cam._linked_cameras_no_update = self
    linked_props = self._linked_cameras[cam]
    cam.set_state(self.get_state(linked_props))
+    print('changed: ', self.name, cam.name)

In example code:

...
vb1.camera.name = 'c1'
vb2.camera.name = 'c2'
vb3.camera.name = 'c3'
vb4.camera.name = 'c4'
...

...
    @view.scene.transform.changed.connect
    def on_transform_change(event):
        print('call transform')
        ...

...
@canvas.events.key_press.connect
def on_key_press(event):
    if event.key == 's':
        print('\n-----------------------------\n')

    if event.key == 'q':
        vb1.camera.azimuth = 10
        print('\n-----------------------------\n')

    if event.key == 'w':
        vb2.camera.azimuth = 20
        print('\n-----------------------------\n')

    if event.key == 'e':
        vb3.camera.azimuth = 30
        print('\n-----------------------------\n')

    if event.key == 'r':
        vb4.camera.azimuth = 40
        print('\n-----------------------------\n')

Press the keys in the following order: <s>, <q>, <w>, <e>, <r>, I got:

-----------------------------

call transform
changed:  c1 c2
changed:  c1 c3
changed:  c1 c4

-----------------------------

call transform
changed:  c1 c3
changed:  c1 c4
call transform
changed:  c1 c3
changed:  c1 c4
call transform
changed:  c1 c3
changed:  c1 c4
call transform
changed:  c1 c3
changed:  c1 c4
call transform
changed:  c1 c3
changed:  c1 c4
call transform
changed:  c1 c3
changed:  c1 c4
changed:  c2 c1

-----------------------------

call transform
changed:  c1 c2
changed:  c1 c4
call transform
changed:  c1 c2
changed:  c1 c4
call transform
changed:  c1 c2
changed:  c1 c4
call transform
changed:  c1 c2
changed:  c1 c4
call transform
changed:  c1 c2
changed:  c1 c4
call transform
changed:  c1 c2
changed:  c1 c4
changed:  c3 c1

-----------------------------

call transform
changed:  c1 c2
changed:  c1 c3
call transform
changed:  c1 c2
changed:  c1 c3
call transform
changed:  c1 c2
changed:  c1 c3
call transform
changed:  c1 c2
changed:  c1 c3
call transform
changed:  c1 c2
changed:  c1 c3
call transform
changed:  c1 c2
changed:  c1 c3
changed:  c4 c1

-----------------------------

I suspect that the def _set_scene_transform(self, tr): function should be optimized to remove unnecessary calls.

@JintaoLee-Roger
Copy link
Author

I have identified the issue and resolved it. In the BaseCamera's _set_scene_transform function, there are six key values in linked_props, and when cam.set_state(self.get_state(linked_props)) is executed, it calls _set_scene_transform six times, causing six calls to on_transform_change even if only one key has changed, resulting in unnecessary overhead. Therefore, we should only call _set_scene_transform when the value of a key has changed. This way, on_transform_change will only be called once each time the camera is changed.

The specific modification is as follows:

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)

Can this change be included in the new release?

@djhoese
Copy link
Member

djhoese commented Sep 25, 2023

A couple things:

  1. My first hack that I had posted in the mouse dragging inside the canvas it is not responsive when using 'Share Camera Views' #2530 (comment) was supposed to prevent setting the state multiple times. Or rather, it was supposed to set the state but then not actually execute the transform change. Put another way, it was meant to treat all the property updates as one single update. At least in theory.
  2. Correct me if I'm wrong, but your solution basically says "if the new value is the same as the old value, don't set it", right? I think I'd rather this go on each individual property if possible. Either of these solutions though is a definite win so it would be good to have.

@JintaoLee-Roger
Copy link
Author

Well, the first hack doesn't seem to prevent multiple calls to the 'on_transform_change' function (it even called it one extra time), and I don't know why it didn't work, even though it looks correct.

Secondly, I am not aware of the cost of calling the 'view_changed' function (when the values are exactly the same). If its cost is significantly higher than comparing the two values, I think performing a comparison would be more cost-effective (also to handle similar situations).

@JintaoLee-Roger
Copy link
Author

Your first hack intends to change all properties first and finally execute view_changed to update the camera, but it doesn't seem to prevent multiple calls to the on_transform_change function.

For cameras based on the PerspectiveCamera, the default _update_transform function is not used; it's overridden at

def _update_transform(self, event=None):
. In this function, the call to on_transform_change occurs on the line self._update_projection_transform() (
self._update_projection_transform(fx, fy)
) (I don't know why it happens on this line; I've only confirmed it through methods like print). Therefore, what you're doing inside the _set_scene_transform function has no effect.

I don't know how to make modifications based on the principle of your first hack?

@djhoese
Copy link
Member

djhoese commented Sep 25, 2023

Ooohhhh that makes sense. That was something I was concerned about but then forgot throughout the debug process. Ok...interesting....let me take a look. I don't have a lot of time this week to do volunteer vispy work, but I'll see if I can throw together some changes that I have in mind.

@djhoese
Copy link
Member

djhoese commented Sep 25, 2023

See #2532 and let me know what you think.

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

No branches or pull requests

2 participants