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

Allow directional lights to automatically follow shadows #5025

Merged
merged 6 commits into from Mar 25, 2022

Conversation

AdaRoseCannon
Copy link
Contributor

@AdaRoseCannon AdaRoseCannon commented Mar 18, 2022

Description:

To make AR look believable it really helps to have tone mapping and have shadows that show on the real floor.

Unfortunately when you place an AR object it's easy to leave the region covered by the shadow map. This PR adds the ability to
allow the light's shadow map to automatically follow a particular 3D model to ensure it always has a good shadow.

Changes proposed:

  • Add a new AR example based on the hello world
  • Add shadowCamAutoTarget to light for Directional lights and auto-shadow-cam to <a-light>

image
In the image above both Torus Knots are pure white but the one on the left has toneMapped:false to still displays brightly.

There is a link to the live demo so you can see how the AR looks: https://ada-aframe-test.glitch.me/

The example included in this relies on #5029

@AdaRoseCannon
Copy link
Contributor Author

TODO: Update renderer docs and update material docs

@dmarcos
Copy link
Member

dmarcos commented Mar 21, 2022

Can we break exposure and auto shadows in two PRs? Sorry for the extra work. It will make for a more clear changelog and easier to debug in the future if there are any issues

@AdaRoseCannon
Copy link
Contributor Author

sure, i'll leave this one for shadows and make a new one for exposure related bits

@AdaRoseCannon AdaRoseCannon changed the title Add exposure and allow directional lights to automatically follow shadows Allow directional lights to automatically follow shadows Mar 21, 2022
@@ -15,6 +15,7 @@ registerPrimitive('a-light', {
penumbra: 'light.penumbra',
type: 'light.type',
target: 'light.target',
envmap: 'light.envMap'
envmap: 'light.envMap',
'auto-shadow-cam': 'light.shadowCameraAutoTarget'
Copy link
Member

@dmarcos dmarcos Mar 21, 2022

Choose a reason for hiding this comment

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

camel case for consistency with property names convention and probably avoid shortening automaticShadowCameraTarget makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@AdaRoseCannon
Copy link
Contributor Author

I updated the name to be shadowCameraAutomatic so that it's of the same format as the other shadow camera properties.

@@ -11,6 +11,23 @@ var CubeLoader = new THREE.CubeTextureLoader();

var probeCache = {};

function distanceOfPointFromPlane (positionOnPlane, planeNormal, p1) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe factor out into utils/math.js to keep this tidier.

@@ -146,6 +172,50 @@ module.exports.Component = registerComponent('light', {
this.updateShadow();
},

tick: (function tickSetup () {
Copy link
Member

Choose a reason for hiding this comment

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

no need to name this function

var sphere = new THREE.Sphere();
var tempVector = new THREE.Vector3();

return function tick () {
Copy link
Member

Choose a reason for hiding this comment

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

no need to name this function

var tempVector = new THREE.Vector3();

return function tick () {
if (
Copy link
Member

Choose a reason for hiding this comment

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

anyway to simplify this condition and do an early return for readability?

if (XXX) { return; }

<html>
<head>
<meta charset="utf-8">
<title>Hello, World! • A-Frame</title>
Copy link
Member

Choose a reason for hiding this comment

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

Hello, AR World! perhaps to differentiate

<head>
<meta charset="utf-8">
<title>Hello, World! • A-Frame</title>
<meta name="description" content="Hello, World! • A-Frame">
Copy link
Member

Choose a reason for hiding this comment

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

Hello, AR World! perhaps

* @param {THREE.Vector3} p1 point to test
* @returns Number
*/
function distanceOfPointFromPlane (positionOnPlane, planeNormal, p1) {
Copy link
Member

Choose a reason for hiding this comment

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

pointToTest instead of p1

* @param {THREE.Vector3} out where to store the result.
* @returns
*/
function nearestPointInPlane (positionOnPlane, planeNormal, p1, out) {
Copy link
Member

Choose a reason for hiding this comment

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

pointToTest instead of p1

resultPoint

@AdaRoseCannon
Copy link
Contributor Author

Fixed, sorry for the delay.

@dmarcos
Copy link
Member

dmarcos commented Mar 25, 2022

Thanks so much for the patience! I really appreciate. This is awesome work. Thanks soooo much! Have a nice wekend.

@dmarcos dmarcos merged commit 2fa8e45 into aframevr:master Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants