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

Nodes: Add SepiaNode #28961

Merged
merged 6 commits into from
Jul 26, 2024
Merged

Nodes: Add SepiaNode #28961

merged 6 commits into from
Jul 26, 2024

Conversation

cmhhelgeson
Copy link
Contributor

@cmhhelgeson cmhhelgeson commented Jul 24, 2024

Related issue:

Description

Port of GLSL Sepia Shader to node system.

postProcessing = new THREE.PostProcessing( renderer );
const scenePass = pixelationPass( scene, camera, effectController.pixelSize, 
effectController.normalEdgeStrength, effectController.depthEdgeStrength );
// scenePass.getTextureNode() returns pixelation.textureNode, renderOutput returns full effect
const sepiaPass = sepia( { color: scenePass.renderOutput() } );
postProcessing.outputNode = mix( scenePass, sepiaPass, effectController.mixValue );

Live Example

Copy link

github-actions bot commented Jul 24, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
684 kB (169.4 kB) 684 kB (169.4 kB) +0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
460.9 kB (111.2 kB) 460.9 kB (111.2 kB) +0 B

@cmhhelgeson cmhhelgeson marked this pull request as draft July 24, 2024 23:26
@cmhhelgeson cmhhelgeson marked this pull request as ready for review July 25, 2024 00:28
@sunag
Copy link
Collaborator

sunag commented Jul 25, 2024

I think serpia could be introduced without needing a texture as a parameter, maybe just a tslFn could solve the issue.

@sunag
Copy link
Collaborator

sunag commented Jul 25, 2024

Could amount be replaced by a mix externally? Maybe we don't need it as a parameter.

@cmhhelgeson
Copy link
Contributor Author

Could amount be replaced by a mix externally? Maybe we don't need it as a parameter.

I think that makes sense if we're just strictly replicating the sepia effect, but for portability between the old and new shaders I think it makes sense to keep in cases where people are deliberately pushing the value to achieve specific effects.

I think serpia could be introduced without needing a texture as a parameter, maybe just a tslFn could solve the issue.

Were you think something more like the colorAdjustmentNode where the tslFn is only taking in a color? Or just export a tslFn directly from the file?

@sunag
Copy link
Collaborator

sunag commented Jul 25, 2024

I think that makes sense if we're just strictly replicating the sepia effect, but for portability between the old and new shaders I think it makes sense to keep in cases where people are deliberately pushing the value to achieve specific effects.

Developers didn't have support for this level of abstraction before, so we had intensity and amount everywhere, but that's no longer necessary, adding them nowadays would just be adding extra code and operations.

Even if this were preserved and for the sake of practicality, the feeling when looking at the code is that it should be done with mix, multiplying all the values ​​of the serpia vector seems more costly for gpu..

Were you think something more like the colorAdjustmentNode where the tslFn is only taking in a color? Or just export a tslFn directly from the file?

I think no class is needed for this, just export tslFn, like:

export const serpia = /*@__PURE__*/ tslFn( ( [ color ] ) => {

} );

@cmhhelgeson
Copy link
Contributor Author

Developers didn't have support for this level of abstraction before, so we had intensity and amount everywhere, but that's no longer necessary, adding them nowadays would just be adding extra code and operations.

That makes sense, thank you for that explanation, and I'll adjust the code accordingly.

@Mugen87 Mugen87 added this to the r168 milestone Jul 26, 2024
@Mugen87 Mugen87 merged commit 76920df into mrdoob:dev Jul 26, 2024
12 checks passed
@sunag sunag mentioned this pull request Jul 26, 2024
brunosimon pushed a commit to brunosimon/three.js that referenced this pull request Jul 28, 2024
* add SepiaNode

* add node to export

* fix

* condense sepia into tslFn

* remove SepiaNode from Node.js

* Update SepiaNode.js

---------

Co-authored-by: Michael Herzog <[email protected]>
@cmhhelgeson cmhhelgeson deleted the wegpu_sepia_node branch August 5, 2024 16:50
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.

3 participants