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

Implementation of the final API for clustered lighting, shadowType support #3763

Merged
merged 15 commits into from Dec 10, 2021

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Dec 2, 2021

Clustered lighting API has been updated to its final form.

New API (still private)

        app.scene.clusteredLightingEnabled = true;

        app.scene.lighting.cells = new pc.Vec3(30, 2, 30);
        app.scene.lighting.maxLightsPerCell = 20;

        app.scene.lighting.shadowsEnabled = true;
        app.scene.lighting.shadowType = pc.SHADOW_PCF1;
        app.scene.lighting.shadowAtlasResolution = 2048;

        app.scene.lighting.cookiesEnabled = true;
        app.scene.lighting.cookieAtlasResolution = 1500;

        app.scene.lighting.areaLightsEnabled = true;

Old API (removed)

        pc.LayerComposition.clusteredLightingEnabled = true;
        app.scene.layers.clusteredLightingCells = new pc.Vec3(16, 12, 16);
        app.scene.layers.clusteredLightingMaxLights = 80;
        app.scene.layers.clusteredLightingShadowsEnabled = true;
        app.scene.layers.clusteredLightingCookiesEnabled = true;
        app.scene.layers.clusteredLightingAreaLightsEnabled = true;

Other changes

  • The Scene object now accepts GraphicsDevice as a parameter. This constructor has been made private as well, as the engine does not support multiple Scenes at all.
  • Added controls to Clustered spot and omni shadow examples

Clustered shadow filtering: support for PCF1, 3 and 5 filtering for clustered shadows (PCF5 is WebGl2 only, falls backs to PCF3 on WebGl1). Details about added PCF1:

  • on webgl1 this is a single sample with point filtering (linear filtering would need 4 samples)
  • on webgl2 this is a single sample with linear depth filtering (hardware depth filter)

Screenshot 2021-12-06 at 15 06 30
Screenshot 2021-12-06 at 15 06 14

@mvaligursky mvaligursky requested a review from a team December 2, 2021 09:31
@Maksims
Copy link
Contributor

Maksims commented Dec 2, 2021

If there is one scene per app, so one clustered lighting per app, would it be better to have such API (less depth): app.clusteredLighting.enabled and similar.
Also easier on documentation, can be separately found in own section instead of scene.

@mvaligursky
Copy link
Contributor Author

The longer team goal is to make multi Scene supported. Scene would represent its set up - global lighting, fog, clustered lighting and similar. But due to many shortcuts taken in how this was put together, this is not possible at the moment, and so there is no reason to expose Scene constructor. But I don't want to close this door.

@willeastcott
Copy link
Contributor

My knee-jerk reaction is that the property names are quite verbose. So presumably we're heading for a world where it's not really 'clustered lighting' any more but just 'lighting' (because we plan to dump the current lighting system).

So, I'm thinking more along the lines of:

        app.scene.lightingCells = new pc.Vec3(30, 2, 30);
        app.scene.maxLights = 20;
        app.scene.shadowsEnabled = true;
        app.scene.cookiesEnabled = true;
        app.scene.areaLightsEnabled = true;
        app.scene.shadowAtlasResolution = 2048;
        app.scene.cookieAtlasResolution = 1500;

I've not included app.scene.clusteredLightingEnabled above because presumably, that will go away when clustered lighting is the only option?

Also, keep in mind that the Editor codebase does call the Scene constructor in at least a couple of places.

Martin Valigursky added 2 commits December 6, 2021 09:18
# Conflicts:
#	src/scene/renderer/forward-renderer.js
@mvaligursky
Copy link
Contributor Author

great suggestions @willeastcott - I've done just that, but additionally renamed lightingCells to lightCells.

src/scene/scene.js Outdated Show resolved Hide resolved
@mvaligursky mvaligursky changed the title Implementation of the final API for clustered lighting Implementation of the final API for clustered lighting, shadowType support Dec 6, 2021
@willeastcott
Copy link
Contributor

This PR introduces Scene#shadowType but we already have LightComponent#shadowType. How do we manage this API change?

@mvaligursky
Copy link
Contributor Author

This PR introduces Scene#shadowType but we already have LightComponent#shadowType. How do we manage this API change?

Clustered lighting ignores shadowType on the LightComponent, and all cluster lights use the same shadow filtering. If at some point we switch to clustered lighting as a main lighting system, only directional lights will have shadowType exposed on the component. Similarly to shadow resolution - this is automatically assigned from the atlas.

We could possibly do the same with light attenuation - this is per light, as it's reasonably cheap .. but personally I would make it global for cluster lights to save all dynamic branches we can.

@willeastcott
Copy link
Contributor

Do we prefer enableXXX or xxxEnabled?

I can find a few precedents for enableXXX. For example:

@mvaligursky
Copy link
Contributor Author

mvaligursky commented Dec 6, 2021

Good question.

I like enabled vs enable as that matches the Component.enabled which is commonly used. In a way, its used in a similar way to for example entity.camera.enabled or scripts.bloom.enabled

We also have lightmapFilterEnabled.

But I don't mind either way.

src/scene/constants.js Show resolved Hide resolved
src/scene/scene.js Outdated Show resolved Hide resolved
src/scene/scene.js Outdated Show resolved Hide resolved
src/scene/scene.js Outdated Show resolved Hide resolved
@mvaligursky
Copy link
Contributor Author

the naming agreed to in a discussion:

scene.lighting
	.cells
	.maxLightsPerCell
	.shadowsEnabled
	.shadowType = pc.SHADOW_PCF1;
	.shadowAtlasResolution = 2048;

	.cookiesEnabled = true;
	.cookieAtlasResolution = 1500;

	.areaLightsEnabled = true;

Martin Valigursky added 2 commits December 8, 2021 16:25
# Conflicts:
#	src/graphics/program-lib/programs/standard.js
#	src/scene/materials/standard-material-options-builder.js
#	src/scene/scene.js
@slimbuck
Copy link
Member

slimbuck commented Dec 9, 2021

Do we prefer enableXXX or xxxEnabled?

I can find a few precedents for enableXXX. For example:

To me xxxEnabled sounds informative, not something I can set. Since this is lighting API for setting config, I'd probably prefer enableXxx form. From a quick look around the engine I see use of both forms, but probably more enableXxx at this point.

@mvaligursky
Copy link
Contributor Author

to me, xxxEnabled matches with the style of entity.enabled and component.enabled.
enableXXX sounds like a name of a function to me, not a getter & setter.
@willeastcott any thoughts on this?

@willeastcott
Copy link
Contributor

OK, @mvaligursky, while I'm still a little torn on this, I'll go with xxxEnabled.

@mvaligursky mvaligursky merged commit 406fb89 into dev Dec 10, 2021
@mvaligursky mvaligursky deleted the mvaligursky-clustered-api branch December 10, 2021 09:41
@slimbuck slimbuck mentioned this pull request Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants