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.

Sign up for GitHub

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

Improve variable naming for RgbaSurface. Remove unnecessary comments. #6

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Improve variable naming for RgbaSurface. Remove unnecessary comments. #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2772a29
Add comments explaining RgbaSurface's usage
KYovchevski Jun 7, 2022
bc4a781
Improve variable naming. Remove obvious comments
KYovchevski Jun 7, 2022
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
2 changes: 1 addition & 1 deletion examples/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ fn main() {
let surface = intel_tex_2::RgbaSurface {
width,
height,
stride: width * 4,
stride_in_bytes: width * 4,
data: &rgba_img,
};

Expand Down
2 changes: 1 addition & 1 deletion src/astc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ pub fn compress_blocks_into(settings: &EncodeSettings, surface: &RgbaSurface, bl
let mut surface = kernel_astc::rgba_surface {
width: surface.width as i32,
height: surface.height as i32,
stride: surface.stride as i32,
stride: surface.stride_in_bytes as i32,
ptr: surface.data.as_ptr() as *mut u8,
};

Expand Down
2 changes: 1 addition & 1 deletion src/bc1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub fn compress_blocks_into(surface: &RgbaSurface, blocks: &mut [u8]) {
let mut surface = kernel::rgba_surface {
width: surface.width as i32,
height: surface.height as i32,
stride: surface.stride as i32,
stride: surface.stride_in_bytes as i32,
ptr: surface.data.as_ptr() as *mut u8,
};

Expand Down
2 changes: 1 addition & 1 deletion src/bc3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub fn compress_blocks_into(surface: &RgbaSurface, blocks: &mut [u8]) {
let mut surface = kernel::rgba_surface {
width: surface.width as i32,
height: surface.height as i32,
stride: surface.stride as i32,
stride: surface.stride_in_bytes as i32,
ptr: surface.data.as_ptr() as *mut u8,
};
Comment on lines 23 to 28
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why every file is manually converting from RgbaSurface to kernel::RgbaSurface?

Copy link
Author

Choose a reason for hiding this comment

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

That's taken from Graham's code. kernel::rgba_surface appears to be part of the generated code, so adding an impl for it seems like it would be hacky, since you'll be relying on it matching the generated Rust code for the struct.

I assume this might be the reason it also isn't straight up using kernel::rgba_surface as part of the interface. I think it would be more user friendly to have a struct that is unlikely to change between versions, and if you have changes to the kernel struct, you only change the conversion rather than the user having to change their code.

Copy link
Member

Choose a reason for hiding this comment

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

Generated code is imported into this crate, so you should be able to safely implement an impl From<RgbaSurface> for kernel::rgba_surface and do away with a bunch of duplicated code.


Expand Down
2 changes: 1 addition & 1 deletion src/bc6h.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub fn compress_blocks_into(settings: &EncodeSettings, surface: &RgbaSurface, bl
let mut surface = kernel::rgba_surface {
width: surface.width as i32,
height: surface.height as i32,
stride: surface.stride as i32,
stride: surface.stride_in_bytes as i32,
ptr: surface.data.as_ptr() as *mut u8,
};
let mut settings = kernel::bc6h_enc_settings {
Expand Down
2 changes: 1 addition & 1 deletion src/bc7.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub fn compress_blocks_into(settings: &EncodeSettings, surface: &RgbaSurface, bl
let mut surface = kernel::rgba_surface {
width: surface.width as i32,
height: surface.height as i32,
stride: surface.stride as i32,
stride: surface.stride_in_bytes as i32,
ptr: surface.data.as_ptr() as *mut u8,
};
let mut settings = kernel::bc7_enc_settings {
Expand Down
2 changes: 1 addition & 1 deletion src/etc1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ pub fn compress_blocks_into(settings: EncodeSettings, surface: &RgbaSurface, blo
let mut surface = kernel::rgba_surface {
width: surface.width as i32,
height: surface.height as i32,
stride: surface.stride as i32,
stride: surface.stride_in_bytes as i32,
ptr: surface.data.as_ptr() as *mut u8,
};
let mut settings = kernel::etc_enc_settings {
Expand Down
8 changes: 7 additions & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,18 @@ pub mod bc6h;
pub mod bc7;
pub mod etc1;

/// Describes a 2D uncompressed little endian RGBA image.
#[derive(Debug, Copy, Clone)]
pub struct RgbaSurface<'a> {
/// The pixel data for the image.
/// The data does not need to be tightly packed, but if it isn't, stride must be different from `width * 4`.
///
/// Expected to be at least `stride * height` bytes long.
Copy link
Member

Choose a reason for hiding this comment

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

Nice on adding bytes long :)

Copy link
Member

Choose a reason for hiding this comment

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

change stride to stride_in_bytes :)

pub data: &'a [u8],
pub width: u32,
pub height: u32,
pub stride: u32,
/// The stride between the rows of pixels, in bytes.
Copy link
Member

@MarijnS95 MarijnS95 Jun 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think there's no need to prefix with The here, same for the other doc-comments?

pub stride_in_bytes: u32,
}

#[inline(always)]
Expand Down