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

Skinning Shaders: Remove need skinning texture size uniform by using textureSize function #27117

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Nov 5, 2023

Related issue: --

Description

Similar to what's used in batching - use the textureSize function to get the texture size, instead. WebGL2 only but Skinning already requires webgl 2.

@gkjohnson gkjohnson added this to the r159 milestone Nov 5, 2023
Copy link

github-actions bot commented Nov 5, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
653.4 kB (161.9 kB) 653.1 kB (161.8 kB) -310 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
446 kB (107.9 kB) 445.8 kB (107.8 kB) -264 B

@gkjohnson gkjohnson requested a review from Mugen87 November 5, 2023 12:13
@Mugen87 Mugen87 merged commit 3e0f80f into mrdoob:dev Nov 6, 2023
19 checks passed
@Mugen87 Mugen87 mentioned this pull request Nov 6, 2023
@gkjohnson gkjohnson deleted the remove-skinning-size-uniform branch November 6, 2023 09:06
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 4, 2023

but Skinning already requires webgl 2.

This is actually not true. I have also forgot this but skinning did work with WebGL 1 when OES_texture_float and vertex textures were supported. This is also documented (see https://threejs.org/docs/index.html#api/en/objects/SkinnedMesh).

With this PR, skinning support for WebGL 1 was completely stopped. A user complains now in the forum: https://discourse.threejs.org/t/r159-use-skinning-webgl1-error/58872

WebGL 1 support should be stopped with r163 but I wonder if this change was a bit premature 🤔 .

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 4, 2023

The thing is I've seen devs already migrating their custom skinning code to the new version (#27291), so reverting would produce even more confusion.

It's a bit unfortunate but I think it's best if we stick to this change and just mentioned in the migration guide that skinning does not work anymore with WebGL 1.

@gkjohnson
Copy link
Collaborator Author

Sorry this was my mistake - I'd forgotten that WebGL 1 could support those features.

It's a bit unfortunate but I think it's best if we stick to this change and just mentioned in the migration guide that skinning does not work anymore with WebGL 1.

I think I agree with this.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 4, 2023

@arpu Out of curiosity: What are doing in your project when we stop WebGL 1 support in r163? Will you modify your fork of three.js to keep WebGL 1? Why do you still need it in your project?

@arpu
Copy link

arpu commented Dec 4, 2023

@Mugen87 the idea was to remove support for ios 14 ( webgl1) with r163 and switch to the new webgpu/webgl2 renderer

arpu added a commit to capticxyz/three.js that referenced this pull request Feb 2, 2024
SemiConscious pushed a commit to SemiConscious/three.js that referenced this pull request Jun 25, 2024
SemiConscious pushed a commit to SemiConscious/three.js that referenced this pull request Jun 25, 2024
SemiConscious pushed a commit to SemiConscious/three.js that referenced this pull request Jun 30, 2024
SemiConscious pushed a commit to SemiConscious/three.js that referenced this pull request Jun 30, 2024
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