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: RenderOutputNode name change #29163

Open
WestLangley opened this issue Aug 18, 2024 · 2 comments
Open

Nodes: RenderOutputNode name change #29163

WestLangley opened this issue Aug 18, 2024 · 2 comments
Milestone

Comments

@WestLangley
Copy link
Collaborator

WestLangley commented Aug 18, 2024

Description

RenderOutputNode converts scene-referred linear light values to display-referred code values, and then converts the code value to a color space appropriate for the display.

The current name is derived from OutputPass, but I think we can do better.

Solution

I suggest the following nomenclature changes:

  1. RenderOutputNode => DisplayNode, DisplayViewNode, or ViewNode
  2. export const renderOutput => display, displayView, or view

Given that the transforms in this node are specific to a display, it seems reasonable to include Display in the name.

A "view", I believe, can include additional post-processing. That may be appropriate if the node were to include other optional transforms at some point.

//

For clarity, this is the current nomenclature:

scenePass.setMRT( mrt( {
	output: output,
	...
} ) );

const outputPass = scenePass.getTextureNode( 'output' );

postProcessing.outputNode = outputPass.renderOutput();

I recommend this nomenclature:

scenePass.setMRT( mrt( {
	output: output,
	...
} ) );

const outputPass = scenePass.getTextureNode( 'output' );

const viewPass = outputPass.displayView();

postProcessing.outputNode = viewPass;

Alternatives

None.

Additional context

No response

@WestLangley WestLangley added this to the r168 milestone Aug 18, 2024
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 18, 2024

TBH, I find the output nomenclature appropriate.

If you want to change to display, you have to do this uniformly at least for the WebGPURenderer related code. So for example Renderer.outputColorSpace should be Renderer.displayColorSpace.

But right now, I don't see enough arguments to go through all these name changes.

@mrdoob mrdoob modified the milestones: r168, r169 Aug 30, 2024
@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 15, 2024

I'm not opposed to a change, but I'm having trouble convincing myself of the improvement.

Perhaps worth noting, the closest term in WebGL would be gl.drawingBufferColorSpace, and in WebGPU that's GPUCanvasConfiguration#colorSpace. In neither case is it necessarily representative of the display or view. The browser or OS may apply additional mappings, or the application may intend to do something else with the canvas. So I'm nervous of committing to display/view terminology in these lower-level APIs. In an application like the three.js Editor, the terms "Display" or "View" would seem entirely appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
@mrdoob @WestLangley @donmccurdy @Mugen87 and others