-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
enable easier modification of image samplers in image loaders #22042
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clicking approve, but added a couple of optional suggestions/nitpicks.
I'm mildly negative on lumping mipmap_filter in with other filters since it's kinda orthogonal, but not a big deal. I guess "set everything to linear" is the most common case.
For bonus points, could also add anisotropy?
pub fn with_anisotropic_filter(&mut self, anisotropy_clamp: u16) -> &mut Self {
self.with_filter(ImageFilterMode::Linear);
self.anisotropy_clamp = anisotropy_clamp;
self
}| } | ||
|
|
||
| /// Returns this sampler descriptor with a new `ImageFilterMode` min, mag, and mipmap filters | ||
| #[inline] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[inline] |
I don't think inline is appropriate here. I respect that some other functions in the struct are inline, but they're probably from the olden days when inline was more necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're probably right. But out of curiosity, the compiler wouldn't inline this since it would always be used across a crate boundary right (without LTO)? So if this were being applied on a huge set of images it might be slower without the inline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was true in the past, but since Rust 1.75 (2023/12) smaller functions are automatically tagged as cross-crate inlineable even without LTO.
So my personal preference is to let the compiler decide for these kinds of cases. But I don't think it's a big deal if the function is small and someone wants to add an inline hint to make sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to match the rest of the file for this PR and do an inline removal on all of these in a future PR instead.
Co-authored-by: Greeble <[email protected]>
The `with_` prefix is mostly used for copies, which these functions don't do. The `set_` prefix has precedence for returning `&mut App`.
|
I have finished addressing review comments, and it is ready for final review |
Objective
Previously, to modify a sampler when loading an image with settings it was required to set all fields individually. Much of the time, these settings are all the same so to enable repeating textures you have to set the address_mode 3 times. To set filters you have to set them another 3 times.
Solution
Add two new helpers, loosely modeled after the
Transform::with_<field>functions. Further modifications can still be made to the descriptor for use cases which want to set one of the three fields differently, or additionally set anisotropy_clamp, etc.Testing
New test in bevy_image
Showcase
This PR now includes:
set_filterset_address_modeset_anisotropic_filter