Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add texture scale mode #1444

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions src/sdl2/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ use std::rc::Rc;

use crate::sys;
use crate::sys::SDL_BlendMode;
use crate::sys::SDL_ScaleMode;
use crate::sys::SDL_TextureAccess;

/// Contains the description of an error returned by SDL
Expand Down Expand Up @@ -175,6 +176,29 @@ impl TryFrom<u32> for BlendMode {
}
}

#[repr(i32)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be left out, the compiler will then choose to only make it 1 byte wide instead of 4.

Copy link
Author

@jagprog5 jagprog5 Feb 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm following the standard in this repo. Everywhere else does it like this, and it's not up to me to make a sweeping change.

#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
pub enum ScaleMode {
/// nearest pixel sampling. default
Nearest = SDL_ScaleMode::SDL_ScaleModeNearest as i32,
/// linear filtering
Linear = SDL_ScaleMode::SDL_ScaleModeLinear as i32,
/// anisotropic filtering
Best = SDL_ScaleMode::SDL_ScaleModeBest as i32,
}

impl TryFrom<u32> for ScaleMode {
type Error = ();

fn try_from(n: u32) -> Result<Self, Self::Error> {
Ok(match unsafe { transmute(n) } {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge no-no, it will crash badly if n is not a safe value, even though the function is a try_from, which should simply output a Err on invalid value.

Read the implications of transmute with enums and fix this PR, this is not the only time you're using it, and I don't think you should be using it even once.

Copy link
Author

@jagprog5 jagprog5 Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to make this change. Quick question first, should these existing instances be changed as well? My intent was to follow the existing style.

Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked on the playground, they are all big mistakes and should be fixed ASAP:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1f798ba39aef506228d8ecfeeda9b759

Try in debug and release modes, it's wildly different behaviors, and never what's expected

Copy link
Author

@jagprog5 jagprog5 Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in: aa858db

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cobrand friendly ping!

Copy link
Author

@jagprog5 jagprog5 Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cobrand I have addressed cf7d253 one of the job failures.

But the other one is more involved and related to this. Please consider applying the fix listed there to a different PR - I can't run the CI manually to check it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cobrand hello! I noticed that there is some subtle differences between sdl2-sys and sdl3-sys. sdl2-sys causes the casts to be required in this MR. that being said, I'd prefer the scope of this one not to expand too much. Is there anything I can do on my end? I'd love to get this merged in.

crate::sys::SDL_ScaleMode::SDL_ScaleModeNearest => self::ScaleMode::Nearest,
crate::sys::SDL_ScaleMode::SDL_ScaleModeLinear => self::ScaleMode::Linear,
crate::sys::SDL_ScaleMode::SDL_ScaleModeBest => self::ScaleMode::Best,
})
}
}

impl RendererInfo {
pub unsafe fn from_ll(info: &sys::SDL_RendererInfo) -> RendererInfo {
let texture_formats: Vec<PixelFormatEnum> = info.texture_formats
Expand Down Expand Up @@ -2097,6 +2121,27 @@ impl InternalTexture {
}
}

#[doc(alias = "SDL_SetTextureScaleMode")]
pub fn set_scale_mode(&mut self, scale: ScaleMode) {
let ret =
unsafe { sys::SDL_SetTextureScaleMode(self.raw, transmute(scale as u32)) };
if ret != 0 {
panic!("Error setting scale mode: {}", get_error())
}
}

#[doc(alias = "SDL_GetTextureScaleMode")]
pub fn scale_mode(&self) -> ScaleMode {
let mut scale: MaybeUninit<SDL_ScaleMode> = mem::MaybeUninit::uninit();
let ret = unsafe { sys::SDL_GetTextureScaleMode(self.raw, scale.as_mut_ptr()) };
if ret != 0 {
panic!("{}", get_error())
} else {
let scale = unsafe { scale.assume_init() };
ScaleMode::try_from(scale as u32).unwrap()
}
}

#[doc(alias = "SDL_SetTextureAlphaMod")]
pub fn set_alpha_mod(&mut self, alpha: u8) {
let ret = unsafe { sys::SDL_SetTextureAlphaMod(self.raw, alpha) };
Expand Down Expand Up @@ -2440,6 +2485,18 @@ impl<'r> Texture<'r> {
InternalTexture { raw: self.raw }.color_mod()
}

/// Sets an additional color value multiplied into render copy operations.
#[inline]
pub fn set_scale_mode(&mut self, scale: ScaleMode) {
InternalTexture { raw: self.raw }.set_scale_mode(scale)
}

/// Gets the additional color value multiplied into render copy operations.
#[inline]
pub fn scale_mode(&self) -> ScaleMode {
InternalTexture { raw: self.raw }.scale_mode()
}

/// Sets an additional alpha value multiplied into render copy operations.
#[inline]
pub fn set_alpha_mod(&mut self, alpha: u8) {
Expand Down