pygfx icon indicating copy to clipboard operation
pygfx copied to clipboard

Changing `get_render_info` upon camera updates

Open cassiersg opened this issue 10 months ago • 6 comments

I am trying to change the vertex indices for a shader depending on the camera position (as an optimization, to reduce the number of vertex shader invocations.

My current attempt is to access shared.uniform_buffer.data["cam_transform"] and shared.uniform_buffer.data["cam_transform"] in my shader's get_render_info. However, this doesn't work: get_render_info is not called when I update the camera. As far as I can tell, the root cause of this issue is that the changes to self._shared.uniform_buffer.data in WgpuRender._update_stdinfo_buffer are not tracked in the Tracker mechanism.

  • Could we fix this issue by changing WgpuRender._update_stdinfo_buffer to somehow give access to the camera matrix through Shared in a way that can be tracked?
  • Is there a better approach for what I'm trying to do? (I found a workaround by explicitly by the rendered object's geometry.positions.draw_range in my top-level animate function and using this value in get_render_info, but this top-level code feels like a workaround for something that should be handled within the rendering engine/shaders).

Context (feel free to ignore). I am interested in plotting very long time series (e.g., 10**8 points) and allow interactive viewing of the series (i.e., a 2d line plot where the x-coordinates of the points is np.arange(N), and the y-coordiantes is the data). pygfx looks like an amazing tool for this (thank you so much for building it!), but as expected simply using Line doesn't give a good performance. My current plan is:

  1. For high zoom levels (e.g. zoom such that at most a few thousands data points are visible): adapt the Line shader to only compute the vertex shader to the points near the viewport along the x-axis (I encountered the present issue while making a "clean" implementation of this).
  2. For lower zoom level (possibly millions of data points in the viewport): have pre-computed downsampled (with max/min pooling) of the data, fill a polygon from the min/max downsampled data (multiple levels of downsampling according to the zoom level).
  3. Handle numerical accuracy issues: when zooming into the end of the time series, the numerical accuracy of float32 might not be sufficient for the x-axis. I think that the simplest solution is to use a camera that is not directly tied to the renderer, but instead directly control the behavior of a new set of "camera-aware" objects. In this way, we can keep all shaders using float32 without numerical issues, while using float64 for camera position.

If any part of this work may be interesting to you, I'd be happy to work on upstreaming it.

Edit: a patch that seems to fix the issue. get_render_info can now rely on shared.uniform_data:

diff --git a/pygfx/renderers/wgpu/engine/renderer.py b/pygfx/renderers/wgpu/engine/renderer.py
index 874236d..9d2600a 100644
--- a/pygfx/renderers/wgpu/engine/renderer.py
+++ b/pygfx/renderers/wgpu/engine/renderer.py
@@ -727,17 +727,20 @@ class WgpuRenderer(RootEventHandler, Renderer):
         self, camera: Camera, physical_size, logical_size, ndc_offset
     ):
         # Update the stdinfo buffer's data
-        stdinfo_data = self._shared.uniform_buffer.data
-        stdinfo_data["cam_transform"] = camera.world.inverse_matrix.T
-        stdinfo_data["cam_transform_inv"] = camera.world.matrix.T
-        stdinfo_data["projection_transform"] = camera.projection_matrix.T
-        stdinfo_data["projection_transform_inv"] = camera.projection_matrix_inverse.T
-        # stdinfo_data["ndc_to_world"].flat = la.mat_inverse(stdinfo_data["cam_transform"] @ stdinfo_data["projection_transform"])
-        stdinfo_data["ndc_offset"] = ndc_offset
-        stdinfo_data["physical_size"] = physical_size
-        stdinfo_data["logical_size"] = logical_size
-        # Upload to GPU
-        self._shared.uniform_buffer.update_full()
+        stdinfo_data = self._shared.uniform_data
+        new_stdinfo_data = stdinfo_data.copy()
+        new_stdinfo_data["cam_transform"] = camera.world.inverse_matrix.T
+        new_stdinfo_data["cam_transform_inv"] = camera.world.matrix.T
+        new_stdinfo_data["projection_transform"] = camera.projection_matrix.T
+        new_stdinfo_data["projection_transform_inv"] = (
+            camera.projection_matrix_inverse.T
+        )
+        # new_stdinfo_data["ndc_to_world"].flat = la.mat_inverse(stdinfo_data["cam_transform"] @ stdinfo_data["projection_transform"])
+        new_stdinfo_data["ndc_offset"] = ndc_offset
+        new_stdinfo_data["physical_size"] = physical_size
+        new_stdinfo_data["logical_size"] = logical_size
+        if new_stdinfo_data.tobytes() != stdinfo_data.tobytes():
+            self._shared.uniform_data = new_stdinfo_data
 
     # Picking
 
