Skip to content

r175 breaks custom shadow because RenderTarget changed to multisampled #30895

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

Closed
Spiri0 opened this issue Apr 8, 2025 · 3 comments · Fixed by #30921
Closed

r175 breaks custom shadow because RenderTarget changed to multisampled #30895

Spiri0 opened this issue Apr 8, 2025 · 3 comments · Fixed by #30921
Milestone

Comments

@Spiri0
Copy link
Contributor

Spiri0 commented Apr 8, 2025

Description

I created a live example using r174. As soon as I switch to r175, it breaks because in r175 the RenderTarget suddenly sets to multisampled. Even if I set samples in RenderTarget to 1 it doesn't work.
I think the RenderTarget would benefit from a parameter that lets you set it to multisampled or non-multisampled.

I created my own light source, as simple as possible, from lines 221 to 328. In it, I switch between a placeholder depth texture and the scene depth texture. This has the advantage that the shader is initialized from the beginning and I avoid the frame delay that occurs when using threejs light sources if I want to use the shadow map from them for my own shader.

Reproduction steps

I think simply using my working codePen / jsfiddle with r174 and looking at the small, quick-to-understand light source from 221-328 is easier to understand than a long to-do description. 242 to 273 are the interesting parts.
I installed the samplerComparison into my r174. It works fine.

Simply switch from r174 to r175 and in the console you can see that the RenderTarget suddenly delivers a multisampled texture. If I can use multisampled depth textures for the placeholder and the scene depth texture, that would also be a possibility. The important thing is that both have the same nature so that they remain interchangeable at the shader uniform.

P.S. I tried to incorporate a few PRs into r174 that were incorporated into r175. Unfortunately, I don't recognize any PR that could be the cause of the issue. It has to be one of the PRs.

Code

look at live example

Live example

https://codepen.io/Spiri0/pen/ByaeZJV?editors=0010
https://jsfiddle.net/Spiri/k7e8trzv/4/

Screenshots

No response

Version

175

Device

No response

Browser

No response

OS

No response

@Spiri0
Copy link
Contributor Author

Spiri0 commented Apr 10, 2025

The cause lies in src/core/RenderTarget.js
If I use r175 with the RenderTarget.js from r174 it works.
I haven't found the corresponding PR yet. The class has been significantly expanded.
In any case, I've already narrowed down the error to this file. Perhaps I'll still find the root cause of the problem in the file itself. @Mugen87, do you know who changed RenderTarget.js?

@Spiri0
Copy link
Contributor Author

Spiri0 commented Apr 10, 2025

In the constructor of RenderTarget.js in r175 this is missing: this.depthTexture = options.depthTexture;
If I add that single line in the constructor, r175 runs without errors.
The rest of the constructor is basically the same as in r174, only more comprehensively described.
The question now is why this one line was deleted?
If it was accidentally forgotten during the extensive revision of RenderTarget.js the issue would have been quickly resolved. I don't want to blindly submit a PR just to get my code working again. Whoever edited this will surely be able to quickly tell whether it was intentional or an oversight. I'm at least happy that I was able to find the cause.

@Spiri0
Copy link
Contributor Author

Spiri0 commented Apr 12, 2025

I found the PR #30633. It was a pretty comprehensive PR with a lot of changes.

Here it can see how this.depthTexture = options.depthTexture; was removed.

In r174 it was:

	this._depthTexture = null;
	this.depthTexture = options.depthTexture;

I admit I still don't understand why this was omitted. However, this.depthTexture is important for correct communication to the shader. With this one line, everything is fine again. I'll take a closer look to perhaps understand it better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants