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

Cascading Shadows #1036

Merged
merged 18 commits into from
Aug 13, 2024
Merged

Cascading Shadows #1036

merged 18 commits into from
Aug 13, 2024

Conversation

Dhruv-0-Arora
Copy link
Collaborator

@Dhruv-0-Arora Dhruv-0-Arora commented Jul 20, 2024

Description

Cascading shadow implementation which increases the quality of the shadows. To change between the directional light and the CSM to increase fps, navigate to the settings modal and turn the quality to "low" or "medium" for the directional light.

Changes

  • Working csm implementation
  • Able to toggle CSM on and off
  • saves in preference manager

NOTE: The light intensity has been increased to 5.0 in both the directional light and CSM.

Dev branch

Screenshot 2024-07-30 at 6 32 03 PM

Low Quality Shadows

Screenshot 2024-07-30 at 6 33 17 PM Screenshot 2024-07-30 at 6 34 33 PM

High Quality Shadows

Screenshot 2024-07-30 at 6 29 31 PM Screenshot 2024-07-30 at 6 34 55 PM

Cascaded Shadow Map from afar

Screenshot 2024-07-30 at 6 36 01 PM

JIRA Issue

@Dhruv-0-Arora Dhruv-0-Arora added the ui/ux Relating to user interface, or in general, user experience label Jul 20, 2024
@Dhruv-0-Arora Dhruv-0-Arora self-assigned this Jul 20, 2024
@Dhruv-0-Arora Dhruv-0-Arora marked this pull request as ready for review July 22, 2024 19:03
@Dhruv-0-Arora Dhruv-0-Arora requested review from HunterBarclay and a team as code owners July 22, 2024 19:03
@Dhruv-0-Arora Dhruv-0-Arora requested review from a-crowell and LucaHaverty and removed request for a team July 22, 2024 19:03
@Dhruv-0-Arora Dhruv-0-Arora marked this pull request as draft July 24, 2024 02:46
Copy link
Member

@BrandonPacewic BrandonPacewic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing about a 15-25% decrease in FPS on my work issued windows laptop.

@Dhruv-0-Arora Dhruv-0-Arora marked this pull request as ready for review July 27, 2024 08:18
Copy link
Member

@HunterBarclay HunterBarclay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good. I'd expand the configuration, either by making low and medium do different settings, or adding direct customization to the settings modal for the lighting. There are a number of artifacts in the shadows that will need to be addressed, but all in all, looking pretty good. Btw, the VSMSoftShadows look a bit better that the PCFSoftShadows. Might want to take a look at that.

Comment on lines 193 to 196
}

// setting light to cascading shadows
else if (quality === "High") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
// setting light to cascading shadows
else if (quality === "High") {
} else if (quality === "High") { // setting light to cascading shadows

}

// setting the shadow map size
const shadowMapSize = Math.min(4096, this._renderer.capabilities.maxTextureSize)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe try using a smaller shadow map size for low quality to reduce memory usage. Cutting the max size from 4096 to 2048 can 1/4th the memory usage of the texture map.

this._light.shadow.camera.right = shadowCamSize
this._light.shadow.mapSize = new THREE.Vector2(shadowMapSize, shadowMapSize)
this._light.shadow.blurSamples = 16
this._light.shadow.bias = 0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add in the normalBias again to fix the artifacts. Also see MirabufInstance about removing the offset of the shadow.

@@ -149,8 +149,10 @@ class MirabufInstance {
opacity: opacity,
transparent: opacity < 1.0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the shadowSide parameters to render against both the front and back faces when casting the shadow. This will remove the gap from the object to the shadow caused by the normal bias.

Comment on lines 198 to 205
parent: this._scene,
camera: this._mainCamera,
cascades: 3,
lightDirection: new THREE.Vector3(0.5, -0.5, 0.5).normalize(),
lightIntensity: 5,
shadowMapSize: shadowMapSize,
mode: "custom",
maxFar: 30,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a shadow bias of -0.00001 worked pretty well for the closest cascade, but not so much for the others. I'd tinker with it.

@HunterBarclay
Copy link
Member

HunterBarclay commented Jul 29, 2024

@Dhruv-0-Arora When you've settled on the settings you like, could you grab comparisons of each different configuration, using a baseline of the current dev branch? Either that or the commit this branch was made from.

Copy link
Collaborator

@a-crowell a-crowell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow these shadows are so clean! I noticed most of the requested changes were made. Just want to make sure Hunter's comment on SceneRenderer:177 was addressed. Otherwise, ready to approve. Great work!

@Dhruv-0-Arora
Copy link
Collaborator Author

Wow these shadows are so clean! I noticed most of the requested changes were made. Just want to make sure Hunter's comment on SceneRenderer:177 was addressed. Otherwise, ready to approve. Great work!

Lowering the Shadow map created some problems with the shadows appearing on the ground so I decided to go back and keep the shadowmap at 4096. It also had minimal RAM usage

Copy link
Collaborator

@a-crowell a-crowell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great, LGTM!

Copy link
Member

@BrandonPacewic BrandonPacewic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have quality settings working in this pr atm or is that a separate issue?

@Dhruv-0-Arora
Copy link
Collaborator Author

Do we have quality settings working in this pr atm or is that a separate issue?

Quality settings does work if you change between Low/Medium and High but the more refined version is in another PR

Copy link
Member

@HunterBarclay HunterBarclay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing some pretty odd artifacts in the second (maybe third) cascade:
image

@LucaHaverty
Copy link
Collaborator

Could you add a check to see if the quality settings are set to Ultra from a previous branch, and if so automatically correct them to 'high'? Mine were set to 'ultra' when I first loaded Synthesis and there were no shadows at all.

@Dhruv-0-Arora
Copy link
Collaborator Author

Could you add a check to see if the quality settings are set to Ultra from a previous branch, and if so automatically correct them to 'high'? Mine were set to 'ultra' when I first loaded Synthesis and there were no shadows at all.

Talked to Luca about this; we are just going to leave it for when I remove the low, medium, high dropdown in #1070

@Dhruv-0-Arora
Copy link
Collaborator Author

I'm seeing some pretty odd artifacts in the second (maybe third) cascade: image

@HunterBarclay I could only see that from like a certain angle at around 30m away. One fix for this would be to increase the maxFar but that would make it so the higher res shadows will appear for a longer time. I added a commit that increases the number of cascades which kind of mitigates this

Copy link
Member

@BrandonPacewic BrandonPacewic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that this will be improved once we get quality options in. Is there perhaps some games that we could look to that implement this and see if they have the same sorts of problems?

@LucaHaverty
Copy link
Collaborator

Whenever I change the shadow quality or change tabs, the memory drops down to ~170mb with the 2018 field, then gradually increases before resetting again. Is this the same issue we were discussing before or something else? On dev I've been seeing a steady ~155mb with the same setup.

image

@Dhruv-0-Arora
Copy link
Collaborator Author

Whenever I change the shadow quality or change tabs, the memory drops down to ~170mb with the 2018 field, then gradually increases before resetting again. Is this the same issue we were discussing before or something else? On dev I've been seeing a steady ~155mb with the same setup.

image

Yep thats the memory leak. If you build and then run then the memory leak won't happen so its a development env problem 😔

@HunterBarclay
Copy link
Member

HunterBarclay commented Aug 12, 2024

@LucaHaverty @Dhruv-0-Arora Actually, that cyclic increase is the garbage collector. The memory leak is a steady increase with no reset.

Copy link
Member

@HunterBarclay HunterBarclay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The artifacts are still there, but only appear when decently far away. Lets get this moved on to the next phase and just table it for later.

@HunterBarclay HunterBarclay merged commit 770220d into dev Aug 13, 2024
12 checks passed
@HunterBarclay HunterBarclay deleted the dhruv/1694/shadow-cascades branch August 13, 2024 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui/ux Relating to user interface, or in general, user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants