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

WebGLRenderer doesn't support shared material with different object.receiveShadow. #8379

Closed
7 of 13 tasks
killroy42 opened this issue Mar 16, 2016 · 18 comments
Closed
7 of 13 tasks
Labels

Comments

@killroy42
Copy link

Description of the problem

Initial camera position disables/enables shadow rendering, irrespective of subsequent camera positioning. I've tested various positions, and it seems to either be angle or distance related. Please note that I can move the camera to any position I like afterwards (timeout 0), and shadows will continue to render/not-render depending on the initial position.

Reproduction:
CodePen: http://codepen.io/killroy/pen/YqpbOB?editors=0010

// Shadow renders as expected
camera.position.set(280, 50, 244.16);
scene.add(camera);
setTimeout(function() {
  camera.position.set(280, 50, 244.17);
}, 0);

// Shadow is invisible
camera.position.set(280, 50, 244.17);
scene.add(camera);
setTimeout(function() {
  camera.position.set(280, 50, 244.17);
}, 0);
Three.js version
  • dev
  • r75
  • r74
  • r73
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • Linux
  • Android
  • IOS
Hardware Requirements (graphics card, VR Device, ...)
@mrdoob
Copy link
Owner

mrdoob commented Mar 16, 2016

Interesting! Thanks a lot for the repro. I'll investigate this.

@WestLangley
Copy link
Collaborator

Also, a known issue, shadows do not render on the first call to render().

In the codepen, remove the animate() function. Then,

// shadows not rendered
renderer.render(scene, camera);
//renderer.render(scene, camera);

But call render() again, and

// shadows rendered
renderer.render(scene, camera);
renderer.render(scene, camera);

@RemusMar
Copy link
Contributor

// Shadow renders as expected
camera.position.set(280, 50, 244.16);
// Shadow is invisible
camera.position.set(280, 50, 244.17);

I didn't find any error in that piece of script.
Probably this bug is from X-Files.
Unbelievable!

@killroy42
Copy link
Author

I've tried other locations, modifying different axis', but haven't found the pattern yet. But it seems to be either distance or angle based... probably distance. VERY strange.

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

Also, a known issue, shadows do not render on the first call to render().

Ok, I found out why. Fixing...

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

Ok, also figured out the the bug for this issue too. Seems like I broke shadows when reusing materials.

Surprised that we hadn't caught this yet.

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

Hmm, it's tricky. I'm not sure we can support two objects, one with receiveShadows as true and the other as false sharing the same material...

@RemusMar
Copy link
Contributor

I'm not sure we can support two objects, one with receiveShadows as true and the other as false sharing the same material...

Shadows on/off should work "per geometry" (not "per material")

@killroy42
Copy link
Author

I don't think that would be a problem, as long as it's documented. Perhaps that would mean the "castShadow" property should be on the material instead? Wish I could help more, but I'm new to 3D. Hope I can figure it all out shortly, though.

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

I'm not sure we can support two objects, one with receiveShadows as true and the other as false sharing the same material...

Shadows on/off should work "per geometry" (not "per material")

I agree... It's just that the current design doesn't support that and it'll require quite a bit of refactoring. Will have to give it a go at some point.

@mrdoob mrdoob added the Bug label Mar 17, 2016
@mrdoob mrdoob changed the title Shadow rendering dependent on initial camera position (r75) WebGLRenderer doesn't support shared material with different object.receiveShadow. Mar 17, 2016
@killroy42
Copy link
Author

What's the work-around then? Just clone the material, and ensure that shadow on/off is done on separate materials? PS: I'd love to dig deeper. Any tips as to which files I should look at regarding the camera position issue?

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

Well, is not really a camera position issue. Sorry, I didn't explain...

The issue is that when the camera is at 280, 50, 244.17 the sphere is (correctly or not) inside the frustum.

Because of the front-to-back sorting, the renderer deals with that mesh (which doesn't receive shadows) first and computes the shader for the material. Then it renders the plane (which receives shadows) and finds that the material has already been computed so it uses that.

The code that deals with this, is still a bit of a mess. It's spread around WebGLRenderer, WebGLPrograms and WebGLProgram.

In order to know if a material with the same parameters has been already compiled we use these:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLPrograms.js#L17-L28
https://github.com/mrdoob/three.js/blob/dev/src/renderers/webgl/WebGLPrograms.js#L120-L184

I need to study this a little bit more, it may be possible to take the object.receiveShadow into account at that step.

@killroy42
Copy link
Author

Aha, thanks for the explanation. So it's an ordering issue that is modified by the camera position. Do you update the docs frequently? So a workaround could be noted? (Many of the doc pages and examples are incompatible with r75 regarding shadows as far as I could find) Can I PR for doc updates?

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

Can I PR for doc updates?

Sure! Just make sure you target the dev branch.

@mrdoob
Copy link
Owner

mrdoob commented Mar 17, 2016

So seems like it's possible to add object.receiveShadow as part of the "hash"/"code". But, because the "hash"/"code" will change, that the renderer will keep re-compiling the shader:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L1445

/ping @tschw

@tschw
Copy link
Contributor

tschw commented Mar 21, 2016

But, because the "hash"/"code" will change, that the renderer will keep re-compiling the shader

Yes.

The most straightforward / naive approach would be to cache materialProperties.programWithShadows and materialProperties.programWithoutShadows.

However, while we're at it, we might want to think about a more general solution, where we distinguish between programs (that correspond to the source code) and program instances (that correspond to the runnables under specific settings).

Ideally, there would be some rule-based logic that decides the program to be used based on the material type (or property for ShaderMaterial), the object, the type of the renderer (fancy PBR material may not render quite as fancy when the canvas API is all you got for rendering, and the program to do so may be JavaScript code rather than GLSL) and the rendering pass (see e.g. the not very extensible distance / depth shader stuff for shadow mapping).

On first use, a program gets instantiated based on the material configuration (e.g. diffuse map is present or not), object state (e.g. receives shadow or not), and viewing/lighting environment. The program instance gets cached and eventually shared amongst material / object combinations.

Ideally, the program would advertise the definitions used in the source code, so they can be sorted and the corresponding values can be mangled into a short string key to identify its instances. At this point we may want to add a method to inject pre-optimized shaders into the cache, so they can be shipped with an app. This key would also be most useful to sort by GL program for faster rendering.

@tschw tschw mentioned this issue Mar 25, 2016
@tschw
Copy link
Contributor

tschw commented Apr 7, 2016

Proposal for a concrete implementation that solves the root of the problem in #8552.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 7, 2021

I'm not sure we can support two objects, one with receiveShadows as true and the other as false sharing the same material...

Fixed via #17413.

https://jsfiddle.net/uory9zbs/3/

@Mugen87 Mugen87 closed this as completed Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants