-
Notifications
You must be signed in to change notification settings - Fork 43
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 visible, opacity and blending mode support #15
Add visible, opacity and blending mode support #15
Conversation
WTF you aren't human - DAMN nice!!! Will take a look tonight or in the morning (I'm on US east coast time) |
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.
This PR is B E A U T I F U L
Left a few very, very minor comments
src/blend.rs
Outdated
use crate::sections::layer_and_mask_information_section::layer::BlendMode; | ||
|
||
// Converts layer opacity property to alpha channel | ||
pub(crate) fn perform_opacity(pixel: &mut [u8; 4], opacity: u8) { |
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.
Idea -> apply_opacity
src/blend.rs
Outdated
|
||
use crate::sections::layer_and_mask_information_section::layer::BlendMode; | ||
|
||
// Converts layer opacity property to alpha channel |
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.
Multiplies the pixel's current alpha by the passed in `opacity`
src/blend.rs
Outdated
|
||
/// blend the two pixels. | ||
/// | ||
/// TODO: Take the layer's blend mode into account when blending layers. Right now |
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.
Seems like this can be removed
src/blend.rs
Outdated
/// `Co = co / αo` | ||
/// | ||
/// *The backdrop is the content behind the element and is what the element is composited with. This means that the backdrop is the result of compositing all previous elements. | ||
pub(crate) fn blend_pixels(top: [u8; 4], bottom: [u8; 4], blend_mode: BlendMode, out: &mut [u8; 4]) { |
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.
🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥
type BlendFunction = dyn Fn(f32, f32) -> f32; | ||
|
||
/// Returns blend function for given BlendMode | ||
fn map_blend_mode(blend_mode: BlendMode) -> &'static BlendFunction { |
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.
🔥 🔥 🔥 🔥 🔥
@@ -253,6 +265,78 @@ impl GroupDivider { | |||
} | |||
} | |||
|
|||
|
|||
/// BlendMode represents blending mode. |
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.
Describes how to blend a layer with the layer below it
// - bit 2 = obsolete; | ||
// - bit 3 = 1 for Photoshop 5.0 and later, tells if bit 4 has useful information; | ||
// - bit 4 = pixel data irrelevant to appearance of document | ||
let visible = cursor.read_u8()? & (1 << 4) != 0; |
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.
🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥
tests/blend.rs
Outdated
@@ -0,0 +1,176 @@ | |||
use psd::Psd; |
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.
🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥 🔥
const BLEND_DIFFERENCE_BLUE_RED_PIXEL: [u8; 4] = [170, 0, 170, 192]; | ||
const BLEND_EXCLUSION_BLUE_RED_PIXEL: [u8; 4] = [170, 0, 170, 192]; | ||
|
||
#[test] |
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.
Sorry to bother, but could you follow the new pattern of adding the run command above every test?
I know most people have editor setups that let them run tests - so this is mainly for people that are new to Rust that don't have that setup yet but would like to contribute.
@@ -33,6 +33,15 @@ fn layer_with_chinese_name() { | |||
psd.layer_by_name("圆角矩形").unwrap(); | |||
} | |||
|
|||
#[test] |
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.
Same here
src/blend.rs
Outdated
BlendMode::Multiply => &multiply, | ||
BlendMode::Screen => &screen, | ||
BlendMode::Overlay => &overlay, | ||
// Here we need Dissolve |
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.
Should we instead add BlendMode::Dissolve => unimplemented!()
etc for all of these?
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.
It can be BlendMode::Dissolve => &dissolve
and
fn dissolve(color_b: f32, color_s: f32) -> f32 {
unimplemented!()
}
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.
Even betteer
@tdakkota still interested in this PR? I don't mind taking it over if you've moved on from it - thanks so much for everything you already nailed down here! |
Seems like you already addressed all of the feedback and all that's needed is a rebase on the latest master, or am I missing something? |
@tdakkota thanks for your incredible work on this! |
#14