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

MeshBasicMaterial: Why are there .aoMap, .lightMap, and .envMap properties? #15548

Closed
donmccurdy opened this issue Jan 8, 2019 · 12 comments
Closed
Labels

Comments

@donmccurdy
Copy link
Collaborator

The MeshBasicMaterial docs mention that it supports .envMap, .lightMap, and .aoMap. I'm not sure how that aligns with its definition, This material is not affected by lights.. Not affected by analytic lights? What's the purpose of a light map or ao map here?

@WestLangley
Copy link
Collaborator

Some history: #9975.

@WestLangley
Copy link
Collaborator

Some earlier history: #6263.

@WestLangley
Copy link
Collaborator

Regarding the environment map, the PBR materials treat .envMap as an additive light source.

It was not originally like that; that is why the material.combine property was required.

The original three.js materials have largely remained unchanged since their introduction.

@emackey
Copy link

emackey commented Jan 8, 2019

The root of Don's question here comes from the glTF KHR_materials_unlit extension, which is supported in the GltfLoader.js by way of the MeshBasicMaterial. A typical client of such a loader may blindly mass-apply an environment to all of the materials of a loaded glTF file. But in the case of unlit materials, applying the environment breaks the unlit effect by multiplying in some reflections.

My question on this is, should clients of GltfLoader.js be smart enough to detect MeshBasicMaterial and understand that it comes as a result of KHR_materials_unlit and should therefore be excluded from having an environment map? Or should the loader take steps to turn off reflections, such as by setting .reflections = 0 or similar? Don pointed out in another thread that the glTF loader shouldn't break user expectations of the material system, but, asking users to understand which glTF materials need environment maps seems like a high bar.

Perhaps the environment map could be passed into the glTF loader, so it could make its own decisions about which materials need the environment?

@WestLangley
Copy link
Collaborator

@emackey Your point is well-stated. As far as I am concerned, this is a legacy three.js issue, and I do not have a strong opinion one way or the other how we resolve it.

@donmccurdy
Copy link
Collaborator Author

Thanks @WestLangley — that makes more sense knowing MeshBasicMaterial is treating envMap differently than the PBR materials.

I do think it would be a step too far for GLTFLoader to take steps ensuring that manual user assignments to a material will do nothing... That MeshBasicMaterial supports these other maps was surprising to me, and I'm not sure I understand the purpose (as a user, why not merge map+aoMap+lightMap into a single texture for MeshBasicMaterial? to use separate UVs?) but I don't feel strongly enough to suggest removing support.

Since both creating unlit materials in glTF and adding an environment in three.js are "opt-in" choices, I don't necessarily think a change is needed. Adding a loader.setEnvMap( texture ) method would solve the immediate issue, but it's hard to recommend that when it requires loading the model and the envMap in sequence.

@takahirox
Copy link
Collaborator

I think setting .reflectivity = 0; for MeshBasicMaterial in GLTFLoader is ok. It's valid setting. And I don't think users expect that environment breaks unlit.

@donmccurdy
Copy link
Collaborator Author

Ok, reflectivity=0 sounds good to me. But let's leave the AO and lightmap intensity settings alone.

@takahirox
Copy link
Collaborator

Oops, I'm just aware now of that we're talking about not only envMap but also aoMap and lightMap.

But let's leave the AO and lightmap intensity settings alone.

This means, should aoMap and lightMap be able to be applied to gltf unlit extension material?

@donmccurdy
Copy link
Collaborator Author

The original motivation for the question was just about envMap, since users sometimes assume they need to traverse their models and apply envMap to each material. The same has never happened for aoMap or lightMap. On the other hand, if including the envMap will have a negative performance effect on an otherwise lightweight material, maybe it's best that the effect of the envMap is visible rather than hidden?

I strongly think we should not prevent aoMap or lightMap from being applied, just as we would not prevent a user from changing a material's color, just because the glTF file says it is blue.

@takahirox
Copy link
Collaborator

takahirox commented Jan 11, 2019

if including the envMap will have a negative performance effect on an otherwise lightweight material

Good point. Yeah, setting envMap should have negative performance impact even if .reflectivity=0. It's opposed to performant material that is one of the unlit extension objectives.

Starting to think it may be good we add a note to the doc and ask users to check if material is not MeshBasicMaterial for setting envMap, rather than setting .reflectivity=0, like

scene.traverse(object => {
    if(object.material && !object.material.isMeshBasicMaterial) {
        object.material.envMap = envMap;
        object.material.needsUpdate = true;
    }
});

Regarding aoMap and lightMap, agreed with not preventing user's change. (If we want to prohibit these maps, maybe we should use ShaderMaterial, not MeshBasicMaterial.) So similarly starting to think envMap effect should be visible...

@donmccurdy
Copy link
Collaborator Author

Ok, leaning against making a change here but more feedback is welcome.

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

No branches or pull requests

4 participants