Skip to content
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

r157 #4489

Merged
merged 4 commits into from
Oct 4, 2023
Merged

r157 #4489

merged 4 commits into from
Oct 4, 2023

Conversation

elalish
Copy link
Collaborator

@elalish elalish commented Oct 2, 2023

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 elalish self-assigned this Oct 2, 2023
@elalish
Copy link
Collaborator Author

elalish commented Oct 3, 2023

@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
Copy link
Contributor

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
Copy link
Collaborator Author

elalish commented Oct 3, 2023

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 mrdoob/three.js#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
Copy link
Contributor

donmccurdy commented Oct 3, 2023

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
Copy link
Collaborator Author

elalish commented Oct 4, 2023

@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
Copy link
Contributor

.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
Copy link
Collaborator Author

elalish commented Oct 4, 2023

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
Copy link
Collaborator Author

elalish commented Oct 4, 2023

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...

Copy link
Collaborator

@diegoteran diegoteran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome

@elalish elalish merged commit b48087e into master Oct 4, 2023
5 of 6 checks passed
@elalish elalish deleted the r157 branch October 6, 2023 20:44
JL-Vidinoti pushed a commit to vidinoti/model-viewer that referenced this pull request Apr 22, 2024
* r157 and updated events

* TS issue

* fix TS issue

* fixed package-lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants