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: output_fragment -> opaque_fragment #26278

Merged
merged 1 commit into from
Jun 17, 2023

Conversation

WestLangley
Copy link
Collaborator

"Output pass" is now the term we use for a device-specific, tone-mapping/color-space transform.

Consequently, I think output_fragment is misnamed; opaque_fragment seems reasonable to me.

I have included a migration helper, so this shader chunk name change is backwards-compatible.

@WestLangley WestLangley added this to the r154 milestone Jun 16, 2023
@WestLangley
Copy link
Collaborator Author

This PR requires updated build files.

@github-actions
Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
642.8 kB (159.4 kB) 642.8 kB (159.4 kB) +38 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
436.1 kB (105.6 kB) 436.1 kB (105.6 kB) +38 B

@Mugen87 Mugen87 merged commit c044b5c into mrdoob:dev Jun 17, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 17, 2023

Updated builds: c9ace8c

@WestLangley WestLangley deleted the dev-opaque_frag branch June 17, 2023 14:08
@drcmda
Copy link
Contributor

drcmda commented Jun 25, 2023

I'm not sure, but does backward compat include shader.replace?

If not could this please be reverted? Naming changes cause a lot of churn, not to mention that libraries that do move up have to pin Threejs to latest, which forces them into a major.

I can understand that breaking changes are sometimes necessary but all recent releases were technically semver majors toppling packages and libraries. It would be better to plan ahead and deal with naming in a single release, but spreading this over x releases is not sustainable.

Would be good to hear what @mrdoob thinks as well.

@drcmda

This comment was marked as outdated.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 25, 2023

In what use cases do you use replace()?

@drcmda
Copy link
Contributor

drcmda commented Jun 25, 2023

there are countless of shaders out there extending existing materials via replace. https://github.com/mjurczyk/openvdb/blob/2173458e3b6d646deee5401ec6493079b09c091c/src/openvdb/utils/depth.js#L82 if this breaks i would suggest a revert.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 25, 2023

I've mentioned my concerns in #26206 about renaming shader chunks.

However, it is no option to "freeze" the name of chunks since we have to change/improve/refactor things. It is also no option right now to change the release management (please do not start a discussion about this here).

Given the fallback in #26206 I would say it's appropriate to make this change. We can't guarantee that things do not ever break again and the migration task for fixing this one is easy.

@drcmda
Copy link
Contributor

drcmda commented Nov 2, 2023

the migration task for fixing this one is easy.

it is only easy when you think of migration in terms of local code. it is not easy if you ship a library that people rely on, like drei which is used by thousands, which is now, as i feared, breaking. pmndrs/drei@1169c02

this change clamps it to latest three, while users reported drei failing before the change. imagine publishing something and every month this happens to you, or having to introduce hacks and band aids pmndrs/drei#1592. all we are asking for is that threejs considers the impact of breaking-changes for the surrounding eco systems. a vanilla 3rd party library has little chance to survive, and i could name a bunch that have fallen out over time.

breaking changes are essential, but every sensible library on the web collects a bunch of necessary changes and releases a major with a migration plan, so that people can prepare and switch. threejs relentlessly breaks every single release without stop or mercy and there is no way we can even pin dependencies or make it in any way safe for the end user.

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