Consider EffectPass.needsUpdate flag

[donmccurdy]

Is your feature request related to a problem?

For three-filmic, I'm extending EffectPass with a custom pass that (a) applies the user's specified effects, then (b) applies a selected view transform / tone map. The filmic view transforms are more complex than those built into three.js — e.g. involving LUTs — and several internal Effects are used to structure them. This makes maintenance much easier, as I can reuse LUTPass rather than rewriting it into a more complex pass.

The code is working, but there are some wrinkles in the API that I'd like to smooth out:

const filmicPass = new FilmicPass(camera, ...userCustomEffects);
filmicPass.view = View.FILMIC;
filmicPass.lookLUT = await lut1DLoader.loadAsync('path/to/lut_1d.cube');
filmicPass.desatLUT = await lut3DLoader.loadAsync('path/to/lut_3d.cube');
filmicPass.build();

The .build() method makes a few internal updates to this.effects and then calls this.recompile(). Several issues here:

1. Would this be the only pass with a required .build() method? Might confuse users.
2. We could call .build() automatically when a property changes, but then we'll do a lot of extra work, e.g. above when setting three properties in a row.
3. Within .build(), I have to check whether this.renderer exists yet before calling this.recompile() or an error is thrown. this.renderer is a deprecated property.

Describe the solution you'd like

I think my ideal solution would be a dirty flag, and for Pass.js or EffectPass.js to recompile just before rendering if that flag is true:

filmicPass.needsUpdate = true;

This way we only recompile once, and there's no need for my custom pass to have internal knowledge of whether it's safe to call .recompile() or not. The needsUpdate property could be protected rather than public if that's preferable.

Describe alternatives you've considered

Alternatively, perhaps some kind of onBeforeRender hook that the pass can implement, which will be called before rendering giving the pass a chance to recompile itself if needed.

[vanruesc]

1. Would this be the only pass with a required .build() method? Might confuse users.

As an alternative, you could override recompile which is already part of the public API.

2. We could call .build() automatically when a property changes, but then we'll do a lot of extra work, e.g. above when setting three properties in a row.

Effect merging/compilation is pretty fast. I'd probably go with this approach until performance becomes an issue. At that point it'd be easy to switch to a dirty flag solution.

3. Within .build(), I have to check whether this.renderer exists yet before calling this.recompile() or an error is thrown. this.renderer is a deprecated property.

That's a bug; verifyResources should check if the renderer is null.

I think my ideal solution would be a dirty flag

The EffectPass used to use a private needsUpdate flag to perform material recompilations in the render method, but I didn't like the approach and changed it. Now Effects dispatch change events to trigger immediate recompilations. This basically works like point 2. If you'd like to use the needsUpdate approach, you could implement it in FilmicPass by overriding the render method:

render(renderer, inputBuffer, outputBuffer, deltaTime, stencilTest) {

    if(this.needsUpdate) {

        this.recompile();

    }

    super.render(renderer, inputBuffer, outputBuffer, deltaTime, stencilTest);

}

I just realized that I probably shouldn't have made effects protected. The EffectPass listens for change events on all effects and if those effects are removed from the effects list, they will still dispatch events to the pass. I could use a second array to check which ones were removed in updateMaterial, but I'd rather not :upside_down_face: Since this is still fresh, I think it should be fine to revert the visibility change in favor of a setEffects method that properly unregisters the pass from old effects.

[donmccurdy]

Thanks! This all sounds good to me. No problem about reverting the visibility change and switching to setEffects (or similar).

[donmccurdy]

No remaining changes needed here — thanks!