Merge pull request #506 from phaazon/fix/483-webgl-index-buffer-corru…

…ption

Fix buffer target corruption in WebGL2 backend.

examples/common/src/funtest_483_indices_mut_corruption.rs

@@ -0,0 +1,45 @@
+use crate::{shared::Vertex, Example, InputAction, LoopFeedback, PlatformServices};
+use luminance_front::{context::GraphicsContext, framebuffer::Framebuffer, texture::Dim2, Backend};
+use std::ops::Deref as _;
+
+pub struct LocalExample;
+
+impl Example for LocalExample {
+  fn bootstrap(
+    _: &mut impl PlatformServices,
+    context: &mut impl GraphicsContext<Backend = Backend>,
+  ) -> Self {
+    let vertices = [
+      Vertex::new([1., 2.].into(), [1., 1., 1.].into()),
+      Vertex::new([-1., 2.].into(), [1., 0., 1.].into()),
+      Vertex::new([1., -2.].into(), [1., 1., 0.].into()),
+    ];
+    let mut tess = context
+      .new_tess()
+      .set_vertices(&vertices[..])
+      .set_indices([0u8, 1, 2])
+      .set_mode(luminance_front::tess::Mode::Point)
+      .build()
+      .expect("tessellation");
+
+    {
+      let mut slice = tess.indices_mut().expect("sliced indices");
+      log::info!("slice before mutation is: {:?}", slice.deref());
+
+      slice[1] = 2;
+      log::info!("slice after mutation is:  {:?}", slice.deref());
+    }
+
+    LocalExample
+  }
+
+  fn render_frame(
+    self,
+    _: f32,
+    _: Framebuffer<Dim2, (), ()>,
+    _: impl Iterator<Item = InputAction>,
+    _: &mut impl GraphicsContext<Backend = Backend>,
+  ) -> LoopFeedback<Self> {
+    LoopFeedback::Exit
+  }
+}

examples/common/src/lib.rs

@@ -49,6 +49,8 @@ pub mod vertex_instancing;
 #[cfg(feature = "funtest")]
 pub mod funtest_360_manually_drop_framebuffer;
 #[cfg(feature = "funtest")]
+pub mod funtest_483_indices_mut_corruption;
+#[cfg(feature = "funtest")]
 pub mod funtest_flatten_slice;
 #[cfg(all(feature = "funtest", feature = "funtest-gl33-f64-uniform"))]
 pub mod funtest_gl33_f64_uniform;

examples/desktop/src/main.rs

@@ -210,6 +210,7 @@ examples! {
   "funtest-360-manually-drop-framebuffer", funtest_360_manually_drop_framebuffer,
   "funtest-flatten-slice", funtest_flatten_slice,
   "funtest-pixel-array-encoding", funtest_pixel_array_encoding,
+  "funtest-483-indices-mut-corruption", funtest_483_indices_mut_corruption,
 }
 
 fn main() {

examples/web/src/lib.rs

@@ -282,6 +282,7 @@ examples! {
   "funtest-360-manually-drop-framebuffer", funtest_360_manually_drop_framebuffer,
   "funtest-flatten-slice", funtest_flatten_slice,
   "funtest-pixel-array-encoding", funtest_pixel_array_encoding,
+  "funtest-483-indices-mut-corruption", funtest_483_indices_mut_corruption,
 }
 
 #[wasm_bindgen]

luminance-webgl/CHANGELOG.md

@@ -11,6 +11,8 @@ compatible with as many crates as possible. In that case, you want `cargo update
 
 <!-- vim-markdown-toc GFM -->
 
+* [0.4 + 1](#04--1)
+  * [Patch changes](#patch-changes)
 * [0.4](#04)
 * [0.3.2](#032)
 * [0.3.1](#031)
@@ -25,6 +27,15 @@ compatible with as many crates as possible. In that case, you want `cargo update
 
 <!-- vim-markdown-toc -->
 
+# 0.4 + 1
+
+> ?
+
+## Patch changes
+
+- Fix buffer kind not correctly being used (i.e. mixing vertex and index buffers is not possible, for instance). This
+  fix was the premise of the full fix, as a redesign of luminance’s buffer interface was needed to fully fix the problem.
+
 # 0.4
 
 > Apr 25, 2021

luminance-webgl/src/webgl2/buffer.rs

@@ -20,6 +20,11 @@ use luminance::buffer::BufferError;
 #[derive(Clone, Debug)]
 struct BufferWrapper {
   handle: WebGlBuffer,
+  /// Target the buffer was created with; WebGL2 doesn’t play well with rebinding a buffer to a different target (unlike
+  /// OpenGL, in which that optimization is handy).
+  ///
+  /// See https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext/bindBuffer#exceptions for further details
+  target: u32,
   state: Rc<RefCell<WebGL2State>>,
 }
 
@@ -61,7 +66,7 @@ impl<T> Buffer<T> {
       .create_buffer()
       .ok_or_else(|| BufferError::cannot_create())?;
 
-    Self::bind(&mut state, &handle, target)?;
+    bind_buffer(&mut state, &handle, target, Bind::Forced)?;
 
     let bytes = mem::size_of::<T>() * len;
     state
@@ -70,6 +75,7 @@ impl<T> Buffer<T> {
 
     let gl_buf = BufferWrapper {
       handle,
+      target,
       state: webgl2.state.clone(),
     };
 
@@ -88,7 +94,7 @@ impl<T> Buffer<T> {
       .create_buffer()
       .ok_or_else(|| BufferError::cannot_create())?;
 
-    Self::bind(&mut state, &handle, target)?;
+    bind_buffer(&mut state, &handle, target, Bind::Forced)?;
 
     let bytes = mem::size_of::<T>() * len;
     let data = unsafe { slice::from_raw_parts(vec.as_ptr() as *const _, bytes) };
@@ -98,29 +104,13 @@ impl<T> Buffer<T> {
 
     let gl_buf = BufferWrapper {
       handle,
+      target,
       state: webgl2.state.clone(),
     };
 
     Ok(Buffer { gl_buf, buf: vec })
   }
 
-  /// Bind a buffer to a given state regarding the input target.
-  fn bind(state: &mut WebGL2State, handle: &WebGlBuffer, target: u32) -> Result<(), BufferError> {
-    // depending on the buffer target, we are not going to bind it the same way, as the first bind
-    // is actually meaningful in WebGL2
-    match target {
-      WebGl2RenderingContext::ARRAY_BUFFER => state.bind_array_buffer(Some(handle), Bind::Forced),
-      WebGl2RenderingContext::ELEMENT_ARRAY_BUFFER => {
-        state.bind_element_array_buffer(Some(handle), Bind::Forced)
-      }
-
-      // a bit opaque but should never happen
-      _ => return Err(BufferError::CannotCreate),
-    }
-
-    Ok(())
-  }
-
   pub(crate) fn handle(&self) -> &WebGlBuffer {
     &self.gl_buf.handle
   }
@@ -177,12 +167,13 @@ where
       let mut state = buffer.gl_buf.state.borrow_mut();
       let bytes = mem::size_of::<T>() * buffer_len;
       update_webgl_buffer(
+        buffer.gl_buf.target,
         &mut state,
         &buffer.gl_buf.handle,
         buffer.buf.as_ptr() as *const u8,
         bytes,
         i,
-      );
+      )?;
 
       Ok(())
     }
@@ -210,12 +201,13 @@ where
     let mut state = buffer.gl_buf.state.borrow_mut();
     let bytes = mem::size_of::<T>() * buffer_len;
     update_webgl_buffer(
+      buffer.gl_buf.target,
       &mut state,
       &buffer.gl_buf.handle,
       buffer.buf.as_ptr() as *const u8,
       bytes,
       0,
-    );
+    )?;
 
     Ok(())
   }
