Skip to content
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

Extend Texture Effect to support opacity #271

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vegancat
Copy link

This PR extends the Texture Effect to support a new parameter called opacity. this parameter is used when creating TextureEffect to set the opacity of BlendMode. The idea and practice comes from the example provided from postproccessing library.

the reference: https://github.com/pmndrs/postprocessing/blob/e758f65ef2277d006c9785d8148926bf78235441/demo/src/demos/TextureDemo.js#L194

Extend the <Texture /> Effect to support opacity
fix the way we check for availability of opacity.

* edge case: opacity==0 would be considered absent in previous method
Comment on lines 25 to 31
const effect = useMemo(() => {
let tEffect = new TextureEffect({ ...props, texture: t || texture });
if (typeof opacity === "number") {
tEffect.blendMode.opacity.value = opacity;
}
return tEffect;
}, [props, t, texture, opacity])
Copy link
Member

@CodyJasonBennett CodyJasonBennett Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer setting this as a prop instead so we don't recreate the effect and its resources when opacity changes (e.g. <primitive object={effect} blendMode-opacity-value={opacity ?? 1} />).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I didn't know about such possibility. I'd implement that way and commit it then

Update the way opacity is applied

* instead of recreating an effect every time opacity changes we feed it to the final primitive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants