609 channel picker with shader #624
Conversation
|
Colours don't seem to be accurate: colour.webmThis is confirmed by both oculante as well as hyprpicker. |
Thank you! I see what I missed here. Left: build from master, Right: build from my PR - no difference visible for me :) |
Apologies, this was a late night review. I think I was confused by the hex value and other colour values being wrong. I was under the assumption that this would change the colour to update as well to that channel. Thinking about this now it wouldn't really make sense since it's just greyscale. |
|
The alpha channel seems to break the black border of the zoom window. picker.webm |
Thank you for this interesing bug. That's indeed a Problem, cause the shader doesn't do a distinction between border and normal texture. @Stoppedpuma fixed it :) |
There was a problem hiding this comment.
Functionally, this works fine. I'll leave actual code review to @woelper.
|
@woelper is it ok to merge, or do you want to have a look, too? |
|
This is great work! I hope I have some time to review tonight! |
|
What do you think if we (in the future of course) always render with a display shader that has switches depending on the image or operation? |
Now there's always a shader in use for rendering. I only adjust a transformation matrix for the color vectors. |
src/texture_wrapper.rs
Outdated
| fn check_union_buffer_creation(buffer_result:Result<Buffer,String>)->Buffer{ | ||
| match buffer_result { | ||
| Ok(buffer) => buffer, | ||
| Err(error) => panic!("Problem generating union buffer: {error:?}"), |
There was a problem hiding this comment.
I think this is useless: You check the result with match, but you panic at the Err. This would be equivalent to just using unwrap() or expect("Problem generating union buffer"). The app will terminate at a problem.
I would suggest to remove this function entirely.
Here are some things I wish I knew earlier about error handling:
let res: Result<std::fs::File, std::io::Error> = std::fs::File::create("foo");
// at this point, the above could have worked or not
// terminate program if this fails, for example if there is no sane way to continue
// This is hightly discouraged
let f = res.unwrap();
// if you are inside a function that itself returns a result, you can bubble the error
// upwards with `?`. The error types need to be compatible, and `anyhow` solves this.
// You need to handle the result one level higher.
fn can_fail() -> anyhow::Result<std::fs::File> {
let res = std::fs::File::create("foo")?;
Ok(res)
}
// It does not make much sense in a small function like this, usually
// you have a larger enclosing function that has multiple steps that can fail.
// if you just want to execute the command and don't care if it succeeds
_ = res;
// more readable would be to call this directly:
_ = std::fs::File::create("foo");
// In the case you don't need the result but still want to show a log output you
// can just operate on the Err part - this saves you a match:
_ = std::fs::File::open("foobar")
.map_err(|e| error!("This failed: {e}"));
// If you don't care about the error, but still want to deal with potential absence later,
// similar to your case:
let mut maybe_file: Option<std::fs::File> = res.ok();
// you can combine this with the above of course:
let mut maybe_file: Option<std::fs::File> = res
.map_err(|e| error!("This failed: {e}"))
.ok();There was a problem hiding this comment.
At least we would see the error in the console, that's what I thought.
Will look deeper when I got the time.
I see what you're thinking there and understand why I should just create a warning instead of panicking.
|
@woelper I now completely reworked the error handling. I use the Result type whenever there is a notan call in a function which has an Result return type. If approved I would now update the calls to match the Result and displaying a warning if theres an Error. What do you think? Thank you for "nitpicking", I learned many things about error handling with that. In my opinion I should have changed this way earlier. |
| //First: try to update an existing texture | ||
| if let Some(tex) = &mut self.current_texture { | ||
| if tex.width() as u32 == img.width() | ||
| && tex.height() as u32 == img.height() | ||
| && img.color() == tex.image_format | ||
| { | ||
| debug!("Re-using texture as it is the same size and type."); | ||
| tex.update_textures(gfx, img); | ||
| return; | ||
| match tex.update_textures(gfx, img) { |
There was a problem hiding this comment.
Not required, just a hint:
you can just use the Err if you ignore the Ok() part in match:
if let Err(error) = tex.update_textures(gfx, img) {
self.clear();
error!("{error}");
return Err(error);
}instead of
match tex.update_textures(gfx, img) {
Ok(_) => {}
Err(error) => {
self.clear();
error!("{error}");
return Err(error);
}
}
Super nice, thank you for your work!! |
No description provided.