@@ -229,12 +221,13 @@ where
     let mut state = buffer.gl_buf.state.borrow_mut();
     let bytes = buffer.buf.len() * mem::size_of::<T>();
     update_webgl_buffer(
+      buffer.gl_buf.target,
       &mut state,
       &buffer.gl_buf.handle,
       buffer.buf.as_ptr() as *const u8,
       bytes,
       0,
-    );
+    )?;
 
     Ok(())
   }
@@ -280,6 +273,7 @@ impl<T> Deref for BufferSlice<T> {
 /// When a buffer is mapped, we are the only owner of it. We can then read or write from/to the
 /// mapped buffer, and then update the GPU buffer on the [`Drop`] implementation.
 pub struct BufferSliceMutWrapper {
+  target: u32,
   handle: WebGlBuffer,
   ptr: *mut u8,
   bytes: usize,
@@ -289,7 +283,14 @@ pub struct BufferSliceMutWrapper {
 impl Drop for BufferSliceMutWrapper {
   fn drop(&mut self) {
     let mut state = self.state.borrow_mut();
-    update_webgl_buffer(&mut state, &self.handle, self.ptr, self.bytes, 0);
+    let _ = update_webgl_buffer(
+      self.target,
+      &mut state,
+      &self.handle,
+      self.ptr,
+      self.bytes,
+      0,
+    );
   }
 }
 
@@ -355,6 +356,7 @@ where
     buffer: &mut Self::BufferRepr,
   ) -> Result<Self::SliceMutRepr, BufferError> {
     let raw = BufferSliceMutWrapper {
+      target: buffer.gl_buf.target,
       handle: buffer.gl_buf.handle.clone(),
       ptr: buffer.buf.as_mut_ptr() as *mut u8,
       bytes: buffer.buf.len() * mem::size_of::<T>(),
@@ -369,23 +371,43 @@ where
   }
 }
 
+/// Bind a buffer to a given state regarding the input target.
+fn bind_buffer(
+  state: &mut WebGL2State,
+  handle: &WebGlBuffer,
+  target: u32,
+  bind_mode: Bind,
+) -> Result<(), BufferError> {
+  // depending on the buffer target, we are not going to bind it the same way, as the first bind
+  // is actually meaningful in WebGL2
+  match target {
+    WebGl2RenderingContext::ARRAY_BUFFER => state.bind_array_buffer(Some(handle), bind_mode),
+    WebGl2RenderingContext::ELEMENT_ARRAY_BUFFER => {
+      state.bind_element_array_buffer(Some(handle), bind_mode)
+    }
+
+    // a bit opaque but should never happen
+    _ => return Err(BufferError::CannotCreate),
+  }
+
+  Ok(())
+}
+
 /// Update a WebGL buffer by copying an input slice.
 fn update_webgl_buffer(
+  target: u32,
   state: &mut WebGL2State,
-  gl_buf: &WebGlBuffer,
+  handle: &WebGlBuffer,
   data: *const u8,
   bytes: usize,
   offset: usize,
-) {
-  state.bind_array_buffer(Some(gl_buf), Bind::Cached);
+) -> Result<(), BufferError> {
+  bind_buffer(state, handle, target, Bind::Cached)?;
 
   let data = unsafe { slice::from_raw_parts(data as _, bytes) };
   state
     .ctx
-    .buffer_sub_data_with_i32_and_u8_array_and_src_offset(
-      WebGl2RenderingContext::ARRAY_BUFFER,
-      offset as _,
-      data,
-      0,
-    );
+    .buffer_sub_data_with_i32_and_u8_array_and_src_offset(target, offset as _, data, 0);
+
+  Ok(())
 }