diff --git a/pygfx/renderers/wgpu/engine/shared.py b/pygfx/renderers/wgpu/engine/shared.py
index 822b6dd..73e6d9f 100644
--- a/pygfx/renderers/wgpu/engine/shared.py
+++ b/pygfx/renderers/wgpu/engine/shared.py
@@ -93,9 +93,8 @@ class Shared(Trackable):
         # Create a uniform buffer for std info
         # Stored on _store so if we'd ever swap it out for another buffer,
         # the pipeline automatically update.
-        self._store.uniform_buffer = Buffer(
-            array_from_shadertype(stdinfo_uniform_type), force_contiguous=True
-        )
+        self._store.uniform_data = array_from_shadertype(stdinfo_uniform_type)
+        self._store.uniform_buffer = Buffer(self.uniform_data, force_contiguous=True)
         self._store.uniform_buffer._wgpu_usage |= wgpu.BufferUsage.UNIFORM
 
         # Init glyph atlas texture
@@ -126,6 +125,22 @@ class Shared(Trackable):
         """The shared WGPU device object."""
         return self._device
 
+    @property
+    def uniform_data(self):
+        """The shared uniform data in which the renderer puts
+        information about the canvas and camera (same content as uniform_buffer).
+        """
+        return self._store.uniform_data
+
+    @uniform_data.setter
+    def uniform_data(self, value):
+        """The shared uniform data in which the renderer puts
+        information about the canvas and camera (same content as uniform_buffer).
+        """
+        value.flags.writeable = False
+        self._store.uniform_data = value
+        self._store.uniform_buffer.set_data(value)
+
     @property
     def uniform_buffer(self):
         """The shared uniform buffer in which the renderer puts

cassiersg avatar Apr 26 '25 15:04 cassiersg

The point of shared.uniform_buffer (or u_stdinfo in the shader), or any uniform buffer really, is that the values can change without resulting in a change for the shader. Making it trigger a "trackable change" on each draw as you suggest in the diff would cause a performance penalty for all objects 😅

What you need is a little code to run at each draw to match the draw_range with the camera position. You can indeed do that in your animate function somehow, but I understand that does not feel very elegant. For this purpose there is WorldObject._update_object(). I suspect you already subclassed the Line object. Implement _update_object() on your subclass (don't forget to call super()._update_object()), to do the thing.

almarklein avatar May 06 '25 08:05 almarklein

Thanks for the info.

If I understand correctly, _update_object is called before drawing each frame and before get_render_info. The solution is therefore to trigger a "trackable change" in _update_object so that get_render_info gets actually called.

Further, to avoid unnecessary computation _update_object should check itself whether the camera changed.

However, I don't see how to access the camera from _update_object in a non-hacky way? (Further, it seems to me that WorldObject should not care too much about the camera.) Do you have any suggestion on how to do this?

As an alternative, maybe putting the camera and view size as separate trackable properties on Shared would be better? That would provide a way to access these values in the shader.

Further, the only value I want do change in the render info is the "indices", which is very cheap, as far as I understand (unlike the other render infos). Would it make sense to change the shader interface to have a dedicated get_indices method?

cassiersg avatar May 06 '25 09:05 cassiersg

If I understand correctly, _update_object is called before drawing each frame and before get_render_info. The solution is therefore to trigger a "trackable change" in _update_object so that get_render_info gets actually called.

Yeah. Setting geometry.positions.draw_range should be enough. The default line shader's get_render_info sets the indices based on it.

Further, to avoid unnecessary computation _update_object should check itself whether the camera changed.

The tracking system detects whether a value actually changes, so if the camera has not changes, it won't trigger get_render_info. That said, you may be able to avoid some matrix operations if the camera matrix has not changed.

However, I don't see how to access the camera from _update_object in a non-hacky way? (Further, it seems to me that WorldObject should not care too much about the camera.) Do you have any suggestion on how to do this?

Yeah, in general a world object should not care about a camera, but your object does 😉. So making that dependency explicit is not that hacky.

An alternative would be to use the shared.uniform_buffer.data["cam_transform"] but _update_object is called before _update_stdinfo_buffer, so probably not a good idea, unless we explicitly change this order in the renderer 🤔

Another possibility is to pass args to _update_object allowing it to access the current camera. But this is tricky; we don't want to promote that, since ideally an object does not depend on the camera.

As an alternative, maybe putting the camera and view size as separate trackable properties on Shared would be better? That would provide a way to access these values in the shader.

We don't want any camera's to be global. There can be multiple canvases, each with different cameras. The shared.uniform_buffer is a data structure that's re-used by all renderers; its contents are only valid for a specific camera during a render.render().

Further, the only value I want do change in the render info is the "indices", which is very cheap, as far as I understand (unlike the other render infos). Would it make sense to change the shader interface to have a dedicated get_indices method?

In #1002 this basically happens (because the render mask stuff is removed).

almarklein avatar May 06 '25 09:05 almarklein

Yeah, in general a world object should not care about a camera, but your object does 😉. So making that dependency explicit is not that hacky.

Semantically, my object is close to "just a line", but it dynamically adjusts the set of vertices rendered depending on the camera as an optimization (see https://github.com/fastplotlib/fastplotlib/discussions/811 for background and https://github.com/cassiersg/scaviz for my code). Therefore, it somehow "feels right" to leave this handling to the shader class and not to the object itself. It is also more convenient from an API point of view since we don't have to pass the camera around. Ideally, we indeed don't want a global camera for the object, we just want to compute the parameters at render time, when we know the camera transform.

As I extended my code, I now need to update a uniform buffer depending on the camera parameter. Again, the "right place" for creating this buffer seems to be the shader class, since this buffer is not tied to the object itself, but rather liked to its rendering. Once created, only the buffer's content needs to be updated, we shouldn't call get_bindings again. This makes me think that it would be great to have an update function on the shader (e.g. _update_buffers), similar to _update_object but taking the shared global as an argument.

That being said, your proposal of working with the camera stored in the object and using _update_object would indeed work, at the cost of a less ergonomic API.

cassiersg avatar May 07 '25 14:05 cassiersg

The shader class can actually implement a base function: def bake_function(self, wobject, camera, logical_size): which would serve your purpose very well. Apart from implementing it, you must set the shader's class atrribute needs_bake_function = False.

However, I believe the bake function will be called after the renderer has updated the shader objects. So if yoy set draw_range in the bake function, it will trigger get_render_info but only in the next draw.

Thinking about this now, with the upcoming change that will reduce get_render_info to just producing indices, I think it would worth considering combining this with the bake function: a function of the shader class that will be called on every draw, that returns the indices to draw, and acts as a point where stuff can be baked.

almarklein avatar May 08 '25 09:05 almarklein

Thanks!

Thinking about this now, with the upcoming change that will reduce get_render_info to just producing indices, I think it would worth considering combining this with the bake function: a function of the shader class that will be called on every draw, that returns the indices to draw, and acts as a point where stuff can be baked.

This sounds great.

cassiersg avatar May 08 '25 10:05 cassiersg