(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... 
|
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. |
sites/avivator/package.json
Outdated
| "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 |
This reverts commit f4be45b.
|
Argh... just realised that I had actually introduced a regression recently by not re-introducing the code to import I'm not aware of other outstanding issues - the various review threads have got a bit complex to navigate at this point. I'm tempted to publish an npm package so I could start developing features against it in MDV... I'd rather not but unfortunately I'm not knowledgeable enough to figure out how to import it otherwise. If I do I'll make it very clear that it's purpose is as an experimental version of the original. |
Is a local install an issue? Or install from a remote branch? I think either or |
Thanks - I'll try. I'd like to have it not-just-locally ideally, and I think the above would need some kind of extra step for it to have the build available... Probably mostly a skill issue, and something I want to be able to do more generally - so I don't really want to rush you guys too much, but when I last tried I didn't quite manage. Don't want to pollute this thread with this too much - maybe I should start a discussion elsewhere. |
sounds good! i have success with this in the |
I agree fully. I think this PR should not put the repo into a broken state though. I think the Avivator changes are split now, so we shouldn't be intentionally breaking things. I think we can just work off this branch for now, with installing it into various envs to test out. |
Addresses #
#804
Background
As discussed #790
Fairly extensive changes will be required; discussion should probably migrate to here.