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

Src: encodings_fragment -> colorspace_fragment #26206

Merged
merged 3 commits into from
Jun 10, 2023

Conversation

WestLangley
Copy link
Collaborator

... plus related changes.

@WestLangley WestLangley added this to the r154 milestone Jun 7, 2023
@github-actions
Copy link

github-actions bot commented Jun 7, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
640 kB (158.8 kB) 640.3 kB (158.9 kB) +274 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
433.4 kB (105 kB) 433.6 kB (105.1 kB) +274 B

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 7, 2023

I'm a bit worried about this change. The encodings_fragment shader chunk is frequently used in custom shader materials. With this change, all these material definitions will be broken.

So I wonder if the renaming is worth the breakage.

I think it would be best to deprecate the shader chunk first and then log a warning for ten releases. Maybe we can try to implement a small deprecation check in WebGLProgram that checks for outdated chunks and performs a resolution to newer ones. The includeReplacer() function could be the right spot for this. Ideally it would log a warning and then map encodings_fragment to color_space_fragment.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 7, 2023

This should do it:

const shaderChunkMap = new Map( [
	[ 'encodings_fragment', 'color_space_fragment' ], // @deprecated, r154
	[ 'encodings_pars_fragment', 'color_space_pars_fragment' ] // @deprecated, r154
] );

function includeReplacer( match, include ) {

	let string = ShaderChunk[ include ];

	if ( string === undefined ) {

		const newInclude = shaderChunkMap.get( include );

		if ( newInclude !== undefined ) {

			string = ShaderChunk[ newInclude ];
			console.warn( 'THREE.WebGLRenderer: Shader chunk "%s" has been deprecated. Use "%s" instead.', include, newInclude );

		} else {

			throw new Error( 'Can not resolve #include <' + include + '>' );

		}

	}

	return resolveIncludes( string );

}

@WestLangley
Copy link
Collaborator Author

@Mugen87 Good suggestion, thanks. I implemented it in this PR.

@mrdoob
Copy link
Owner

mrdoob commented Jun 8, 2023

I think colorspace_fragment and colorspace_pars_fragment would fit better with the rest of the shaderchunk names.

@WestLangley
Copy link
Collaborator Author

WestLangley commented Jun 8, 2023

We decided to use the term "color space", not "colorspace".

Hence, the variable name became colorSpace, not colorspace.

Consequently, I chose to use color_space_fragment, not colorspace_fragment.

@WestLangley WestLangley changed the title Src: encodings_fragment -> color_space_fragment Src: encodings_fragment -> colorspace_fragment Jun 9, 2023
@WestLangley WestLangley force-pushed the dev-encoding_to_color_space branch from f0fccb9 to bc8833c Compare June 9, 2023 17:05
@mrdoob mrdoob merged commit f8d580c into mrdoob:dev Jun 10, 2023
@mrdoob
Copy link
Owner

mrdoob commented Jun 10, 2023

@WestLangley Thanks! 🙏

Also great to finally have a migration helper for these 🥲

@WestLangley WestLangley deleted the dev-encoding_to_color_space branch June 10, 2023 10:00
@WestLangley
Copy link
Collaborator Author

This PR requires updated build files. Otherwise, the modified examples are out-of-sync with core.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 11, 2023

e31a6d4 🚀

@rndexe rndexe mentioned this pull request Aug 8, 2024
1 task
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