LightProbeHelper: Add WebGPU version. #29301
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
| this.size = size; | ||
| this.type = 'LightProbeHelper'; | ||
|
|
||
| this._intensity = intensity; |
There was a problem hiding this comment.
Is this property doing anything?
There was a problem hiding this comment.
The uniform is updated in onBeforeRender().
There was a problem hiding this comment.
I do see that, but why is the same "hack" not needed for the probe coefficients, but is needed for the intensity?
There was a problem hiding this comment.
The line is indeed redundant. I'll remove it.
There was a problem hiding this comment.
The line is indeed redundant. I'll remove it.
This PR was working. Now it is not.
If you figure it out, please do your best to answer my question above so we will both understand. 🙏
There was a problem hiding this comment.
I think there is a difference because intensity is a primitive value and _sh an array of objects. It probably would not work if you exchange the vector objects in the array and not just update their values.
There was a problem hiding this comment.
I think we can rewrite the intensity uniform like so:
const intensity = reference( 'intensity', 'float', lightProbe );Then a manual uniform update isn't necessary. However, reference() would not work for uniform arrays ,afaik.
There was a problem hiding this comment.
You added _sh in #29303, and that was not necessary if you are updating just the contents of the array.
With all due respect, I think you merged PRs too fast, without allowing time for the reviewer to respond.
Can you please continue to investigate this so we can all understand?
There was a problem hiding this comment.
You added _sh in #29303, and that was not necessary if you are updating just the contents of the array.
I suspect you have tested this by just updating the existing vector objects, right? However, when the array is overwritten with new objects, the changes from #29303 are required.
I just wanted to be on the safe side, that's why I have honored the coefficients in #29303. If you think this is not required since the coefficient vectors are usually not overwritten with new instances, #29303 can be partly reverted.
With all due respect, I think you merged PRs too fast, without allowing time for the reviewer to respond.
I understand but it is also imported to move forward with the migration. We can fix such minor issues in dev as well.
There was a problem hiding this comment.
you have tested this by just updating the existing vector objects, right?
Right. Just the contents.
If you think this is not required since the coefficient vectors are usually not overwritten with new instances, #29303 can be partly reverted.
I think it's not required. Granted, it is a judgment call.
Related issue: #29295
Description
This PR adds a version of
LightProbeHelperthat is compatible withWebGPURenderer.