set_normal() w/o set_albedo()

[virtualritz]

I just spent an hour banging my head against the wall because I missed this small part from the OIDN docs:

normal [...] requires setting the albedo image too

If you specify normal only you get a black image from OIDN. No error, no warning, just black. :)

Specifying albedo only and no normal is fine btw.

I think it would improve the UX of this crate if some check would be performed that causes a panic or a build failure if normal was set but albedo wasn't.

At the very least a big, fat warning should be added to the docs for set_normal().

[Twinklebear]

Ah sorry about that, what I think would actually work well (and cover #5 which I've been slacking on...) is to change to have different execute methods so it's not possible to misuse the library:

execute(&mut self, color: &[f32], output: &mut [f32])
execute(&mut self, color: &[f32], normal: &[f32], albedo: &[f32], output: &mut [f32])

[Twinklebear]

This is now fixed in release 1.2.0 https://github.com/Twinklebear/oidn-rs/releases/tag/1.2.0 , I've changed the API for execute to only accept valid configurations for the filter:

    pub fn execute(&mut self, color: &[f32], output: &mut [f32]) -> Result<(), FilterError> {}

    pub fn execute_with_albedo(
        &mut self,
        color: &[f32],
        albedo: &[f32],
        output: &mut [f32],
    ) -> Result<(), FilterError> {}

    /// Normal requires albedo to be present
    pub fn execute_with_albedo_normal(
        &mut self,
        color: &[f32],
        albedo: &[f32],
        normal: &[f32],
        output: &mut [f32],
    ) -> Result<(), FilterError> {}

[virtualritz]

Thanks. Another option that kept the API more compact would be to expose/rename execute_filter() as pub execute() and add something like this to the top of the function:

if None == albedo && None != normal {
    compile_error!("oidn_rs::RayTracing::execute_filter(): specifying normal requires specifying albedo.")
}

[virtualritz]

I meant the above comment to read: that way you could keep the old builder API which was more canonical Rust, I guess.

[Twinklebear]

Yeah, I was debating adding the error check but I think it's nicer to have the API prevent you from setting up an invalid configuration in the first place.