search

google / model-viewer

Easily display interactive 3D models on the web and in AR!
https://modelviewer.dev
Apache License 2.0
6.76k stars 801 forks source link

r157 #4489

Closed elalish closed 9 months ago

elalish commented 10 months ago

Updated three.js. The new types are pickier about typing events, so this is updated to fix that. Also removed an unnecessary testing-only event.

elalish commented 10 months ago

@Beilinson I'm finding an odd test failure upon updating three.js from 156 to 157. It seems that when the ColorGrade is applied and a render is triggered it somehow makes a render target without a colorspace and then segfaults. However, all the examples seem to work fine, so it seems to be specific to the tests. Any thoughts?

Beilinson commented 10 months ago

Does that still happen if you upgrade postprocessing to the latest version?

Looking at the r157 commit it seems some preliminary support for wide gamut p3 color space was added, maybe the issue is related to that?

elalish commented 10 months ago

Does that still happen if you upgrade postprocessing to the latest version?

Sadly yes.

Looking at the r157 commit it seems some preliminary support for wide gamut p3 color space was added, maybe the issue is related to that?

Perhaps so. @donmccurdy my test is segfaulting since updating to r157, though it seems to work fine in the normal render loop. Might be related to https://github.com/mrdoob/three.js/pull/26644? In the test I appear to get a frame buffer texture that has an undefined colorspace, which appears to be causing the issue. Any idea what might be going wrong?

donmccurdy commented 10 months ago

Hm, I haven't heard of any regressions yet, and the color changes in r157 shouldn't have any effect on normal usage... what do you mean by "undefined" color space? Is that THREE.NoColorSpace, or something else? In most post-processing workflows you probably want renderTarget.colorSpace = THREE.LinearSRGBColorSpace, but I wouldn't think NoColorSpace should fail either.

elalish commented 9 months ago

@donmccurdy When I followed the exception to its source, it said colorspace = undefined on the texture, like it really wasn't there at all. I think NoColorSpace would have worked fine. The trouble is, that feels like an engine bug and I'm not sure how to work around it.

donmccurdy commented 9 months ago

.colorSpace = undefined (note capital "S") is not a valid state. If something within three.js is causing that assignment, I would consider it a bug in three.js. If something in postprocessing or model-viewer causes that assignment, I believe it would be a bug in those projects.

elalish commented 9 months ago

Yeah, inside of setupRenderTarget the texture definitely has no colorSpace defined. Strange that it's fine in the on-page render loop, but fails in the test. I think that texture is owned internally by three.js, so I don't think model-viewer or postprocessing could affect it even if we wanted to. I'll see if I can figure out where it's breaking.

elalish commented 9 months ago

Okay, I figured it out - turns out it was because it was grabbing r148 as a peer dependency in the tests instead of r157. I have no idea why - a different workspace package was set to use r148, but this one specified r157, yet it somehow got 148 anyway. Feels like an npm workspaces bug, but I worked around it by updating so that all our packages use the same three.js version. If it breaks three-gpu-pathtracer, such is life...