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

Implement safer pixel data functions #322

Closed

Conversation

dogunbound
Copy link
Contributor

@dogunbound dogunbound commented Feb 18, 2024

I believe that it is unnecessary for the user to use unsafe functions when it can easily be implemented with safe functionality. This is an example of doing it safely with a small abstraction. Rust is all about safe code, not unsafe code. Such a simple abstraction should be hidden from the user

Note not inside of commit:
I haven't tested it. I'll test it soon and leave a comment that I have tested it's functionality.

This will be a breaking change, but I believe it is for the better. People using these functions will have to change their code and decide whether they want to use the unchecked or standard version of the function.

@dogunbound
Copy link
Contributor Author

Ok, i might still be testing this by the time you see this @crumblingstatue

I'm uploading some debug code, so my cargo build can see it off my branch

@dogunbound dogunbound force-pushed the safer_image_functions branch 5 times, most recently from ea1dda3 to 2b5f38f Compare February 19, 2024 05:34
@dogunbound
Copy link
Contributor Author

Finally done testing. Here is the test code i used:

use sfml::graphics::{Color, Image};

fn main() {
    let mut image = Image::new(64, 64);

    for x in 0..64 {
        for y in 0..64 {
            match image.set_pixel(x, y, Color::RED) {
                Ok(_) => (),
                Err(e) => {
                    let error_message = format!("{}", e);
                    println!("{}", error_message);
                }
            };

            match image.pixel_at(x, y) {
                Some(_) => (),
                None => {
                    println!("no pixel at x:{} y:{}", x, y);
                }
            }
        }
    }
}

Copy link
Collaborator

@crumblingstatue crumblingstatue left a comment

Choose a reason for hiding this comment

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

Overall I approve of adding safe variants of these functions.
Just a few nitpicks.

src/graphics/image.rs Outdated Show resolved Hide resolved
src/graphics/image.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@crumblingstatue crumblingstatue left a comment

Choose a reason for hiding this comment

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

Some more nitpicks, sorry.

src/graphics/image.rs Outdated Show resolved Hide resolved
src/graphics/image.rs Outdated Show resolved Hide resolved
src/graphics/image.rs Outdated Show resolved Hide resolved
src/graphics/image.rs Outdated Show resolved Hide resolved
src/graphics/image.rs Outdated Show resolved Hide resolved
I believe that it is unnecessary for the user to use unsafe functions
when it can easily be implemented with safe functionality. This is an
example of doing it safely with a small abstraction. Rust is all about
safe code, not unsafe code. Such a simple abstraction should be hidden
from the user
@dogunbound
Copy link
Contributor Author

Cargo fmt caught something in src/window/context.rs also. I added it as a seperate commit. Thought I might as well include it into this PR since it was already up.

@crumblingstatue
Copy link
Collaborator

Thank you for your contribution! I merged it manually in 3c732e8

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.

2 participants