Conversation
| private readonly scene: THREE.Scene; | ||
| private readonly composer: EffectComposer; | ||
| private readonly fog: THREE.Fog; | ||
| private readonly view: View; |
There was a problem hiding this comment.
I'd like to add _ before all private members but it will mess up the diff, so maybe it's better to wait for a first review
There was a problem hiding this comment.
Nice feature.
I mainly added architectural comments to simplify the code and increase its robustness.
There is also an integration of THREE.js passes; I think this logic should be integrated into the iTowns renderer, since others might want to use it as well, like @Desplandis for his EDL (points cloud rendering).
| mesh.material.color = new THREE.Color(0xffffff); | ||
| } | ||
|
|
||
| mesh.castShadow = true; |
There was a problem hiding this comment.
mesh.castShadow = true; is hard coded value
We prefer global property : TiledGeometryLayer.castShadow to handle castShadow tile meshes
| // disable fog only during render | ||
| // to let its parameters be modified elsewhere | ||
| if (this.enabled) { this.scene.fog = null; } | ||
| this.update(camera); | ||
| this.update(); | ||
| }; | ||
| scene.onAfterRender = () => { | ||
| if (this.enabled) { this.scene.fog = this.fog; } |
There was a problem hiding this comment.
Practice using updates and rendering in iTowns to take advantage of other features (user events, visibility, frozen
- extends
SkyManager, fromLayerGeometryto use LayerGeometry.updateinstead ofscene.onBeforeRender`, - use
LayerGeometry.visibleinstead ofenable/disable - use
LayerGeometry.object3d.onAfterRenderinstead ofscene.onAfterRender - add
this.sky, this.sunLight, this.sunLight.target, this.skyLightinLayerGeometry.object3dand Layer.visible = false, this will avoid additions and deletions in scene. - As it is preferable to avoid t
his.composer.addPassandremovePassand use pass.enabled.
|
|
||
| update(camera: THREE.Camera) { | ||
| update() { | ||
| const camera = this.view.camera3D as THREE.PerspectiveCamera | THREE.OrthographicCamera; |
There was a problem hiding this comment.
if you use LayerGeometry.update(context, layer, node) the context have the view and the current camera.
packages/Main/src/Core/TileMesh.ts
Outdated
| this.receiveShadow = true; | ||
|
|
There was a problem hiding this comment.
same comment, could use global property ?
| model.castShadow = true; | ||
| model.receiveShadow = true; |
There was a problem hiding this comment.
could use global property, from 3d tile layer ?
|
Nice feature @Neptilo , great visual improvement 🤩
Do you think we could remove replace the picking by something less costly? |
My computer is too good to notice any performance issue! ^^" At least I can see it in the profiler. |
|
@gchoqueux I suggest we discuss the architecture decisions of SkyManager separately since they are not directly related to shadow integration and deal with code that was there prior to this PR |
Thanks, the perfs are way better ! |
There are issues in the
Basically, the first step would be a waste of time, since we would end up reworking all the points mentioned in the second one anyway. It would be more logical and consistent to adapt the architecture right away. |
…eorganize renderer.shadowMap setting
|
The Shadows PR currently relies on the In addition, a performance issue has been reported in issue #2701. It's possible that this slowdown is related to an imperfect integration of the In this context, continuing to develop the Shadows feature without addressing these underlying points could make performance diagnostics more difficult and potentially lead us to rework this feature later. I believe it would be preferable to first consolidate the foundation — particularly regarding the dependency integration and the performance issue — in order to ensure a more stable base moving forward. |
|
@gchoqueux Both of you asked for my input on this PR, so here it is. We discussed the other day with @Neptilo and I think we've reached a compromise that could satisfy both of you (didn't have time to discuss it with you yet). In my opinion, this PR could be implemented independently from the previous one. This would introduce a clearer separation of concerns (instead of the initially proposed monolithic approach) and allow this PR to be reviewed on its own. We should address the sky manager in a dedicated issue (which I haven't open yet, lack of time!). The "layerisation" part seems relatively straightforward. However, the implementation of the "local" (or not?) post-processing passes requires further discussions before implementing any changes. I would gladly have your input in this subject. Given that this new code implements the shadows independently of sky manager (minus some caveats, see below), I think it can be reviewed separately of the previous one. Some ideas that I suggested when we discussed:
P.S.: The performance issue is also upstream, so we should open an issue ! |

Description
Integrate shadows separately from the "realistic lighting" mode. Add UI to control date/time in one example. The Sun position is defined from the date and shared between the shadow rendering and the "realistic lighting", if enabled.
Motivation and Context
Visualizing shadows can be useful to view illumination of buildings or simply to make the rendering more realistic.
Fixes #1825
Screenshots (if appropriate)