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

Add envmap generation for environments #38

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

msfeldstein
Copy link

@msfeldstein msfeldstein commented Mar 19, 2018

PBR materials, particularly anything metallic, don't look good without an envmap. They are very dark. With this change, an envmap will be automatically generated with the environment component, and applied to any materials in the scene. Here is an example of a before and after.
screen shot 2018-03-18 at 2 57 23 pm
screen shot 2018-03-18 at 2 57 09 pm

@msfeldstein
Copy link
Author

i realize that this probably won't bundle properly, having includes in the index.js. If this is something you think would belong in this component we can figure out a solution, otherwise i can package this up as its own optional sibling component

@donmccurdy
Copy link

donmccurdy commented Mar 19, 2018

Nice work! I'll leave to others whether this should be part of the environment component but my hunch would be a separate component makes best sense. A few other comments:

  1. Better to use object3dset event instead of model-loaded, to catch meshes added outside of loaders.
  2. Why scene.remove(this.el.object3D) and then reattaching? I don't think that should be needed, but note it triggersobject3dset events if you implement (1).
  3. Looks like the camera is located at 0,0,0, and some reflections are under the ground plane. Maybe use 0,1.6,0 instead, or default camera position? Or if doing a separate component, could include a property for this.
  4. In applyEnvMap() I would take an argument for the root node, and only traverse that. This would avoid re-compiling more shaders than necessary. Maybe also limit to PBR materials and handle multi-materials:
var materials = Array.isArray(mesh.material) ? mesh.material: [mesh.material];
materials.forEach((material) => {
  if (material.isMeshStandardMaterial) {
    material.envMap = this.envMap;
    material.needsUpdate = true;
  }
});

If you go the separate component route, could also include some more options:

<a-scene env-map-generator="sourcePosition: 0 1.6 0;
                            near: 0.1;
                            far: 1000;
                            resolution: 512;">
</a-scene>

Could also attach the component within the scene, to have an inherent position.... can't decide if I prefer that vs this scene-level singleton syntax.

@msfeldstein
Copy link
Author

the reason i removed the environment elements is because i didn't want to apply the envmap onto anything inside the actual environment objects. A better way will be to just check if the environment component is an ancestor of the object im traversing. I'm going to pull this into a separate compoenent and will post a link here.

@msfeldstein
Copy link
Author

So talking with @donmccurdy a bit in the slack, and if this is a separate component, we need a way to target just the environment elements (actually we need it if its built in too). We were thinking we could add an "environment-object" class name or something to all the children in this component, so when people add an environment map they can target all .environment-object's and anythign else they want in the reflections. This is necessary because if you add environment component to the a-scene element, there's no way to get just the environment tags, and not the rest of the scene.

@feiss
Copy link
Contributor

feiss commented Mar 21, 2018

Dude, this is brilliant, thanks for making this. Yup, I agree with @donmccurdy and I think this should be a separated component, so everybody can benefit from it, not only environment-component users.

Yup, I do agree on adding a classname to the entities in this component. Will do it asap.

@msfeldstein
Copy link
Author

msfeldstein commented Mar 21, 2018 via email

@feiss
Copy link
Contributor

feiss commented Mar 22, 2018

done! environment-entity added to all entities

@msfeldstein
Copy link
Author

looks very cool, but you mark this.el with the class, and this.el is the scene element a lot of times (for example in the examples set up via angle). This would cause everything in the scene to be captured.

@donmccurdy
Copy link

Also minor nit i would vote for just environment as the classname, i think object or entity is implied. 😇

@feiss
Copy link
Contributor

feiss commented Mar 23, 2018

yeah ok! environment class, removed from this.el 👍

@feiss
Copy link
Contributor

feiss commented Mar 23, 2018

oh @msfeldstein , when you have the component ready, could you open a PR here modifying the README, explaining how to use it with the environment? thanks a lot!

(Add a section called sth like "Environment Map Generation" before the "Performance" section, for example)

@msfeldstein
Copy link
Author

Here's what ive got so far. I don't want to publish this until this aframe-environment-component pushes a new version so people can get the latest with the class names. It does work great though!

https://github.com/msfeldstein/aframe-environment-map-component/blob/master/README.md

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.

3 participants