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 8 commits into
base: master
Choose a base branch
from
Open

Add texture scale mode #1444

wants to merge 8 commits into from

Conversation

jagprog5
Copy link
Contributor

@jagprog5 jagprog5 commented Dec 3, 2024

otherwise texture copy to a smaller destination can look quite bad

@jagprog5
Copy link
Contributor Author

jagprog5 commented Jan 8, 2025

@Cobrand

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
Contributor 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
Contributor 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
Contributor 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
Contributor 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
Contributor 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.

@jagprog5
Copy link
Contributor Author

jagprog5 commented Feb 5, 2025

@AngryLawyer

@jagprog5
Copy link
Contributor Author

jagprog5 commented Feb 5, 2025

Relates #1388

#[repr(i32)]
#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
pub enum ScaleMode {
/// nearest pixel sampling. default
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not include this test, as it requires creating a texture creator, which I doubt would work with the CI.

Of note, the default is Nearest for SDL2, but Linear for SDL3.

use sdl2::{render::ScaleMode, surface::Surface};

extern crate sdl2;
extern crate sdl2_sys;

#[test]
fn test_scale_mode() {
    let sdl_context = sdl2::init().unwrap();
    let sdl_video_subsystem = sdl_context.video().unwrap();
    let window = sdl_video_subsystem
    .window(
        "tester",
        100,
        100,
    )
    .build()
    .unwrap();

    let canvas = window.into_canvas().build().unwrap();
    let texture_creator = canvas.texture_creator();
    let surface = Surface::new(1, 1, sdl2::pixels::PixelFormatEnum::ARGB8888).unwrap();

    let mut texture = texture_creator.create_texture_from_surface(surface).unwrap();

    // checking the default scale mode
    assert_eq!(texture.scale_mode(), ScaleMode::Nearest);
    // check set get
    texture.set_scale_mode(ScaleMode::Nearest);
    assert_eq!(texture.scale_mode(), ScaleMode::Nearest);
}

@Cobrand
Copy link
Member

Cobrand commented Feb 18, 2025

I feel like there is probably a prettier way to match everything to their sdl2-sys counterparts without doing x if x == CONST as u32 but I can't find it. In the meantime it's at least sound, so I'll wait for the workflow to finish and merge.

@antonilol
Copy link
Contributor

I feel like there is probably a prettier way to match everything to their sdl2-sys counterparts without doing x if x == CONST as u32 but I can't find it. In the meantime it's at least sound, so I'll wait for the workflow to finish and merge.

I don't know, because the constants are enum variants the as casting is needed. A macro can always make it look nicer (and still result in the same compiled code).

@jagprog5
Copy link
Contributor Author

jagprog5 commented Feb 19, 2025

@Cobrand @antonilol for comparison the equivalent change working with sdl3-rs: https://github.com/vhspace/sdl3-rs/pull/92/files

that being said, I don't want this MR to get out of scope - this can be a follow up issue

@jagprog5
Copy link
Contributor Author

jagprog5 commented Feb 19, 2025

the difference is:

sdl3-sys

Auto generated bindings gave a struct: here:

pub struct SDL_EventType(pub Uint32);
impl SDL_EventType {
    /// Unused (do not remove)
    pub const FIRST: Self = Self(0);
    ...

sdl2-sys

Auto generated bindings gave an enum:

pub enum SDL_EventType {
    #[doc = "< Unused (do not remove)"]
    SDL_FIRSTEVENT = 0,
    ...

Copy link
Contributor

@antonilol antonilol left a comment

Choose a reason for hiding this comment

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

UB is gone but I don't like x if x == <constant> as u32 => ... match arms, a macro can be used to fix this. (I don't think there is a better way without a macro)

I checked with compiler explorer and Rust seems to have no issue compiling this compared to what it was before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants