-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
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
WebGLRenderer: Tight morph target packing. #27768
base: dev
Are you sure you want to change the base?
Changes from all commits
83d7f36
9d48b7a
28e55a0
eef9f15
bda507e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,12 +20,34 @@ export default /* glsl */` | |
|
||
vec4 getMorph( const in int vertexIndex, const in int morphTargetIndex, const in int offset ) { | ||
|
||
int texelIndex = vertexIndex * MORPHTARGETS_TEXTURE_STRIDE + offset; | ||
int texelIndex = vertexIndex * MORPHTARGETS_TEXTURE_STRIDE + 3 * offset; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to multiply the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To keep in line with what we previously had on the shader chunks that call getMorph. Now the position is at offset 0, normals at offset 3 and colors at offset 6. Alternatively we can modify the other shader chunks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid any misunderstanding, you want me to change the other shader chunks to eliminate the multiplication with 3? At some point we will probably have to revisit that part anyway if/when we add additional morphing attributes like uv. When that time comes more extensive changes on how we handle morphing will be needed because now it's a bit fixed like if there's a color morph but not normals the latter will be empty space in the texture. This is how it used to be even before this PR but we can make it more flexible when more attributes are introduced There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep the current version of this PR 👍 . |
||
|
||
int y = texelIndex / morphTargetsTextureSize.x; | ||
int x = texelIndex - y * morphTargetsTextureSize.x; | ||
|
||
ivec3 morphUV = ivec3( x, y, morphTargetIndex ); | ||
return texelFetch( morphTargetsTexture, morphUV, 0 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are multiple fetches really faster than a single one? I'm afraid this needs to be tested one more than one device. Especially mobile devices could show a performance regression here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As it is currently with rgba format it will always fetch 4 floats so fetching 3 instead should always be faster especially on mobiles where bandwidth is precious. I'll replace the horse example with the faces one and do some more testing but I only have one android phone with an older snapdragon and the quest 3 which all show similar improvements There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively, you could also add a slider to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to have the faces instead because they are using more influences and also morph the normals so its a more complete case than the horses. |
||
|
||
vec4 ret = vec4(0.); | ||
|
||
ret.x = texelFetch( morphTargetsTexture, morphUV, 0 ).r; | ||
|
||
morphUV.x++; | ||
|
||
ret.y = texelFetch( morphTargetsTexture, morphUV, 0 ).r; | ||
|
||
morphUV.x++; | ||
|
||
ret.z = texelFetch( morphTargetsTexture, morphUV, 0 ).r; | ||
|
||
#if MORPHTARGETS_TEXTURE_STRIDE == 10 | ||
|
||
morphUV.x++; | ||
|
||
ret.a = offset == 2 ? texelFetch( morphTargetsTexture, morphUV, 0 ).r : 0.; | ||
|
||
#endif | ||
|
||
return ret; | ||
|
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to set this to 'high-performance'?