(feat): Upgrade deck.gl and luma.gl
#805
Conversation
|
I made some fixes (commits should be messaged clearly). Thanks for getting this started. More to come tomorrow! I am not clear what was previously |
|
Yeah - as for clear messaging of commits, I'm generally very on-board with that, previous commit was a result of starting to mess around & seeing how far I got, then figured it was more useful to commit than just leave on my machine... I see that as far up as |
|
https://github.com/visgl/luma.gl/blob/master/docs/legacy/porting-guide.md#quick-v9-api-overview
|
|
I think that what's happening is that likely the I think setting uniforms like that should work in the short-term although it'll clearly need changing once it gets to making it run on WebGPU etc. |
|
https://luma.gl/docs/api-reference/core/texture-formats/ (not sure how helpful I'm being here, just dropping things in that seem relevant)
|
|
I optimistically tried changing model
.setBindings({uniforms: {
...uniforms,
contrastLimits: paddedContrastLimits,
...textures
}})
model.draw(); //n.b. took a few moments to realise setBindings() returns undefined...I've not really processed whether that's actually a correct way to be passing uniforms, but it does change the kind of error we see... I don't know how important the warnings that appear before that ( Actually, the error thrown in This is where things get flagged in lumagl // PRIVATE METHODS
// setAttributes(attributes: Record<string, Buffer>): void {}
// setBindings(bindings: Record<string, Binding>): void {}
async _linkShaders() {
const { gl } = this.device;
gl.attachShader(this.handle, this.vs.handle);
gl.attachShader(this.handle, this.fs.handle);
log.time(LOG_PROGRAM_PERF_PRIORITY, `linkProgram for ${this.id}`)();
gl.linkProgram(this.handle);
log.timeEnd(LOG_PROGRAM_PERF_PRIORITY, `linkProgram for ${this.id}`)();
// TODO Avoid checking program linking error in production
if (log.level === 0) {
// return;
}
if (!this.device.features.has('compilation-status-async-webgl')) {
const status = this._getLinkStatus();
this._reportLinkStatus(status); //<<<<<<<
return;
}
// async case
log.once(1, 'RenderPipeline linking is asynchronous')();
await this._waitForLinkComplete();
log.info(2, `RenderPipeline ${this.id} - async linking complete: ${this.linkStatus}`)();
const status = this._getLinkStatus();
this._reportLinkStatus(status);
}FWIW on my M1 running Chrome stable in Sonoma, it doesn't have the I might commit my minor change without great confidence in its correctness... just going to dig through the debugger & docs a bit more and see if I make any more progress... |
|
ok... so a couple of things of note. When I changed I could do with better understanding how pipelines are managed - bearing in mind that When we call /** Update pipeline if needed */
_updatePipeline() {
if (this._pipelineNeedsUpdate) {
let prevShaderVs = null;
let prevShaderFs = null;
if (this.pipeline) {
log.log(1, `Model ${this.id}: Recreating pipeline because "${this._pipelineNeedsUpdate}".`)();
prevShaderVs = this.pipeline.vs;
prevShaderFs = this.pipeline.fs;
}
this._pipelineNeedsUpdate = false;
const vs = this.shaderFactory.createShader({
id: `${this.id}-vertex`,
stage: 'vertex',
source: this.source || this.vs,
debug: this.props.debugShaders
});
let fs = null;
if (this.source) {
fs = vs;
}
else if (this.fs) {
fs = this.shaderFactory.createShader({
id: `${this.id}-fragment`,
stage: 'fragment',
source: this.source || this.fs,
debug: this.props.debugShaders
});
}
this.pipeline = this.pipelineFactory.createRenderPipeline({
...this.props,
bufferLayout: this.bufferLayout,
topology: this.topology,
parameters: this.parameters,
vs,
fs
});
this._attributeInfos = getAttributeInfosFromLayouts(this.pipeline.shaderLayout, this.bufferLayout);
if (prevShaderVs)
this.shaderFactory.release(prevShaderVs);
if (prevShaderFs)
this.shaderFactory.release(prevShaderFs);
}
return this.pipeline;
}When that gets into I notice that the Anyway, I suppose I'll push what I have currently - not confident it's 100% correct. |
|
OK... I think I've made relevant shader edits. Now I get I suspect we need to work on how the uniform buffers are arranged. The Putting a breakpoint on 
|
…' object was spurious.
|
OK, I think we're getting close: I actually see some graphics getting drawn, albeit not very useful.
|
|
It's starting to look like image data... something bad with some constants / parameters for setting sampler attribs or something I guess... 
|
|
At some point, moving towards eventual WebGPU support, we'll need to change the shaders to use Uniform Buffer Objects, I'm not entirely sure the exact change that'll entail. |
ilan-gold
changed the title
ilan-gold
changed the title
deck.gl and luma.gl
|
I totally agree with @manzt about splitting the PR + separate branch. I am doing code review now but I notice some things:
|
|
Safari screen recording: |
|
I get identical behavior without the scale bar and on |
| }; | ||
| } | ||
| export function getRenderingAttrs(dtype, device, interpolation) { | ||
| /// - WebGL1 is no longer supported by lumagl etc. |
There was a problem hiding this comment.
We should maybe separately completely deprecate webgl1? Maybe in a different PR? What is the status of this PR? It seems that within this PR you have also deprecated WebGL1. That would be good to note in a release note
| import GL from '@luma.gl/constants'; | ||
| import { Geometry, Model, Texture2D } from '@luma.gl/core'; | ||
| import { ProgramManager } from '@luma.gl/engine'; | ||
| // ... needed to destructure for it to build with luma.gl 9, but we probably need to change these anyway |
| // This tells WebGL how to read row data from the texture. For example, the default here is 4 (i.e for RGBA, one byte per channel) so | ||
| // each row of data is expected to be a multiple of 4. This setting (i.e 1) allows us to have non-multiple-of-4 row sizes. For example, for 2 byte (16 bit data), | ||
| // we could use 2 as the value and it would still work, but 1 also works fine (and is more flexible for 8 bit - 1 byte - textures as well). | ||
| // https://stackoverflow.com/questions/42789896/webgl-error-arraybuffer-not-big-enough-for-request-in-case-of-gl-luminance | ||
| gl.pixelStorei(GL.UNPACK_ALIGNMENT, 1); | ||
| gl.pixelStorei(GL.PACK_ALIGNMENT, 1); | ||
| // -- this is now applying relevant parameters, but not helping with the rendering... |
There was a problem hiding this comment.
Is it not helping? Do you have a test to show this?
| // This tells WebGL how to read row data from the texture. For example, the default here is 4 (i.e for RGBA, one byte per channel) so | ||
| // each row of data is expected to be a multiple of 4. This setting (i.e 1) allows us to have non-multiple-of-4 row sizes. For example, for 2 byte (16 bit data), | ||
| // we could use 2 as the value and it would still work, but 1 also works fine (and is more flexible for 8 bit - 1 byte - textures as well). | ||
| // https://stackoverflow.com/questions/42789896/webgl-error-arraybuffer-not-big-enough-for-request-in-case-of-gl-luminance | ||
| // -- this is now applying relevant parameters, but not helping with the rendering... | ||
| device.setParametersWebGL({ | ||
| [GL.UNPACK_ALIGNMENT]: 1, | ||
| [GL.PACK_ALIGNMENT]: 1 | ||
| }); |
There was a problem hiding this comment.
Does this happen by default now?
Co-authored-by: Ilan Gold <[email protected]>
…and height in defaultProps
|
ok @ilan-gold thanks for reviewing (good timing, I just got back from holiday), we're hopefully almost there. This is getting a bit big and noisy...
That last point also seems improved by moving |
Thanks, yea. I think the comment is fairly extensive, but Trevor finally discovered why that was necessary to add and it makes sense so I think we should leave it. Maybe luma.gl does it by default or something but best to be on the safe side rather than have images not render with impossible-to-parse error messages. I also agree with Trevor on the branching issue - let's split of the avivator/zustand updates from this and put in a separate small PR. I am not sure we need a |
|
Some slight extra noise with moving dependencies in Avivator to get vercel deployment working with this. Can revert again if you prefer. |
| "zustand": "^3.4.1", | ||
| "vite": "^5.3.2", | ||
| "@vitejs/plugin-react": "^4.2.1" |
There was a problem hiding this comment.
Hmmm, I might remove TBH. But before that why is this necessary for vercel builds? @manzt might have something more graceful up his sleeve?
There was a problem hiding this comment.
I'm alI'm also confused. Why did we get rid of devDeps for vercel? what about Vercel requires this?
I'm just far enough away that I wasn't sure how large the changes would be to either part. Right now the policy to merge into main is generally that we aren't putting the repo in a broken state. I would assume that changes to the core require changes to avivator, so merging in those will will have broken builds of avivator until the remaining stuff is merged.... which is what worries me and hence a separate |
Addresses #
#804
Background
As discussed #790
Fairly extensive changes will be required; discussion should probably migrate to here.