From f31f4201c69a0b09c55bf6bdcc46164c79eb78bc Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Thu, 27 Jan 2022 21:35:05 +0100 Subject: [PATCH] Improve sixtyfps::Image::size() API for consistency Make size() return an unsigned integer size type instead of floats. cc #431 --- CHANGELOG.md | 1 + api/sixtyfps-cpp/include/sixtyfps_image.h | 10 ++--- sixtyfps_runtime/corelib/backend.rs | 4 +- sixtyfps_runtime/corelib/graphics/image.rs | 27 ++++++++++--- sixtyfps_runtime/corelib/items/image.rs | 16 +++++--- .../rendering_backends/gl/images.rs | 38 +++++++------------ sixtyfps_runtime/rendering_backends/gl/lib.rs | 23 +++++++---- .../rendering_backends/mcu/lib.rs | 8 ++-- .../rendering_backends/mcu/simulator.rs | 8 ++-- sixtyfps_runtime/rendering_backends/qt/lib.rs | 10 ++--- .../rendering_backends/testing/lib.rs | 20 +++------- 11 files changed, 84 insertions(+), 81 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6016d71c9..f7468cc4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ This version changes some APIs in incompatible ways. For details how to migrate - The interpreter does not differentiate anymore between `Value::Array` and `Value::Model` everything is a `Value::Model`, which now contains a `ModelHandle` - In Rust, `sixtyfps::SharedPixelBuffer` and `sixtyfps::SharedImageBuffer` now use a `u32` instead of `usize` for `width`, `height` and `stride`. + - In Rust and C++, `sixtyfps::Image::size()` now returns an integer size type. ### Added diff --git a/api/sixtyfps-cpp/include/sixtyfps_image.h b/api/sixtyfps-cpp/include/sixtyfps_image.h index 9b91c81af..1d4dee430 100644 --- a/api/sixtyfps-cpp/include/sixtyfps_image.h +++ b/api/sixtyfps-cpp/include/sixtyfps_image.h @@ -10,16 +10,16 @@ namespace sixtyfps { #if !defined(DOXYGEN) -using cbindgen_private::types::Size; +using cbindgen_private::types::IntSize; #else /// The Size structure is used to represent a two-dimensional size /// with width and height. -struct Size +struct IntSize { /// The width of the size - float width; + uint32_t width; /// The height of the size - float height; + uint32_t height; }; #endif @@ -46,7 +46,7 @@ public: */ /// Returns the size of the Image in pixels. - Size size() const { return cbindgen_private::types::sixtyfps_image_size(&data); } + IntSize size() const { return cbindgen_private::types::sixtyfps_image_size(&data); } /// Returns the path of the image on disk, if it was constructed via Image::load_from_path(). std::optional path() const diff --git a/sixtyfps_runtime/corelib/backend.rs b/sixtyfps_runtime/corelib/backend.rs index b60a09bb6..76dc3cf31 100644 --- a/sixtyfps_runtime/corelib/backend.rs +++ b/sixtyfps_runtime/corelib/backend.rs @@ -9,7 +9,7 @@ use alloc::boxed::Box; use alloc::rc::Rc; use alloc::string::String; -use crate::graphics::{Image, Size}; +use crate::graphics::{Image, IntSize}; use crate::window::Window; #[cfg(feature = "std")] @@ -63,7 +63,7 @@ pub trait Backend: Send + Sync { /// Send an user event to from another thread that should be run in the GUI event loop fn post_event(&'static self, event: Box); - fn image_size(&'static self, image: &Image) -> Size; + fn image_size(&'static self, image: &Image) -> IntSize; fn duration_since_start(&'static self) -> core::time::Duration { #[cfg(feature = "std")] diff --git a/sixtyfps_runtime/corelib/graphics/image.rs b/sixtyfps_runtime/corelib/graphics/image.rs index 26ab60814..c3e151267 100644 --- a/sixtyfps_runtime/corelib/graphics/image.rs +++ b/sixtyfps_runtime/corelib/graphics/image.rs @@ -35,6 +35,11 @@ impl SharedPixelBuffer { self.height } + /// Returns the size of the image in pixels. + pub fn size(&self) -> IntSize { + [self.width, self.height].into() + } + /// Returns the number of pixels per line. pub fn stride(&self) -> u32 { self.stride @@ -158,6 +163,16 @@ impl SharedImageBuffer { Self::RGBA8Premultiplied(buffer) => buffer.height(), } } + + /// Returns the size of the image in pixels. + #[inline] + pub fn size(&self) -> IntSize { + match self { + Self::RGB8(buffer) => buffer.size(), + Self::RGBA8(buffer) => buffer.size(), + Self::RGBA8Premultiplied(buffer) => buffer.size(), + } + } } impl PartialEq for SharedImageBuffer { @@ -368,7 +383,7 @@ impl Image { } /// Returns the size of the Image in pixels. - pub fn size(&self) -> crate::graphics::Size { + pub fn size(&self) -> IntSize { match &self.0 { ImageInner::None => Default::default(), ImageInner::AbsoluteFilePath(_) | ImageInner::EmbeddedData { .. } => { @@ -377,8 +392,8 @@ impl Image { None => panic!("sixtyfps::Image::size() called too early (before a graphics backend was chosen). You need to create a component first."), } }, - ImageInner::EmbeddedImage(buffer) => [buffer.width() as _, buffer.height() as _].into(), - ImageInner::StaticTextures{size, ..} => size.cast(), + ImageInner::EmbeddedImage(buffer) => buffer.size(), + ImageInner::StaticTextures{size, ..} => size.clone(), } } @@ -411,7 +426,7 @@ fn test_image_size_from_buffer_without_backend() { { let buffer = SharedPixelBuffer::::new(320, 200); let image = Image::from_rgb8(buffer); - assert_eq!(image.size(), [320., 200.].into()) + assert_eq!(image.size(), [320, 200].into()) } } @@ -419,7 +434,7 @@ fn test_image_size_from_buffer_without_backend() { pub(crate) mod ffi { #![allow(unsafe_code)] - use super::super::Size; + use super::super::IntSize; use super::*; /// Expand Rgb8Pixel so that cbindgen can see it. (is in fact rgb::RGB) @@ -441,7 +456,7 @@ pub(crate) mod ffi { } #[no_mangle] - pub unsafe extern "C" fn sixtyfps_image_size(image: &Image) -> Size { + pub unsafe extern "C" fn sixtyfps_image_size(image: &Image) -> IntSize { image.size() } diff --git a/sixtyfps_runtime/corelib/items/image.rs b/sixtyfps_runtime/corelib/items/image.rs index 5ec3ef6ac..92b92806f 100644 --- a/sixtyfps_runtime/corelib/items/image.rs +++ b/sixtyfps_runtime/corelib/items/image.rs @@ -78,9 +78,11 @@ impl Item for ImageItem { let natural_size = self.source().size(); LayoutInfo { preferred: match orientation { - _ if natural_size.width == 0. || natural_size.height == 0. => 0., - Orientation::Horizontal => natural_size.width, - Orientation::Vertical => natural_size.height * self.width() / natural_size.width, + _ if natural_size.width == 0 || natural_size.height == 0 => 0., + Orientation::Horizontal => natural_size.width as f32, + Orientation::Vertical => { + natural_size.height as f32 * self.width() / natural_size.width as f32 + } }, ..Default::default() } @@ -153,9 +155,11 @@ impl Item for ClippedImage { let natural_size = self.source().size(); LayoutInfo { preferred: match orientation { - _ if natural_size.width == 0. || natural_size.height == 0. => 0., - Orientation::Horizontal => natural_size.width, - Orientation::Vertical => natural_size.height * self.width() / natural_size.width, + _ if natural_size.width == 0 || natural_size.height == 0 => 0., + Orientation::Horizontal => natural_size.width as f32, + Orientation::Vertical => { + natural_size.height as f32 * self.width() / natural_size.width as f32 + } }, ..Default::default() } diff --git a/sixtyfps_runtime/rendering_backends/gl/images.rs b/sixtyfps_runtime/rendering_backends/gl/images.rs index f895914ce..795a558f4 100644 --- a/sixtyfps_runtime/rendering_backends/gl/images.rs +++ b/sixtyfps_runtime/rendering_backends/gl/images.rs @@ -5,7 +5,7 @@ use std::cell::RefCell; use std::collections::HashMap; use std::rc::Rc; -use sixtyfps_corelib::graphics::{SharedImageBuffer, Size}; +use sixtyfps_corelib::graphics::{IntSize, SharedImageBuffer}; #[cfg(target_arch = "wasm32")] use sixtyfps_corelib::Property; use sixtyfps_corelib::{items::ImageRendering, slice::Slice, ImageInner, SharedString}; @@ -18,11 +18,11 @@ struct Texture { } impl Texture { - fn size(&self) -> Option { + fn size(&self) -> Option { self.canvas .borrow() .image_info(self.id) - .map(|info| [info.width() as f32, info.height() as f32].into()) + .map(|info| [info.width() as u32, info.height() as u32].into()) .ok() } } @@ -77,10 +77,10 @@ impl HTMLImage { Self { dom_element, image_load_pending } } - fn size(&self) -> Option { + fn size(&self) -> Option { match self.image_load_pending.as_ref().get() { true => None, - false => Some(Size::new(self.dom_element.width() as _, self.dom_element.height() as _)), + false => Some(IntSize::new(self.dom_element.width(), self.dom_element.height())), } } } @@ -137,15 +137,15 @@ impl CachedImage { Self(RefCell::new(Texture { id: image_id, canvas: canvas.clone() }.into())) } - pub fn new_empty_on_gpu(canvas: &CanvasRc, width: usize, height: usize) -> Option { + pub fn new_empty_on_gpu(canvas: &CanvasRc, width: u32, height: u32) -> Option { if width == 0 || height == 0 { return None; } let image_id = canvas .borrow_mut() .create_image_empty( - width, - height, + width as usize, + height as usize, femtovg::PixelFormat::Rgba8, femtovg::ImageFlags::PREMULTIPLIED | femtovg::ImageFlags::FLIP_Y, ) @@ -337,23 +337,18 @@ impl CachedImage { } } - pub fn size(&self) -> Option { + pub fn size(&self) -> Option { use image::GenericImageView; match &*self.0.borrow() { ImageData::Texture(texture) => texture.size(), - ImageData::DecodedImage(decoded_image) => { - let (width, height) = decoded_image.dimensions(); - Some([width as f32, height as f32].into()) - } - ImageData::EmbeddedImage(buffer) => { - Some([buffer.width() as f32, buffer.height() as f32].into()) - } + ImageData::DecodedImage(decoded_image) => Some(decoded_image.dimensions().into()), + ImageData::EmbeddedImage(buffer) => Some(buffer.size()), #[cfg(feature = "svg")] ImageData::Svg(tree) => { let size = tree.svg_node().size.to_screen_size(); - Some([size.width() as f32, size.height() as f32].into()) + Some([size.width(), size.height()].into()) } #[cfg(target_arch = "wasm32")] @@ -377,12 +372,7 @@ impl CachedImage { } .expect("internal error: Cannot filter non-GPU images"); - let filtered_image = Self::new_empty_on_gpu( - canvas, - size.width.ceil() as usize, - size.height.ceil() as usize, - ) - .expect( + let filtered_image = Self::new_empty_on_gpu(canvas, size.width, size.height).expect( "internal error: this can only fail if the filtered image was zero width or height", ); @@ -402,7 +392,7 @@ impl CachedImage { let size = tex .size() .expect("internal error: CachedImage::as_paint() called on zero-sized texture"); - femtovg::Paint::image(tex.id, 0., 0., size.width, size.height, 0., 1.) + femtovg::Paint::image(tex.id, 0., 0., size.width as f32, size.height as f32, 0., 1.) } _ => panic!("internal error: CachedImage::as_paint() called on non-texture image"), } diff --git a/sixtyfps_runtime/rendering_backends/gl/lib.rs b/sixtyfps_runtime/rendering_backends/gl/lib.rs index f463c33e3..0209f86c6 100644 --- a/sixtyfps_runtime/rendering_backends/gl/lib.rs +++ b/sixtyfps_runtime/rendering_backends/gl/lib.rs @@ -20,7 +20,7 @@ use std::rc::Rc; use euclid::approxeq::ApproxEq; use event_loop::WinitWindow; use sixtyfps_corelib::graphics::{ - Brush, Color, Image, ImageInner, IntRect, Point, Rect, RenderingCache, Size, + Brush, Color, Image, ImageInner, IntRect, IntSize, Point, Rect, RenderingCache, Size, }; use sixtyfps_corelib::item_rendering::{CachedRenderingData, ItemRenderer}; use sixtyfps_corelib::items::{FillRule, ImageFit, ImageRendering}; @@ -546,8 +546,8 @@ impl ItemRenderer for GLItemRenderer { let shadow_rect: euclid::Rect = euclid::rect(0., 0., width + 2. * blur, height + 2. * blur); - let shadow_image_width = shadow_rect.width().ceil() as usize; - let shadow_image_height = shadow_rect.height().ceil() as usize; + let shadow_image_width = shadow_rect.width().ceil() as u32; + let shadow_image_height = shadow_rect.height().ceil() as u32; let shadow_image = CachedImage::new_empty_on_gpu( &self.canvas, @@ -632,7 +632,12 @@ impl ItemRenderer for GLItemRenderer { let shadow_image_paint = shadow_image.as_paint(); let mut shadow_image_rect = femtovg::Path::new(); - shadow_image_rect.rect(0., 0., shadow_image_size.width, shadow_image_size.height); + shadow_image_rect.rect( + 0., + 0., + shadow_image_size.width as f32, + shadow_image_size.height as f32, + ); self.canvas.borrow_mut().save_with(|canvas| { let blur = box_shadow.blur() * self.scale_factor; @@ -844,13 +849,15 @@ impl GLItemRenderer { .canvas .borrow_mut() .create_image_empty( - image_size.width as _, - image_size.height as _, + image_size.width as usize, + image_size.height as usize, femtovg::PixelFormat::Rgba8, femtovg::ImageFlags::PREMULTIPLIED | scaling_flags, ) .expect("internal error allocating temporary texture for image colorization"); + let image_size: Size = image_size.cast(); + let mut image_rect = femtovg::Path::new(); image_rect.rect(0., 0., image_size.width, image_size.height); let brush_paint = self.brush_to_paint(colorize_brush, &mut image_rect).unwrap(); @@ -977,7 +984,7 @@ impl GLItemRenderer { }; let image_id = cached_image.ensure_uploaded_to_gpu(self, Some(image_rendering)); - let image_size = cached_image.size().unwrap_or_default(); + let image_size = cached_image.size().unwrap_or_default().cast(); let (source_width, source_height) = if source_clip_rect.is_empty() { (image_size.width, image_size.height) @@ -1261,7 +1268,7 @@ impl sixtyfps_corelib::backend::Backend for Backend { } } - fn image_size(&'static self, image: &Image) -> Size { + fn image_size(&'static self, image: &Image) -> IntSize { IMAGE_CACHE.with(|image_cache| { image_cache .borrow_mut() diff --git a/sixtyfps_runtime/rendering_backends/mcu/lib.rs b/sixtyfps_runtime/rendering_backends/mcu/lib.rs index e1054d19c..f72532ac3 100644 --- a/sixtyfps_runtime/rendering_backends/mcu/lib.rs +++ b/sixtyfps_runtime/rendering_backends/mcu/lib.rs @@ -296,17 +296,15 @@ mod the_backend { fn image_size( &'static self, image: &sixtyfps_corelib::graphics::Image, - ) -> sixtyfps_corelib::graphics::Size { + ) -> sixtyfps_corelib::graphics::IntSize { let inner: &ImageInner = image.into(); match inner { ImageInner::None => Default::default(), ImageInner::AbsoluteFilePath(_) | ImageInner::EmbeddedData { .. } => { unimplemented!() } - ImageInner::EmbeddedImage(buffer) => { - [buffer.width() as f32, buffer.height() as f32].into() - } - ImageInner::StaticTextures { size, .. } => size.cast(), + ImageInner::EmbeddedImage(buffer) => buffer.size(), + ImageInner::StaticTextures { size, .. } => size, } } diff --git a/sixtyfps_runtime/rendering_backends/mcu/simulator.rs b/sixtyfps_runtime/rendering_backends/mcu/simulator.rs index 47d464c04..a8fec219a 100644 --- a/sixtyfps_runtime/rendering_backends/mcu/simulator.rs +++ b/sixtyfps_runtime/rendering_backends/mcu/simulator.rs @@ -368,15 +368,13 @@ impl sixtyfps_corelib::backend::Backend for SimulatorBackend { .send_event(self::event_loop::CustomEvent::UserEvent(event)); } - fn image_size(&'static self, image: &Image) -> sixtyfps_corelib::graphics::Size { + fn image_size(&'static self, image: &Image) -> sixtyfps_corelib::graphics::IntSize { let inner: &ImageInner = image.into(); match inner { ImageInner::None => Default::default(), ImageInner::AbsoluteFilePath(_) | ImageInner::EmbeddedData { .. } => unimplemented!(), - ImageInner::EmbeddedImage(buffer) => { - [buffer.width() as f32, buffer.height() as f32].into() - } - ImageInner::StaticTextures { size, .. } => size.cast(), + ImageInner::EmbeddedImage(buffer) => buffer.size(), + ImageInner::StaticTextures { size, .. } => size, } } } diff --git a/sixtyfps_runtime/rendering_backends/qt/lib.rs b/sixtyfps_runtime/rendering_backends/qt/lib.rs index ca0b67e4d..a7d016bd4 100644 --- a/sixtyfps_runtime/rendering_backends/qt/lib.rs +++ b/sixtyfps_runtime/rendering_backends/qt/lib.rs @@ -16,7 +16,7 @@ only be used with `version = "=x.y.z"` in Cargo.toml. extern crate alloc; -use sixtyfps_corelib::graphics::{Image, Size}; +use sixtyfps_corelib::graphics::{Image, IntSize}; #[cfg(not(no_qt))] use sixtyfps_corelib::items::ImageFit; use sixtyfps_corelib::window::Window; @@ -267,19 +267,17 @@ impl sixtyfps_corelib::backend::Backend for Backend { }; } - fn image_size(&'static self, _image: &Image) -> Size { + fn image_size(&'static self, _image: &Image) -> IntSize { #[cfg(not(no_qt))] { let inner: &ImageInner = _image.into(); match inner { sixtyfps_corelib::ImageInner::None => Default::default(), - sixtyfps_corelib::ImageInner::EmbeddedImage(buffer) => { - [buffer.width() as _, buffer.height() as _].into() - } + sixtyfps_corelib::ImageInner::EmbeddedImage(buffer) => buffer.size(), _ => qt_window::load_image_from_resource(inner, None, ImageFit::fill) .map(|img| { let qsize = img.size(); - euclid::size2(qsize.width as f32, qsize.height as f32) + euclid::size2(qsize.width, qsize.height) }) .unwrap_or_default(), } diff --git a/sixtyfps_runtime/rendering_backends/testing/lib.rs b/sixtyfps_runtime/rendering_backends/testing/lib.rs index 27cdd29da..7a2126d54 100644 --- a/sixtyfps_runtime/rendering_backends/testing/lib.rs +++ b/sixtyfps_runtime/rendering_backends/testing/lib.rs @@ -12,7 +12,7 @@ You should use the `sixtyfps` crate instead. use image::GenericImageView; use sixtyfps_corelib::component::ComponentRc; -use sixtyfps_corelib::graphics::{Image, Point, Size}; +use sixtyfps_corelib::graphics::{Image, IntSize, Point, Size}; use sixtyfps_corelib::window::{PlatformWindow, Window}; use sixtyfps_corelib::ImageInner; use std::path::Path; @@ -62,30 +62,22 @@ impl sixtyfps_corelib::backend::Backend for TestingBackend { unimplemented!("event with the testing backend"); } - fn image_size(&'static self, image: &Image) -> Size { + fn image_size(&'static self, image: &Image) -> IntSize { let inner: &ImageInner = image.into(); match inner { ImageInner::None => Default::default(), - ImageInner::EmbeddedImage(buffer) => { - Size::new(buffer.width() as _, buffer.height() as _) - } + ImageInner::EmbeddedImage(buffer) => buffer.size(), ImageInner::AbsoluteFilePath(path) => image::open(Path::new(path.as_str())) - .map(|img| { - let dim = img.dimensions(); - Size::new(dim.0 as _, dim.1 as _) - }) + .map(|img| img.dimensions().into()) .unwrap_or_default(), ImageInner::EmbeddedData { data, format } => image::load_from_memory_with_format( data.as_slice(), image::ImageFormat::from_extension(std::str::from_utf8(format.as_slice()).unwrap()) .unwrap(), ) - .map(|img| { - let dim = img.dimensions(); - Size::new(dim.0 as _, dim.1 as _) - }) + .map(|img| img.dimensions().into()) .unwrap_or_default(), - ImageInner::StaticTextures { size, .. } => size.cast(), + ImageInner::StaticTextures { size, .. } => size.clone(), } } }