From 9795a3b8c39e4210e9ce7378b473a2809790433d Mon Sep 17 00:00:00 2001 From: tigregalis Date: Tue, 18 Jun 2024 22:37:32 +0800 Subject: [PATCH 1/2] remove mutex on font system --- crates/bevy_text/src/pipeline.rs | 41 +++++++++---------- crates/bevy_ui/src/layout/mod.rs | 12 +++++- crates/bevy_ui/src/layout/ui_surface.rs | 21 +++++++--- crates/bevy_ui/src/measurement.rs | 52 ++++++++----------------- crates/bevy_ui/src/widget/image.rs | 20 +++++----- crates/bevy_ui/src/widget/text.rs | 23 ++++++----- 6 files changed, 84 insertions(+), 85 deletions(-) diff --git a/crates/bevy_text/src/pipeline.rs b/crates/bevy_text/src/pipeline.rs index 7d8d2e2fdc5b0..2836de526162a 100644 --- a/crates/bevy_text/src/pipeline.rs +++ b/crates/bevy_text/src/pipeline.rs @@ -1,4 +1,4 @@ -use std::sync::{Arc, Mutex}; +use std::sync::Arc; use bevy_asset::{AssetId, Assets}; use bevy_ecs::{component::Component, reflect::ReflectComponent, system::Resource}; @@ -16,16 +16,14 @@ use crate::{ }; /// A wrapper around a [`cosmic_text::FontSystem`] -struct CosmicFontSystem(Arc>); +struct CosmicFontSystem(cosmic_text::FontSystem); impl Default for CosmicFontSystem { fn default() -> Self { let locale = sys_locale::get_locale().unwrap_or_else(|| String::from("en-US")); let db = cosmic_text::fontdb::Database::new(); // TODO: consider using `cosmic_text::FontSystem::new()` (load system fonts by default) - Self(Arc::new(Mutex::new( - cosmic_text::FontSystem::new_with_locale_and_db(locale, db), - ))) + Self(cosmic_text::FontSystem::new_with_locale_and_db(locale, db)) } } @@ -68,7 +66,7 @@ impl TextPipeline { scale_factor: f64, buffer: &mut CosmicBuffer, ) -> Result<(), TextError> { - let font_system = &mut acquire_font_system(&mut self.font_system)?; + let font_system = &mut self.font_system.0; // return early if the fonts are not loaded yet let mut font_size = 0.; @@ -154,7 +152,7 @@ impl TextPipeline { )?; let box_size = buffer_dimensions(&buffer); - let font_system = &mut acquire_font_system(&mut self.font_system)?; + let font_system = &mut self.font_system.0; let swash_cache = &mut self.swash_cache.0; let glyphs = buffer @@ -253,7 +251,7 @@ impl TextPipeline { let min_width_content_size = buffer_dimensions(&buffer); let max_width_content_size = { - let font_system = &mut acquire_font_system(&mut self.font_system)?; + let font_system = &mut self.font_system.0; buffer.set_size(font_system, None, None); buffer_dimensions(&buffer) }; @@ -261,12 +259,18 @@ impl TextPipeline { Ok(TextMeasureInfo { min: min_width_content_size, max: max_width_content_size, - font_system: Arc::clone(&self.font_system.0), // TODO: This clone feels wasteful, is there another way to structure TextMeasureInfo // that it doesn't need to own a buffer? - bytemunch buffer: buffer.0.clone(), }) } + + /// Get a mutable reference to the [`cosmic_text::FontSystem`]. + /// + /// Used internally. + pub fn font_system_mut(&mut self) -> &mut cosmic_text::FontSystem { + &mut self.font_system.0 + } } /// Render information for a corresponding [`Text`](crate::Text) component. @@ -291,7 +295,6 @@ pub struct TextMeasureInfo { /// Maximum size for a text area in pixels, to be used when laying out widgets with taffy pub max: Vec2, buffer: cosmic_text::Buffer, - font_system: Arc>, } impl std::fmt::Debug for TextMeasureInfo { @@ -307,8 +310,11 @@ impl std::fmt::Debug for TextMeasureInfo { impl TextMeasureInfo { /// Computes the size of the text area within the provided bounds. - pub fn compute_size(&mut self, bounds: Vec2) -> Vec2 { - let font_system = &mut self.font_system.try_lock().expect("Failed to acquire lock"); + pub fn compute_size( + &mut self, + bounds: Vec2, + font_system: &mut cosmic_text::FontSystem, + ) -> Vec2 { self.buffer .set_size(font_system, Some(bounds.x.ceil()), Some(bounds.y.ceil())); buffer_dimensions(&self.buffer) @@ -375,14 +381,3 @@ fn buffer_dimensions(buffer: &Buffer) -> Vec2 { Vec2::new(width.ceil(), height).ceil() } - -/// Helper method to acquire a font system mutex. -#[inline(always)] -fn acquire_font_system( - font_system: &mut CosmicFontSystem, -) -> Result, TextError> { - font_system - .0 - .try_lock() - .map_err(|_| TextError::FailedToAcquireMutex) -} diff --git a/crates/bevy_ui/src/layout/mod.rs b/crates/bevy_ui/src/layout/mod.rs index 43d767acdecbc..c05a14377595d 100644 --- a/crates/bevy_ui/src/layout/mod.rs +++ b/crates/bevy_ui/src/layout/mod.rs @@ -1,3 +1,4 @@ +use bevy_text::TextPipeline; use thiserror::Error; use crate::{ContentSize, DefaultUiCamera, Node, Outline, Style, TargetCamera, UiScale}; @@ -94,6 +95,7 @@ pub fn ui_layout_system( just_children_query: Query<&Children>, mut removed_components: UiLayoutSystemRemovedComponentParam, mut node_transform_query: Query<(&mut Node, &mut Transform)>, + #[cfg(feature = "bevy_text")] mut text_pipeline: ResMut, ) { struct CameraLayoutInfo { size: UVec2, @@ -217,10 +219,18 @@ pub fn ui_layout_system( } } + #[cfg(feature = "bevy_text")] + let font_system = text_pipeline.font_system_mut(); + for (camera_id, camera) in &camera_layout_info { let inverse_target_scale_factor = camera.scale_factor.recip(); - ui_surface.compute_camera_layout(*camera_id, camera.size); + ui_surface.compute_camera_layout( + *camera_id, + camera.size, + #[cfg(feature = "bevy_text")] + font_system, + ); for root in &camera.root_nodes { update_uinode_geometry_recursive( *root, diff --git a/crates/bevy_ui/src/layout/ui_surface.rs b/crates/bevy_ui/src/layout/ui_surface.rs index 8bb13d83bcf70..d7139fbd7c304 100644 --- a/crates/bevy_ui/src/layout/ui_surface.rs +++ b/crates/bevy_ui/src/layout/ui_surface.rs @@ -10,7 +10,7 @@ use bevy_utils::default; use bevy_utils::tracing::warn; use crate::layout::convert; -use crate::{LayoutContext, LayoutError, Measure, NodeMeasure, Style}; +use crate::{LayoutContext, LayoutError, Measure, MeasureArgs, NodeMeasure, Style}; #[derive(Debug, Clone, PartialEq, Eq)] pub struct RootNodePair { @@ -196,7 +196,12 @@ without UI components as a child of an entity with UI components, results may be } /// Compute the layout for each window entity's corresponding root node in the layout. - pub fn compute_camera_layout(&mut self, camera: Entity, render_target_resolution: UVec2) { + pub fn compute_camera_layout( + &mut self, + camera: Entity, + render_target_resolution: UVec2, + #[cfg(feature = "bevy_text")] font_system: &mut bevy_text::cosmic_text::FontSystem, + ) { let Some(camera_root_nodes) = self.camera_roots.get(&camera) else { return; }; @@ -219,10 +224,14 @@ without UI components as a child of an entity with UI components, results may be context .map(|ctx| { let size = ctx.measure( - known_dimensions.width, - known_dimensions.height, - available_space.width, - available_space.height, + MeasureArgs { + width: known_dimensions.width, + height: known_dimensions.height, + available_width: available_space.width, + available_height: available_space.height, + #[cfg(feature = "bevy_text")] + font_system, + }, style, ); taffy::Size { diff --git a/crates/bevy_ui/src/measurement.rs b/crates/bevy_ui/src/measurement.rs index 3aa2c5a84b763..cdc279921856a 100644 --- a/crates/bevy_ui/src/measurement.rs +++ b/crates/bevy_ui/src/measurement.rs @@ -16,18 +16,20 @@ impl std::fmt::Debug for ContentSize { } } +pub struct MeasureArgs<'a> { + pub width: Option, + pub height: Option, + pub available_width: AvailableSpace, + pub available_height: AvailableSpace, + #[cfg(feature = "bevy_text")] + pub font_system: &'a mut bevy_text::cosmic_text::FontSystem, +} + /// A `Measure` is used to compute the size of a ui node /// when the size of that node is based on its content. pub trait Measure: Send + Sync + 'static { /// Calculate the size of the node given the constraints. - fn measure( - &mut self, - width: Option, - height: Option, - available_width: AvailableSpace, - available_height: AvailableSpace, - style: &taffy::Style, - ) -> Vec2; + fn measure<'a>(&mut self, measure_args: MeasureArgs<'a>, style: &taffy::Style) -> Vec2; } /// A type to serve as Taffy's node context (which allows the content size of leaf nodes to be computed) @@ -43,28 +45,13 @@ pub enum NodeMeasure { } impl Measure for NodeMeasure { - fn measure( - &mut self, - width: Option, - height: Option, - available_width: AvailableSpace, - available_height: AvailableSpace, - style: &taffy::Style, - ) -> Vec2 { + fn measure(&mut self, measure_args: MeasureArgs, style: &taffy::Style) -> Vec2 { match self { - NodeMeasure::Fixed(fixed) => { - fixed.measure(width, height, available_width, available_height, style) - } + NodeMeasure::Fixed(fixed) => fixed.measure(measure_args, style), #[cfg(feature = "bevy_text")] - NodeMeasure::Text(text) => { - text.measure(width, height, available_width, available_height, style) - } - NodeMeasure::Image(image) => { - image.measure(width, height, available_width, available_height, style) - } - NodeMeasure::Custom(custom) => { - custom.measure(width, height, available_width, available_height, style) - } + NodeMeasure::Text(text) => text.measure(measure_args, style), + NodeMeasure::Image(image) => image.measure(measure_args, style), + NodeMeasure::Custom(custom) => custom.measure(measure_args, style), } } } @@ -77,14 +64,7 @@ pub struct FixedMeasure { } impl Measure for FixedMeasure { - fn measure( - &mut self, - _: Option, - _: Option, - _: AvailableSpace, - _: AvailableSpace, - _: &taffy::Style, - ) -> Vec2 { + fn measure(&mut self, _: MeasureArgs, _: &taffy::Style) -> Vec2 { self.size } } diff --git a/crates/bevy_ui/src/widget/image.rs b/crates/bevy_ui/src/widget/image.rs index 4603daa00c867..976a89501a356 100644 --- a/crates/bevy_ui/src/widget/image.rs +++ b/crates/bevy_ui/src/widget/image.rs @@ -1,5 +1,6 @@ use crate::{ - measurement::AvailableSpace, ContentSize, Measure, Node, NodeMeasure, UiImage, UiScale, + ContentSize, Measure, MeasureArgs, Node, NodeMeasure, UiImage, + UiScale, }; use bevy_asset::Assets; use bevy_ecs::prelude::*; @@ -37,14 +38,15 @@ pub struct ImageMeasure { } impl Measure for ImageMeasure { - fn measure( - &mut self, - width: Option, - height: Option, - available_width: AvailableSpace, - available_height: AvailableSpace, - style: &taffy::Style, - ) -> Vec2 { + fn measure(&mut self, measure_args: MeasureArgs, style: &taffy::Style) -> Vec2 { + let MeasureArgs { + width, + height, + available_width, + available_height, + .. + } = measure_args; + // Convert available width/height into an option let parent_width = available_width.into_option(); let parent_height = available_height.into_option(); diff --git a/crates/bevy_ui/src/widget/text.rs b/crates/bevy_ui/src/widget/text.rs index fbef8160d2202..83500a5d62f49 100644 --- a/crates/bevy_ui/src/widget/text.rs +++ b/crates/bevy_ui/src/widget/text.rs @@ -1,5 +1,6 @@ use crate::{ - ContentSize, DefaultUiCamera, FixedMeasure, Measure, Node, NodeMeasure, TargetCamera, UiScale, + ContentSize, DefaultUiCamera, FixedMeasure, Measure, MeasureArgs, Node, NodeMeasure, + TargetCamera, UiScale, }; use bevy_asset::Assets; use bevy_ecs::{ @@ -47,14 +48,14 @@ pub struct TextMeasure { } impl Measure for TextMeasure { - fn measure( - &mut self, - width: Option, - height: Option, - available_width: AvailableSpace, - _available_height: AvailableSpace, - _style: &taffy::Style, - ) -> Vec2 { + fn measure(&mut self, measure_args: MeasureArgs, _style: &taffy::Style) -> Vec2 { + let MeasureArgs { + width, + height, + available_width, + font_system, + .. + } = measure_args; let x = width.unwrap_or_else(|| match available_width { AvailableSpace::Definite(x) => { // It is possible for the "min content width" to be larger than @@ -70,7 +71,9 @@ impl Measure for TextMeasure { height .map_or_else( || match available_width { - AvailableSpace::Definite(_) => self.info.compute_size(Vec2::new(x, f32::MAX)), + AvailableSpace::Definite(_) => { + self.info.compute_size(Vec2::new(x, f32::MAX), font_system) + } AvailableSpace::MinContent => Vec2::new(x, self.info.min.y), AvailableSpace::MaxContent => Vec2::new(x, self.info.max.y), }, From 5f0cd22ab0f65caa053639dc9053a46b99c94525 Mon Sep 17 00:00:00 2001 From: tigregalis Date: Tue, 18 Jun 2024 22:43:34 +0800 Subject: [PATCH 2/2] remove TextError::FailedToAcquireMutex --- crates/bevy_text/src/error.rs | 4 ---- crates/bevy_text/src/text2d.rs | 6 +----- crates/bevy_ui/src/widget/text.rs | 12 ++---------- 3 files changed, 3 insertions(+), 19 deletions(-) diff --git a/crates/bevy_text/src/error.rs b/crates/bevy_text/src/error.rs index 9d40e0fcdd59c..ef9f7ea590deb 100644 --- a/crates/bevy_text/src/error.rs +++ b/crates/bevy_text/src/error.rs @@ -11,10 +11,6 @@ pub enum TextError { /// Failed to add glyph to a newly created atlas for some reason #[error("failed to add glyph to newly-created atlas {0:?}")] FailedToAddGlyph(u16), - /// Failed to acquire mutex to cosmic-texts fontsystem - //TODO: this can be removed since the mutex should be possible to remove as well - #[error("font system mutex could not be acquired or is poisoned")] - FailedToAcquireMutex, /// Failed to get scaled glyph image for cache key #[error("failed to get scaled glyph image for cache key: {0:?}")] FailedToGetGlyphImage(CacheKey), diff --git a/crates/bevy_text/src/text2d.rs b/crates/bevy_text/src/text2d.rs index 45a21886c9e84..9391dd459c78f 100644 --- a/crates/bevy_text/src/text2d.rs +++ b/crates/bevy_text/src/text2d.rs @@ -230,11 +230,7 @@ pub fn update_text2d_layout( // queue for further processing queue.insert(entity); } - Err( - e @ (TextError::FailedToAddGlyph(_) - | TextError::FailedToAcquireMutex - | TextError::FailedToGetGlyphImage(_)), - ) => { + Err(e @ (TextError::FailedToAddGlyph(_) | TextError::FailedToGetGlyphImage(_))) => { panic!("Fatal error when processing text: {e}."); } Ok(mut info) => { diff --git a/crates/bevy_ui/src/widget/text.rs b/crates/bevy_ui/src/widget/text.rs index 83500a5d62f49..e65f9767a3bdb 100644 --- a/crates/bevy_ui/src/widget/text.rs +++ b/crates/bevy_ui/src/widget/text.rs @@ -115,11 +115,7 @@ fn create_text_measure( // Try again next frame text_flags.needs_new_measure_func = true; } - Err( - e @ (TextError::FailedToAddGlyph(_) - | TextError::FailedToAcquireMutex - | TextError::FailedToGetGlyphImage(_)), - ) => { + Err(e @ (TextError::FailedToAddGlyph(_) | TextError::FailedToGetGlyphImage(_))) => { panic!("Fatal error when processing text: {e}."); } }; @@ -235,11 +231,7 @@ fn queue_text( // There was an error processing the text layout, try again next frame text_flags.needs_recompute = true; } - Err( - e @ (TextError::FailedToAddGlyph(_) - | TextError::FailedToAcquireMutex - | TextError::FailedToGetGlyphImage(_)), - ) => { + Err(e @ (TextError::FailedToAddGlyph(_) | TextError::FailedToGetGlyphImage(_))) => { panic!("Fatal error when processing text: {e}."); } Ok(mut info) => {