-
Notifications
You must be signed in to change notification settings - Fork 198
Try to fix clip optimization #1202
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
base: main
Are you sure you want to change the base?
Conversation
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.
It's a shame this doesn't build on #1199, so it doesn't have a test. The implementation strategy looks OK-ish to me though.
I think this will also need to apply to the CPU shaders?
| for (var i = 0u; i < PIXELS_PER_THREAD; i += 1u) { | ||
| blend_spill[local_blend_start + i] = pack4x8unorm(rgba[i]); | ||
| rgba[i] = vec4(0.0); | ||
| rgba[i] *= rgba_mul; |
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.
Obviously this is assuming that doing a floating point multiply is extremely cheap, right?
At least in the non-isolated case, not doing anything at all is of course more ideal. Although that would of course involve hoisting the condition, leading to code duplication.
| } | ||
| cmd_ix += 2u; | ||
| } | ||
| case CMD_BEGIN_CLIP: { |
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.
My vague plan for implementing this was to turn it into a whole new command, as the operations are at least a little different. I presume you didn't want to do that to avoid the code duplication?
| for (var i = 0u; i < PIXELS_PER_THREAD; i += 1u) { | ||
| blend_spill[local_blend_start + i] = pack4x8unorm(rgba[i]); | ||
| rgba[i] = vec4(0.0); | ||
| rgba[i] *= rgba_mul; |
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.
I presume that the multiply by one (or indeed by zero) can't cause rendering differences. Given that these are only colours, I can't see that it can.
| blend_stack[clip_depth][i] = pack4x8unorm(rgba[i]); | ||
| rgba[i] = vec4(0.0); | ||
| } | ||
| rgba[i] *= rgba_mul; |
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.
You're sure the backdrop isn't now double counted, even if it has alpha? I think that a over a if a is partially transparent isn't equal to a.
| clip_zero_depth = clip_depth + 1u; | ||
| } else { | ||
| write_begin_clip(); | ||
| let blend = scene[dd]; |
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.
Handy that this was available. Seeing what the blend mode was at "write begin clip" time was my greatest worry in implementing this (as in I was worried it would be annoying)
No description provided.