itwinjs-core icon indicating copy to clipboard operation
itwinjs-core copied to clipboard

Raphaelmelancon/athmospheric scattering

Open RaphaelMelanconAtBentley opened this issue 3 years ago • 2 comments

Fixes https://github.com/iTwin/itwinjs-backlog/issues/93

Adds an atmospheric scattering lighting effect to the render system. This feature is marked as Beta.

atmospheric_scattering_globe atmospheric_scattering_sky

The effect is only displayed with 3d geolocated iModels with DisplayStyleSettings.backgroundMap set to a backgroundMap with [BackgroundMapSettings.globeMode]core/common/src/BackgroundMapSettings.ts) equal to GlobeMode.Ellipsoid. It wouldn't make sense to display it otherwise.

The effect can be turned on/off with ViewFlags.atmosphericScattering.

The effect can be controlled using AtmosphericScattering.Settings. It is also reactive to the sun's position defined at DisplayStyle3dSettings.lights.

The effect work in both camera and orthogonal perspective.

The effect is implement directly into the existing techniques using the addAtmosphericScatteringEffect.

Fixes https://github.com/iTwin/roadmap/issues/3

Issues:

  • https://github.com/iTwin/itwinjs-core/issues/4471
  • https://github.com/iTwin/itwinjs-core/issues/4472
  • https://github.com/iTwin/itwinjs-core/issues/4475

Is it still true that it heavily impacts performance?

markschlosseratbentley avatar Jul 22 '22 12:07 markschlosseratbentley

@RaphaelMelancon thanks for your work on this. The PR will require some further cleanup around the API and to resolve the identified issues. I will add my comments with the assumption that someone on the display team will address them. I may have specific questions for you about the implementation.

pmconne avatar Oct 11 '22 19:10 pmconne

This pull request is now in conflicts. Could you fix it @RaphaelMelancon? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

mergify[bot] avatar Feb 24 '23 16:02 mergify[bot]

Still need to finalize some UI stuff in display-test-app, but the core code is ready for review.

I've updated the PR and nextVersion images to reflect the updated effect

evelynpreslar-bentley avatar Apr 03 '23 19:04 evelynpreslar-bentley

If both skybox and atmosphere are enabled, the atmosphere takes precedence (always using a black sky) - right? Please document this.

I reorganized the SkySphere shader to add only the atmosphere code if its enabled. All the gradient and texture specific SkySphere code isn't added at all in this case. I also added comments on Environment and SkyBox to convey that this behavior is intended.

evelynpreslar-bentley avatar Apr 12 '23 19:04 evelynpreslar-bentley

This pull request is now in conflicts. Could you fix it @RaphaelMelancon? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

mergify[bot] avatar Apr 14 '23 16:04 mergify[bot]

This pull request is now in conflicts. Could you fix it @RaphaelMelancon? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

mergify[bot] avatar Apr 17 '23 16:04 mergify[bot]