fix: FilterOptions type error

[oubenruing]

Fixes #480 Make the option`s interface of filters extends from FilterOptions.

[bigtimebuddy]

In general, this is a good change. While this PR satisfies the types, it does not, for instance, actually pass any of those parameters to constructor's super(). Likely need more work to do that.

[oubenruing]

Indeed, I overlooked that. How about this modification?

export class AdjustmentFilter extends Filter
{

    ......

    constructor(options?: AdjustmentFilterOptions)
    {
        options = { ...AdjustmentFilter.DEFAULT_OPTIONS, ...options };

        const gpuProgram = GpuProgram.from({
            vertex: {
                source: wgslVertex,
                entryPoint: 'mainVertex',
            },
            fragment: {
                source,
                entryPoint: 'mainFragment',
            },
        });

        const glProgram = GlProgram.from({
            vertex,
            fragment,
            name: 'adjustment-filter'
        });

       super({
+          ...options,
            gpuProgram,
            glProgram,
            resources: {
                adjustmentUniforms: {
                    uGamma: { value: options.gamma, type: 'f32' },
                    uContrast: { value: options.contrast, type: 'f32' },
                    uSaturation: { value: options.saturation, type: 'f32' },
                    uBrightness: { value: options.brightness, type: 'f32' },
                    uColor: {
                        value: [
                            options.red,
                            options.green,
                            options.blue,
                            options.alpha,
                        ],
                        type: 'vec4<f32>',
                    },
                }
            },
        });

        this.uniforms = this.resources.adjustmentUniforms.uniforms;
    }
    ......
}

[bigtimebuddy]

We probably want more something like this:

const { contrast, gamma, saturation, brightness, red, blue, green, alpha, ...rest } = { ...AdjustmentFilter.DEFAULT_OPTIONS, ...options };

//...

super({
  glProgram,
  gpuProgram,
  resources,
  ...rest,
})

This is a bigger change, but using the rest args allows us to only pass in the relevant props to super, as opposed to everything in options. I also prefer adding rest args last in super as it allows a user to override things like the programs or resources for debugging purposes.

[oubenruing]

Got it, do you mind if I go ahead and finish this part?

[bigtimebuddy]

Please! And thank you! This will be great for setting resolution and other base Filter props.

[bigtimebuddy]

@oubenruing might you be able to take a look at this? If you don't no worries, I'd be happy to wrap it up. Let me know.

[oubenruing]

Hi @bigtimebuddy. Sorry for the delayed reply. I can take care of it. Would it be alright if you give me a day or two? I've been busy with other things recently.

[oubenruing]

Hi @bigtimebuddy , In the latest commit, I've destructured options and passed the rest parameters to super. There are currently two points I would like to seek your opinion.

---

1. Object.assign in the custructor

Most filters use Object.assign(this, options); at the end of the constructor to add properties. Do you think I should do anything else here to exclude the rest parameters that are passed to super?

---

2. About DEFAULT_OPTIONS`s type

DEFAULT_OPTIONS has an explicit type, but the fields in the options type are all optional.

// ...
export interface AdjustmentFilterOptions extends FilterOptions
{
    gamma?: number;
    contrast?: number;
  //...
}

This could lead to the destructured properties being undefined, even though DEFAULT_OPTIONS clearly provides values.

I was thinking it might be a good idea to remove the type from DEFAULT_OPTIONS so that TypeScript can better infer the types. What do you think?

/** Default values for options. */
-    public static readonly DEFAULT_OPTIONS : AdjustmentFilterOptions = {
+   public static readonly DEFAULT_OPTIONS  = {
        gamma: 1,
        contrast: 1,
        saturation: 1,
        brightness: 1,
        red: 1,
        green: 1,
        blue: 1,
        alpha: 1,
    };

[bigtimebuddy]

1. I think the Object.assign should be explicit, like Object.assign(this, { something }) instead of Object.assign(this, options) This would avoid any over lap with options and the filter.
2. I'm okay with removing the type on the DEFAULT_OPTIONS. That makes sense.

[oubenruing]

@bigtimebuddy Changes are done. Please review. Thanks!