From c3e88feb4304d1e2324c2dcabd80b2dd7b37f113 Mon Sep 17 00:00:00 2001 From: Wael El Oraiby Date: Wed, 1 May 2024 20:26:15 -0400 Subject: [PATCH 1/4] Add functions to remove resources and prevent resource leaks for shaders, buffers, pipelines, render passes in GL based renderer Moved resources management from Vec to HashMap Added glDetach --- Cargo.toml | 16 ++++-- examples/index.html | 2 +- js/gl.js | 124 ++++++++++++++++++++++++-------------------- src/graphics.rs | 12 ++++- src/graphics/gl.rs | 115 +++++++++++++++++++++++++--------------- src/native/gl.rs | 1 + 6 files changed, 167 insertions(+), 103 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 186da00d..58ebc815 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,7 +9,7 @@ repository = "https://github.com/not-fl3/miniquad" description = """ Cross-platform window context and rendering library. """ -readme="README.md" +readme = "README.md" exclude = ["examples/"] keywords = ["graphics", "3D", "opengl", "gamedev", "windowing"] categories = ["rendering::graphics-api"] @@ -24,7 +24,17 @@ log-impl = [] libc = "0.2" [target.'cfg(windows)'.dependencies] -winapi = { version = "0.3", features = ["wingdi", "winuser", "libloaderapi", "windef", "shellscalingapi", "errhandlingapi", "windowsx", "winbase", "hidusage"] } +winapi = { version = "0.3", features = [ + "wingdi", + "winuser", + "libloaderapi", + "windef", + "shellscalingapi", + "errhandlingapi", + "windowsx", + "winbase", + "hidusage", +] } [target.'cfg(target_os = "android")'.dependencies] libc = "0.2" @@ -34,5 +44,5 @@ ndk-sys = "0.2" objc = "0.2" [dev-dependencies] -glam = {version = "0.24", features = ["scalar-math"] } +glam = { version = "0.24", features = ["scalar-math"] } quad-rand = "0.1" diff --git a/examples/index.html b/examples/index.html index 4462862c..0ce5717f 100644 --- a/examples/index.html +++ b/examples/index.html @@ -27,4 +27,4 @@ - + \ No newline at end of file diff --git a/js/gl.js b/js/gl.js index 5a67f3f5..5455cbf7 100644 --- a/js/gl.js +++ b/js/gl.js @@ -29,11 +29,11 @@ canvas.requestPointerLock = canvas.requestPointerLock || canvas.mozRequestPointerLock || // pointer lock in any form is not supported on iOS safari // https://developer.mozilla.org/en-US/docs/Web/API/Pointer_Lock_API#browser_compatibility - (function () {}); + (function () { }); document.exitPointerLock = document.exitPointerLock || document.mozExitPointerLock || // pointer lock in any form is not supported on iOS safari - (function () {}); + (function () { }); function assert(flag, message) { if (flag == false) { @@ -562,7 +562,7 @@ function into_sapp_keycode(key_code) { console.log("Unsupported keyboard key: ", key_code) } -function dpi_scale() { +function dpi_scale() { if (high_dpi) { return window.devicePixelRatio || 1.0; } else { @@ -614,7 +614,7 @@ var importObject = { set_emscripten_shader_hack: function (flag) { emscripten_shaders_hack = flag; }, - sapp_set_clipboard: function(ptr, len) { + sapp_set_clipboard: function (ptr, len) { clipboard = UTF8ToString(ptr, len); }, dpi_scale, @@ -666,7 +666,7 @@ var importObject = { gl.texSubImage2D(target, level, xoffset, yoffset, width, height, format, type, pixels ? getArray(pixels, Uint8Array, texture_size(format, width, height)) : null); }, - glReadPixels: function(x, y, width, height, format, type, pixels) { + glReadPixels: function (x, y, width, height, format, type, pixels) { var pixelData = getArray(pixels, Uint8Array, texture_size(format, width, height)); gl.readPixels(x, y, width, height, format, type, pixelData); }, @@ -845,6 +845,11 @@ var importObject = { GL.validateGLObjectID(GL.shaders, shader, 'glAttachShader', 'shader'); gl.attachShader(GL.programs[program], GL.shaders[shader]); }, + glDettachShader: function (program, shader) { + GL.validateGLObjectID(GL.programs, program, 'glDetachShader', 'program'); + GL.validateGLObjectID(GL.shaders, shader, 'glDetachShader', 'shader'); + gl.detachShader(GL.programs[program], GL.shaders[shader]); + }, glLinkProgram: function (program) { GL.validateGLObjectID(GL.programs, program, 'glLinkProgram', 'program'); gl.linkProgram(GL.programs[program]); @@ -956,7 +961,7 @@ var importObject = { array[i] = log.charCodeAt(i); } }, - glGetString: function(id) { + glGetString: function (id) { // getParameter returns "any": it could be GLenum, String or whatever, // depending on the id. var parameter = gl.getParameter(id).toString(); @@ -1006,7 +1011,12 @@ var importObject = { glDrawElementsInstanced: function (mode, count, type, indices, primcount) { gl.drawElementsInstanced(mode, count, type, indices, primcount); }, - glDeleteShader: function (shader) { gl.deleteShader(shader) }, + glDeleteShader: function (shader) { + var id = GL.shaders[shader]; + if (id == null) { return } + gl.deleteShader(id); + GL.shaders[shader] = null + }, glDeleteBuffers: function (n, buffers) { for (var i = 0; i < n; i++) { var id = getArray(buffers + i * 4, Uint32Array, 1)[0]; @@ -1045,45 +1055,45 @@ var importObject = { GL.textures[id] = null; } }, - glGenQueries: function (n, ids) { - _glGenObject(n, ids, 'createQuery', GL.timerQueries, 'glGenQueries'); - }, - glDeleteQueries: function (n, ids) { + glGenQueries: function (n, ids) { + _glGenObject(n, ids, 'createQuery', GL.timerQueries, 'glGenQueries'); + }, + glDeleteQueries: function (n, ids) { for (var i = 0; i < n; i++) { var id = getArray(textures + i * 4, Uint32Array, 1)[0]; var query = GL.timerQueries[id]; if (!query) { - continue; - } + continue; + } gl.deleteQuery(query); query.name = 0; GL.timerQueries[id] = null; } - }, - glBeginQuery: function (target, id) { - GL.validateGLObjectID(GL.timerQueries, id, 'glBeginQuery', 'id'); - gl.beginQuery(target, GL.timerQueries[id]); - }, - glEndQuery: function (target) { - gl.endQuery(target); - }, - glGetQueryObjectiv: function (id, pname, ptr) { - GL.validateGLObjectID(GL.timerQueries, id, 'glGetQueryObjectiv', 'id'); - let result = gl.getQueryObject(GL.timerQueries[id], pname); - getArray(ptr, Uint32Array, 1)[0] = result; - }, - glGetQueryObjectui64v: function (id, pname, ptr) { - GL.validateGLObjectID(GL.timerQueries, id, 'glGetQueryObjectui64v', 'id'); - let result = gl.getQueryObject(GL.timerQueries[id], pname); - let heap = getArray(ptr, Uint32Array, 2); - heap[0] = result; - heap[1] = (result - heap[0])/4294967296; - }, + }, + glBeginQuery: function (target, id) { + GL.validateGLObjectID(GL.timerQueries, id, 'glBeginQuery', 'id'); + gl.beginQuery(target, GL.timerQueries[id]); + }, + glEndQuery: function (target) { + gl.endQuery(target); + }, + glGetQueryObjectiv: function (id, pname, ptr) { + GL.validateGLObjectID(GL.timerQueries, id, 'glGetQueryObjectiv', 'id'); + let result = gl.getQueryObject(GL.timerQueries[id], pname); + getArray(ptr, Uint32Array, 1)[0] = result; + }, + glGetQueryObjectui64v: function (id, pname, ptr) { + GL.validateGLObjectID(GL.timerQueries, id, 'glGetQueryObjectui64v', 'id'); + let result = gl.getQueryObject(GL.timerQueries[id], pname); + let heap = getArray(ptr, Uint32Array, 2); + heap[0] = result; + heap[1] = (result - heap[0]) / 4294967296; + }, glGenerateMipmap: function (index) { gl.generateMipmap(index); }, - setup_canvas_size: function(high_dpi) { + setup_canvas_size: function (high_dpi) { window.high_dpi = high_dpi; resize(canvas); }, @@ -1220,20 +1230,20 @@ var importObject = { window.onresize = function () { resize(canvas, wasm_exports.resize); }; - window.addEventListener("copy", function(e) { + window.addEventListener("copy", function (e) { if (clipboard != null) { event.clipboardData.setData('text/plain', clipboard); event.preventDefault(); } }); - window.addEventListener("cut", function(e) { + window.addEventListener("cut", function (e) { if (clipboard != null) { event.clipboardData.setData('text/plain', clipboard); event.preventDefault(); } }); - window.addEventListener("paste", function(e) { + window.addEventListener("paste", function (e) { e.stopPropagation(); e.preventDefault(); var clipboardData = e.clipboardData || window.clipboardData; @@ -1248,11 +1258,11 @@ var importObject = { } }); - window.ondragover = function(e) { + window.ondragover = function (e) { e.preventDefault(); }; - window.ondrop = async function(e) { + window.ondrop = async function (e) { e.preventDefault(); wasm_exports.on_files_dropped_start(); @@ -1276,9 +1286,9 @@ var importObject = { }; let lastFocus = document.hasFocus(); - var checkFocus = function() { + var checkFocus = function () { let hasFocus = document.hasFocus(); - if(lastFocus == hasFocus){ + if (lastFocus == hasFocus) { wasm_exports.focus(hasFocus); lastFocus = hasFocus; } @@ -1296,22 +1306,22 @@ var importObject = { FS.unique_id += 1; var xhr = new XMLHttpRequest(); xhr.open('GET', url, true); - xhr.responseType = 'arraybuffer'; + xhr.responseType = 'arraybuffer'; - xhr.onreadystatechange = function() { - // looks like readyState === 4 will be fired on either successful or unsuccessful load: - // https://stackoverflow.com/a/19247992 + xhr.onreadystatechange = function () { + // looks like readyState === 4 will be fired on either successful or unsuccessful load: + // https://stackoverflow.com/a/19247992 if (this.readyState === 4) { - if(this.status === 200) { + if (this.status === 200) { var uInt8Array = new Uint8Array(this.response); - + FS.loaded_files[file_id] = uInt8Array; wasm_exports.file_loaded(file_id); } else { FS.loaded_files[file_id] = null; wasm_exports.file_loaded(file_id); } - } + } }; xhr.send(); @@ -1341,22 +1351,22 @@ var importObject = { document.exitPointerLock(); } }, - sapp_set_cursor: function(ptr, len) { + sapp_set_cursor: function (ptr, len) { canvas.style.cursor = UTF8ToString(ptr, len); }, - sapp_is_fullscreen: function() { + sapp_is_fullscreen: function () { let fullscreenElement = document.fullscreenElement; return fullscreenElement != null && fullscreenElement.id == canvas.id; }, - sapp_set_fullscreen: function(fullscreen) { + sapp_set_fullscreen: function (fullscreen) { if (!fullscreen) { document.exitFullscreen(); } else { canvas.requestFullscreen(); } }, - sapp_set_window_size: function(new_width, new_height) { + sapp_set_window_size: function (new_width, new_height) { canvas.width = new_width; canvas.height = new_height; resize(canvas, wasm_exports.resize); @@ -1406,11 +1416,11 @@ function init_plugins(plugins) { if (plugins[i].version != crate_version) { console.error("Plugin " + plugins[i].name + " version mismatch" + - "js version: " + plugins[i].version + ", crate version: " + crate_version) + "js version: " + plugins[i].version + ", crate version: " + crate_version) } } } - } + } } @@ -1427,7 +1437,7 @@ function add_missing_functions_stabs(obj) { for (const i in imports) { if (importObject["env"][imports[i].name] == undefined) { console.warn("No " + imports[i].name + " function in gl.js"); - importObject["env"][imports[i].name] = function() { + importObject["env"][imports[i].name] = function () { console.warn("Missed function: " + imports[i].name); }; } @@ -1454,7 +1464,7 @@ function load(wasm_path) { if (version != crate_version) { console.error( "Version mismatch: gl.js version is: " + version + - ", miniquad crate version is: " + crate_version); + ", miniquad crate version is: " + crate_version); } init_plugins(plugins); obj.exports.main(); @@ -1479,7 +1489,7 @@ function load(wasm_path) { if (version != crate_version) { console.error( "Version mismatch: gl.js version is: " + version + - ", rust sapp-wasm crate version is: " + crate_version); + ", rust sapp-wasm crate version is: " + crate_version); } init_plugins(plugins); obj.exports.main(); diff --git a/src/graphics.rs b/src/graphics.rs index 3b5b1d61..1ae18688 100644 --- a/src/graphics.rs +++ b/src/graphics.rs @@ -1197,6 +1197,7 @@ pub trait RenderingBackend { params: PipelineParams, ) -> Pipeline; fn apply_pipeline(&mut self, pipeline: &Pipeline); + fn delete_pipeline(&mut self, pipeline: Pipeline); /// Create a buffer resource object. /// ```ignore @@ -1230,7 +1231,7 @@ pub trait RenderingBackend { /// More high-level code on top of miniquad probably is going to call this in Drop implementation of some /// more RAII buffer object. /// - /// There is no protection against using deleted textures later. However its not an UB in OpenGl and thats why + /// There is no protection against using deleted buffers later. However its not an UB in OpenGl and thats why /// this function is not marked as unsafe fn delete_buffer(&mut self, buffer: BufferId); @@ -1243,6 +1244,15 @@ pub trait RenderingBackend { /// this function is not marked as unsafe fn delete_texture(&mut self, texture: TextureId); + /// Delete GPU program, leaving handle unmodified. + /// + /// More high-level code on top of miniquad probably is going to call this in Drop implementation of some + /// more RAII buffer object. + /// + /// There is no protection against using deleted programs later. However its not a CPU-level Porgram and thats why + /// this function is not marked as unsafe + fn delete_program(&mut self, program: ShaderId); + /// Set a new viewport rectangle. /// Should be applied after begin_pass. fn apply_viewport(&mut self, x: i32, y: i32, w: i32, h: i32); diff --git a/src/graphics/gl.rs b/src/graphics/gl.rs index 1ae6753c..868c9159 100644 --- a/src/graphics/gl.rs +++ b/src/graphics/gl.rs @@ -1,4 +1,4 @@ -use std::ffi::CString; +use std::{collections::HashMap, ffi::CString, hash::Hash}; use crate::window; @@ -443,10 +443,17 @@ impl Textures { } } pub struct GlContext { - shaders: Vec, - pipelines: Vec, - passes: Vec, - buffers: Vec, + shader_id: usize, + shaders: HashMap, + + pipeline_id: usize, + pipelines: HashMap, + + pass_id: usize, + passes: HashMap, + + buffer_id: usize, + buffers: HashMap, textures: Textures, default_framebuffer: GLuint, pub(crate) cache: GlCache, @@ -468,10 +475,14 @@ impl GlContext { glBindVertexArray(vao); GlContext { default_framebuffer, - shaders: vec![], - pipelines: vec![], - passes: vec![], - buffers: vec![], + shader_id: 0, + shaders: HashMap::default(), + pipeline_id: 0, + pipelines: HashMap::default(), + pass_id: 0, + passes: HashMap::default(), + buffer_id: 0, + buffers: HashMap::default(), textures: Textures(vec![]), features: Features { instancing: !crate::native::gl::is_gl2(), @@ -521,6 +532,11 @@ fn load_shader_internal( glAttachShader(program, fragment_shader); glLinkProgram(program); + // delete no longer used shaders + glDetachShader(program, vertex_shader); + glDeleteShader(vertex_shader); + glDeleteShader(fragment_shader); + let mut link_status = 0; glGetProgramiv(program, GL_LINK_STATUS, &mut link_status as *mut _); if link_status == 0 { @@ -796,8 +812,9 @@ impl RenderingBackend for GlContext { _ => panic!("Metal source on OpenGl context"), }; let shader = load_shader_internal(vertex, fragment, meta)?; - self.shaders.push(shader); - Ok(ShaderId(self.shaders.len() - 1)) + self.shaders.insert(self.shader_id, shader); + self.shader_id += 1; + Ok(ShaderId(self.shader_id - 1)) } fn new_texture( @@ -814,6 +831,17 @@ impl RenderingBackend for GlContext { let t = self.textures.get(texture); t.delete(); } + + fn delete_program(&mut self, program: ShaderId) { + unsafe { glDeleteProgram(self.shaders[&program.0].program) }; + self.shaders.remove(&program.0); + self.cache.cur_pipeline = None; + } + + fn delete_pipeline(&mut self, pipeline: Pipeline) { + self.pipelines.remove(&pipeline.0); + } + fn texture_set_wrap(&mut self, texture: TextureId, wrap_x: TextureWrap, wrap_y: TextureWrap) { let t = self.textures.get(texture); @@ -965,15 +993,16 @@ impl RenderingBackend for GlContext { depth_texture: depth_img, }; - self.passes.push(pass); - - RenderPass(self.passes.len() - 1) + self.passes.insert(self.pass_id, pass); + self.pass_id += 1; + RenderPass(self.pass_id - 1) } fn render_pass_color_attachments(&self, render_pass: RenderPass) -> &[TextureId] { - &self.passes[render_pass.0].color_textures + &self.passes[&render_pass.0].color_textures } fn delete_render_pass(&mut self, render_pass: RenderPass) { - let render_pass = &mut self.passes[render_pass.0]; + let pass_id = render_pass.0; + let render_pass = self.passes.get_mut(&pass_id).unwrap(); unsafe { glDeleteFramebuffers(1, &mut render_pass.gl_fb as *mut _) } @@ -983,6 +1012,7 @@ impl RenderingBackend for GlContext { if let Some(depth_texture) = render_pass.depth_texture { self.textures.get(depth_texture).delete(); } + self.passes.remove(&pass_id); } fn new_pipeline( @@ -1021,7 +1051,7 @@ impl RenderingBackend for GlContext { assert!(cache.stride <= 255); } - let program = self.shaders[shader.0].program; + let program = self.shaders[&shader.0].program; let attributes_len = attributes .iter() @@ -1092,16 +1122,17 @@ impl RenderingBackend for GlContext { params, }; - self.pipelines.push(pipeline); - Pipeline(self.pipelines.len() - 1) + self.pipelines.insert(self.pipeline_id, pipeline); + self.pipeline_id += 1; + Pipeline(self.pipeline_id - 1) } fn apply_pipeline(&mut self, pipeline: &Pipeline) { self.cache.cur_pipeline = Some(*pipeline); { - let pipeline = &self.pipelines[pipeline.0]; - let shader = &mut self.shaders[pipeline.shader.0]; + let pipeline = &self.pipelines[&pipeline.0]; + let shader = self.shaders.get_mut(&pipeline.shader.0).unwrap(); unsafe { glUseProgram(shader.program); } @@ -1131,14 +1162,14 @@ impl RenderingBackend for GlContext { } } - self.set_cull_face(self.pipelines[pipeline.0].params.cull_face); + self.set_cull_face(self.pipelines[&pipeline.0].params.cull_face); self.set_blend( - self.pipelines[pipeline.0].params.color_blend, - self.pipelines[pipeline.0].params.alpha_blend, + self.pipelines[&pipeline.0].params.color_blend, + self.pipelines[&pipeline.0].params.alpha_blend, ); - self.set_stencil(self.pipelines[pipeline.0].params.stencil_test); - self.set_color_write(self.pipelines[pipeline.0].params.color_write); + self.set_stencil(self.pipelines[&pipeline.0].params.stencil_test); + self.set_color_write(self.pipelines[&pipeline.0].params.color_write); } fn new_buffer( @@ -1183,8 +1214,9 @@ impl RenderingBackend for GlContext { size, index_type, }; - self.buffers.push(buffer); - BufferId(self.buffers.len() - 1) + self.buffers.insert(self.buffer_id, buffer); + self.buffer_id += 1; + BufferId(self.buffer_id - 1) } fn buffer_update(&mut self, buffer: BufferId, data: BufferSource) { @@ -1193,7 +1225,7 @@ impl RenderingBackend for GlContext { _ => panic!("buffer_update expects BufferSource::slice"), }; debug_assert!(data.is_slice); - let buffer = &self.buffers[buffer.0]; + let buffer = &self.buffers[&buffer.0]; if matches!(buffer.buffer_type, BufferType::IndexBuffer) { assert!(buffer.index_type.is_some()); @@ -1214,7 +1246,7 @@ impl RenderingBackend for GlContext { /// Size of buffer in bytes fn buffer_size(&mut self, buffer: BufferId) -> usize { - self.buffers[buffer.0].size + self.buffers[&buffer.0].size } /// Delete GPU buffer, leaving handle unmodified. @@ -1225,9 +1257,10 @@ impl RenderingBackend for GlContext { /// There is no protection against using deleted textures later. However its not an UB in OpenGl and thats why /// this function is not marked as unsafe fn delete_buffer(&mut self, buffer: BufferId) { - unsafe { glDeleteBuffers(1, &self.buffers[buffer.0].gl_buf as *const _) } + unsafe { glDeleteBuffers(1, &self.buffers[&buffer.0].gl_buf as *const _) } self.cache.clear_buffer_bindings(); self.cache.clear_vertex_attributes(); + self.buffers.remove(&buffer.0); } /// Set a new viewport rectangle. @@ -1252,8 +1285,8 @@ impl RenderingBackend for GlContext { index_buffer: BufferId, textures: &[TextureId], ) { - let pip = &self.pipelines[self.cache.cur_pipeline.unwrap().0]; - let shader = &self.shaders[pip.shader.0]; + let pip = &self.pipelines[&self.cache.cur_pipeline.unwrap().0]; + let shader = &self.shaders[&pip.shader.0]; for (n, shader_image) in shader.images.iter().enumerate() { let bindings_image = textures @@ -1271,11 +1304,11 @@ impl RenderingBackend for GlContext { self.cache.bind_buffer( GL_ELEMENT_ARRAY_BUFFER, - self.buffers[index_buffer.0].gl_buf, - self.buffers[index_buffer.0].index_type, + self.buffers[&index_buffer.0].gl_buf, + self.buffers[&index_buffer.0].index_type, ); - let pip = &self.pipelines[self.cache.cur_pipeline.unwrap().0]; + let pip = &self.pipelines[&self.cache.cur_pipeline.unwrap().0]; for attr_index in 0..MAX_VERTEX_ATTRIBUTES { let cached_attr = &mut self.cache.attributes[attr_index]; @@ -1288,7 +1321,7 @@ impl RenderingBackend for GlContext { "Attribute index outside of vertex_buffers length" ); let vb = vertex_buffers[attribute.buffer_index]; - let vb = self.buffers[vb.0]; + let vb = self.buffers[&vb.0]; if cached_attr.map_or(true, |cached_attr| { attribute != cached_attr.attribute || cached_attr.gl_vbuf != vb.gl_buf @@ -1329,8 +1362,8 @@ impl RenderingBackend for GlContext { } fn apply_uniforms_from_bytes(&mut self, uniform_ptr: *const u8, size: usize) { - let pip = &self.pipelines[self.cache.cur_pipeline.unwrap().0]; - let shader = &self.shaders[pip.shader.0]; + let pip = &self.pipelines[&self.cache.cur_pipeline.unwrap().0]; + let shader = &self.shaders[&pip.shader.0]; let mut offset = 0; @@ -1433,7 +1466,7 @@ impl RenderingBackend for GlContext { ) } Some(pass) => { - let pass = &self.passes[pass.0]; + let pass = &self.passes[&pass.0]; // new_render_pass will panic with both color and depth components none // so unwrap is safe here let texture = pass @@ -1491,7 +1524,7 @@ impl RenderingBackend for GlContext { return; } - let pip = &self.pipelines[self.cache.cur_pipeline.unwrap().0]; + let pip = &self.pipelines[&self.cache.cur_pipeline.unwrap().0]; let primitive_type = pip.params.primitive_type.into(); let index_type = self.cache.index_type.expect("Unset index buffer type"); diff --git a/src/native/gl.rs b/src/native/gl.rs index cf1bb3ce..9959b949 100644 --- a/src/native/gl.rs +++ b/src/native/gl.rs @@ -587,6 +587,7 @@ gl_loader!( fn glStencilMask(mask: GLuint) -> (), fn glStencilMaskSeparate(face: GLenum, mask: GLuint) -> (), fn glAttachShader(program: GLuint, shader: GLuint) -> (), + fn glDetachShader(program: GLuint, shader: GLuint) -> (), fn glGetError() -> GLenum, fn glClearColor(red: GLclampf, green: GLclampf, blue: GLclampf, alpha: GLclampf) -> (), fn glBlendColor(red: GLclampf, green: GLclampf, blue: GLclampf, alpha: GLclampf) -> (), From 07bc49e338f5582182d352d05b54c6d984a58b6c Mon Sep 17 00:00:00 2001 From: Wael El Oraiby Date: Wed, 1 May 2024 21:09:58 -0400 Subject: [PATCH 2/4] Move management code to a ResourceManager container --- Cargo.toml | 11 +++++ src/graphics/gl.rs | 107 +++++++++++++++++++-------------------------- src/lib.rs | 43 ++++++++++++++++++ 3 files changed, 99 insertions(+), 62 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 58ebc815..d81ef2fb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -46,3 +46,14 @@ objc = "0.2" [dev-dependencies] glam = { version = "0.24", features = ["scalar-math"] } quad-rand = "0.1" + +[profile.release] +lto = true +panic = 'abort' +opt-level = "s" +overflow-checks = false +debug-assertions = false +incremental = false +rpath = false +codegen-units = 1 +strip = true diff --git a/src/graphics/gl.rs b/src/graphics/gl.rs index 868c9159..41a0ce26 100644 --- a/src/graphics/gl.rs +++ b/src/graphics/gl.rs @@ -1,6 +1,6 @@ -use std::{collections::HashMap, ffi::CString, hash::Hash}; +use std::ffi::CString; -use crate::window; +use crate::{window, ResourceManager}; mod cache; @@ -443,17 +443,10 @@ impl Textures { } } pub struct GlContext { - shader_id: usize, - shaders: HashMap, - - pipeline_id: usize, - pipelines: HashMap, - - pass_id: usize, - passes: HashMap, - - buffer_id: usize, - buffers: HashMap, + shaders: ResourceManager, + pipelines: ResourceManager, + passes: ResourceManager, + buffers: ResourceManager, textures: Textures, default_framebuffer: GLuint, pub(crate) cache: GlCache, @@ -475,14 +468,10 @@ impl GlContext { glBindVertexArray(vao); GlContext { default_framebuffer, - shader_id: 0, - shaders: HashMap::default(), - pipeline_id: 0, - pipelines: HashMap::default(), - pass_id: 0, - passes: HashMap::default(), - buffer_id: 0, - buffers: HashMap::default(), + shaders: ResourceManager::default(), + pipelines: ResourceManager::default(), + passes: ResourceManager::default(), + buffers: ResourceManager::default(), textures: Textures(vec![]), features: Features { instancing: !crate::native::gl::is_gl2(), @@ -812,9 +801,7 @@ impl RenderingBackend for GlContext { _ => panic!("Metal source on OpenGl context"), }; let shader = load_shader_internal(vertex, fragment, meta)?; - self.shaders.insert(self.shader_id, shader); - self.shader_id += 1; - Ok(ShaderId(self.shader_id - 1)) + Ok(ShaderId(self.shaders.add(shader))) } fn new_texture( @@ -833,13 +820,13 @@ impl RenderingBackend for GlContext { } fn delete_program(&mut self, program: ShaderId) { - unsafe { glDeleteProgram(self.shaders[&program.0].program) }; - self.shaders.remove(&program.0); + unsafe { glDeleteProgram(self.shaders[program.0].program) }; + self.shaders.remove(program.0); self.cache.cur_pipeline = None; } fn delete_pipeline(&mut self, pipeline: Pipeline) { - self.pipelines.remove(&pipeline.0); + self.pipelines.remove(pipeline.0); } fn texture_set_wrap(&mut self, texture: TextureId, wrap_x: TextureWrap, wrap_y: TextureWrap) { @@ -993,18 +980,17 @@ impl RenderingBackend for GlContext { depth_texture: depth_img, }; - self.passes.insert(self.pass_id, pass); - self.pass_id += 1; - RenderPass(self.pass_id - 1) + RenderPass(self.passes.add(pass)) } fn render_pass_color_attachments(&self, render_pass: RenderPass) -> &[TextureId] { - &self.passes[&render_pass.0].color_textures + &self.passes[render_pass.0].color_textures } fn delete_render_pass(&mut self, render_pass: RenderPass) { let pass_id = render_pass.0; - let render_pass = self.passes.get_mut(&pass_id).unwrap(); - unsafe { glDeleteFramebuffers(1, &mut render_pass.gl_fb as *mut _) } + let render_pass = &self.passes[pass_id]; + + unsafe { glDeleteFramebuffers(1, &render_pass.gl_fb as *const _) } for color_texture in &render_pass.color_textures { self.textures.get(*color_texture).delete(); @@ -1012,7 +998,7 @@ impl RenderingBackend for GlContext { if let Some(depth_texture) = render_pass.depth_texture { self.textures.get(depth_texture).delete(); } - self.passes.remove(&pass_id); + self.passes.remove(pass_id); } fn new_pipeline( @@ -1051,7 +1037,7 @@ impl RenderingBackend for GlContext { assert!(cache.stride <= 255); } - let program = self.shaders[&shader.0].program; + let program = self.shaders[shader.0].program; let attributes_len = attributes .iter() @@ -1122,17 +1108,15 @@ impl RenderingBackend for GlContext { params, }; - self.pipelines.insert(self.pipeline_id, pipeline); - self.pipeline_id += 1; - Pipeline(self.pipeline_id - 1) + Pipeline(self.pipelines.add(pipeline)) } fn apply_pipeline(&mut self, pipeline: &Pipeline) { self.cache.cur_pipeline = Some(*pipeline); { - let pipeline = &self.pipelines[&pipeline.0]; - let shader = self.shaders.get_mut(&pipeline.shader.0).unwrap(); + let pipeline = &self.pipelines[pipeline.0]; + let shader = &self.shaders[pipeline.shader.0]; unsafe { glUseProgram(shader.program); } @@ -1162,14 +1146,14 @@ impl RenderingBackend for GlContext { } } - self.set_cull_face(self.pipelines[&pipeline.0].params.cull_face); + self.set_cull_face(self.pipelines[pipeline.0].params.cull_face); self.set_blend( - self.pipelines[&pipeline.0].params.color_blend, - self.pipelines[&pipeline.0].params.alpha_blend, + self.pipelines[pipeline.0].params.color_blend, + self.pipelines[pipeline.0].params.alpha_blend, ); - self.set_stencil(self.pipelines[&pipeline.0].params.stencil_test); - self.set_color_write(self.pipelines[&pipeline.0].params.color_write); + self.set_stencil(self.pipelines[pipeline.0].params.stencil_test); + self.set_color_write(self.pipelines[pipeline.0].params.color_write); } fn new_buffer( @@ -1214,9 +1198,8 @@ impl RenderingBackend for GlContext { size, index_type, }; - self.buffers.insert(self.buffer_id, buffer); - self.buffer_id += 1; - BufferId(self.buffer_id - 1) + + BufferId(self.buffers.add(buffer)) } fn buffer_update(&mut self, buffer: BufferId, data: BufferSource) { @@ -1225,7 +1208,7 @@ impl RenderingBackend for GlContext { _ => panic!("buffer_update expects BufferSource::slice"), }; debug_assert!(data.is_slice); - let buffer = &self.buffers[&buffer.0]; + let buffer = &self.buffers[buffer.0]; if matches!(buffer.buffer_type, BufferType::IndexBuffer) { assert!(buffer.index_type.is_some()); @@ -1246,7 +1229,7 @@ impl RenderingBackend for GlContext { /// Size of buffer in bytes fn buffer_size(&mut self, buffer: BufferId) -> usize { - self.buffers[&buffer.0].size + self.buffers[buffer.0].size } /// Delete GPU buffer, leaving handle unmodified. @@ -1257,10 +1240,10 @@ impl RenderingBackend for GlContext { /// There is no protection against using deleted textures later. However its not an UB in OpenGl and thats why /// this function is not marked as unsafe fn delete_buffer(&mut self, buffer: BufferId) { - unsafe { glDeleteBuffers(1, &self.buffers[&buffer.0].gl_buf as *const _) } + unsafe { glDeleteBuffers(1, &self.buffers[buffer.0].gl_buf as *const _) } self.cache.clear_buffer_bindings(); self.cache.clear_vertex_attributes(); - self.buffers.remove(&buffer.0); + self.buffers.remove(buffer.0); } /// Set a new viewport rectangle. @@ -1285,8 +1268,8 @@ impl RenderingBackend for GlContext { index_buffer: BufferId, textures: &[TextureId], ) { - let pip = &self.pipelines[&self.cache.cur_pipeline.unwrap().0]; - let shader = &self.shaders[&pip.shader.0]; + let pip = &self.pipelines[self.cache.cur_pipeline.unwrap().0]; + let shader = &self.shaders[pip.shader.0]; for (n, shader_image) in shader.images.iter().enumerate() { let bindings_image = textures @@ -1304,11 +1287,11 @@ impl RenderingBackend for GlContext { self.cache.bind_buffer( GL_ELEMENT_ARRAY_BUFFER, - self.buffers[&index_buffer.0].gl_buf, - self.buffers[&index_buffer.0].index_type, + self.buffers[index_buffer.0].gl_buf, + self.buffers[index_buffer.0].index_type, ); - let pip = &self.pipelines[&self.cache.cur_pipeline.unwrap().0]; + let pip = &self.pipelines[self.cache.cur_pipeline.unwrap().0]; for attr_index in 0..MAX_VERTEX_ATTRIBUTES { let cached_attr = &mut self.cache.attributes[attr_index]; @@ -1321,7 +1304,7 @@ impl RenderingBackend for GlContext { "Attribute index outside of vertex_buffers length" ); let vb = vertex_buffers[attribute.buffer_index]; - let vb = self.buffers[&vb.0]; + let vb = self.buffers[vb.0]; if cached_attr.map_or(true, |cached_attr| { attribute != cached_attr.attribute || cached_attr.gl_vbuf != vb.gl_buf @@ -1362,8 +1345,8 @@ impl RenderingBackend for GlContext { } fn apply_uniforms_from_bytes(&mut self, uniform_ptr: *const u8, size: usize) { - let pip = &self.pipelines[&self.cache.cur_pipeline.unwrap().0]; - let shader = &self.shaders[&pip.shader.0]; + let pip = &self.pipelines[self.cache.cur_pipeline.unwrap().0]; + let shader = &self.shaders[pip.shader.0]; let mut offset = 0; @@ -1466,7 +1449,7 @@ impl RenderingBackend for GlContext { ) } Some(pass) => { - let pass = &self.passes[&pass.0]; + let pass = &self.passes[pass.0]; // new_render_pass will panic with both color and depth components none // so unwrap is safe here let texture = pass @@ -1524,7 +1507,7 @@ impl RenderingBackend for GlContext { return; } - let pip = &self.pipelines[&self.cache.cur_pipeline.unwrap().0]; + let pip = &self.pipelines[self.cache.cur_pipeline.unwrap().0]; let primitive_type = pip.params.primitive_type.into(); let index_type = self.cache.index_type.expect("Unset index buffer type"); diff --git a/src/lib.rs b/src/lib.rs index ef402e7a..c008105b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6,6 +6,8 @@ mod event; pub mod fs; pub mod graphics; mod native; +use std::collections::HashMap; +use std::ops::{Index, IndexMut}; #[cfg(feature = "log-impl")] pub mod log; @@ -18,6 +20,47 @@ mod default_icon; pub use native::gl; +#[derive(Clone)] +pub(crate) struct ResourceManager { + id: usize, + resources: HashMap, +} + +impl Default for ResourceManager { + fn default() -> Self { + Self { + id: 0, + resources: HashMap::new(), + } + } +} + +impl ResourceManager { + pub fn add(&mut self, resource: T) -> usize { + self.resources.insert(self.id, resource); + self.id += 1; + self.id - 1 + } + + pub fn remove(&mut self, id: usize) { + // Let it crash if the resource is not found + self.resources.remove(&id).unwrap(); + } +} + +impl Index for ResourceManager { + type Output = T; + fn index(&self, index: usize) -> &Self::Output { + &self.resources[&index] + } +} + +impl IndexMut for ResourceManager { + fn index_mut(&mut self, index: usize) -> &mut Self::Output { + self.resources.get_mut(&index).unwrap() + } +} + pub mod date { #[cfg(not(target_arch = "wasm32"))] pub fn now() -> f64 { From 07a2223e737e78026041103c0e6b1446c514e63d Mon Sep 17 00:00:00 2001 From: Wael El Oraiby Date: Fri, 3 May 2024 20:17:40 -0400 Subject: [PATCH 3/4] Rename delete_program to delete_shader (add delete_shader, delete_pipeline to the metal backend as place holder) --- src/graphics.rs | 2 +- src/graphics/gl.rs | 2 +- src/graphics/metal.rs | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/graphics.rs b/src/graphics.rs index 1ae18688..ff320b93 100644 --- a/src/graphics.rs +++ b/src/graphics.rs @@ -1251,7 +1251,7 @@ pub trait RenderingBackend { /// /// There is no protection against using deleted programs later. However its not a CPU-level Porgram and thats why /// this function is not marked as unsafe - fn delete_program(&mut self, program: ShaderId); + fn delete_shader(&mut self, program: ShaderId); /// Set a new viewport rectangle. /// Should be applied after begin_pass. diff --git a/src/graphics/gl.rs b/src/graphics/gl.rs index 41a0ce26..62e4baac 100644 --- a/src/graphics/gl.rs +++ b/src/graphics/gl.rs @@ -819,7 +819,7 @@ impl RenderingBackend for GlContext { t.delete(); } - fn delete_program(&mut self, program: ShaderId) { + fn delete_shader(&mut self, program: ShaderId) { unsafe { glDeleteProgram(self.shaders[program.0].program) }; self.shaders.remove(program.0); self.cache.cur_pipeline = None; diff --git a/src/graphics/metal.rs b/src/graphics/metal.rs index e4614502..2e95c4e5 100644 --- a/src/graphics/metal.rs +++ b/src/graphics/metal.rs @@ -1248,6 +1248,8 @@ impl RenderingBackend for MetalContext { } } + fn delete_shader(&mut self, shader: ShaderId) {} + fn delete_pipeline(&mut self, pipeline: Pipeline) {} fn commit_frame(&mut self) { unsafe { assert!(!self.command_queue.is_null()); From 26541ca3c06b4e42d31e8a9f498506e3cc8df7aa Mon Sep 17 00:00:00 2001 From: Wael El Oraiby Date: Mon, 6 May 2024 23:11:29 -0400 Subject: [PATCH 4/4] Add todo comments for metal --- src/graphics/metal.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/graphics/metal.rs b/src/graphics/metal.rs index 2e95c4e5..fcc7d0cc 100644 --- a/src/graphics/metal.rs +++ b/src/graphics/metal.rs @@ -1248,8 +1248,13 @@ impl RenderingBackend for MetalContext { } } - fn delete_shader(&mut self, shader: ShaderId) {} - fn delete_pipeline(&mut self, pipeline: Pipeline) {} + fn delete_shader(&mut self, shader: ShaderId) { + // TODO: place holder + } + fn delete_pipeline(&mut self, pipeline: Pipeline) { + // TODO: place holder + } + fn commit_frame(&mut self) { unsafe { assert!(!self.command_queue.is_null());