Skip to content

Conversation

AdaRoseCannon
Copy link
Contributor

Description:

Adds behavior to background to allow it to set the environment property on the scene element.

It automatically generates an environment map as a cube texture which can be overridden by setting the environment map on a per element basis.

No background:

image

Background is hotpink, no skybox

image

Background is still hot pink but with an image skybox

image

@AdaRoseCannon
Copy link
Contributor Author

For the future this could also be updated to support the environment cube maps generated by WebXR Lighting Estimation:

https://github.com/immersive-web/lighting-estimation/blob/main/lighting-estimation-explainer.md

Copy link
Contributor

@vincentfretin vincentfretin left a comment

Choose a reason for hiding this comment

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

The pictures are nice. :)

@@ -5,7 +5,8 @@ var COMPONENTS = require('../../core/component').components;
module.exports.Component = register('background', {
schema: {
color: {type: 'color', default: 'black'},
transparent: {default: false}
transparent: {default: false},
generateEnvironment: {default: true}
Copy link
Member

Choose a reason for hiding this comment

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

Do we want the default to be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's pretty safe and not a huge performance hit as it only renders the once. It will even fix materials unexpectedly not looking right because they expect an environment map. So it's a nice feature, that you only need to type 'background` to get.

I'd even be tempted to take it a step further and make background="transparent:true" a default component on <a-scene> or at least add background to the hello-world boilerplate just to get the better behaviour by default.

this.cubeCamera = cubeCamera;
this.needsEnvironmentUpdate = true;
scene.environment = cubeRenderTarget.texture;
this.el.sceneEl.addEventListener('loaded', function () {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this event handler is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right,

On a related note, do you know if there is an event handler for things like an object or image has finished loading asynchronously, which would allow this to still work nicely even if someone is loading their skybox etc without using <a-assets>?

Copy link
Member

Choose a reason for hiding this comment

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

mmh not sure if there's a way to universally know it all assets have loaded if <a-assets> is not used. Components can do all sort of arbitrary stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess if someone wanted this behaviour they could do background.needsEnvironmentUpdate=true on this component from their custom components.

It might be nice if components like material would fire a bubbling event once they had loaded an image asynchronously so components like this can update the texture in a few common situations. Although this feature may be the carrot needed to get more developers to use <a-assets>

@dmarcos
Copy link
Member

dmarcos commented Feb 16, 2021

Nice job! It's pretty close. Just a few changes.

This is going to be really useful. Some people got confused with models looking dark and we didn't have a quick solution to give them

@AdaRoseCannon
Copy link
Contributor Author

I think this now resolves the issues.

@AdaRoseCannon
Copy link
Contributor Author

that's done now :-)

@dmarcos
Copy link
Member

dmarcos commented Feb 17, 2021

It looks great! Thanks so much! 🥇

@dmarcos dmarcos merged commit 5a3ae78 into aframevr:master Feb 17, 2021
@AdaRoseCannon
Copy link
Contributor Author

Since Lighting Estimation has landed in Samsung Internet, I'm going to update this to handle lighting estimation by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants