Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Nov 26, 2018

Documents Material.lights just once in Material and not in other documentation pages. Also added a note so users consider this property as read-only except for ShaderMaterial and RawShaderMaterial.

Fixed #13896

@donmccurdy
Copy link
Collaborator

I didn't realize it was readonly for default materials. That's good to know. 😅

@WestLangley
Copy link
Collaborator

Documents Material.lights just once in Material and not in other documentation pages.

Why? The defaults are different.

@WestLangley
Copy link
Collaborator

I do not think you should be updating Chinese docs. We need to be consistent. Always update them -- or never update them, and let someone else do that.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 26, 2018

FWIW my preference would be to update the structure of the Chinese docs when we can (e.g. adding empty headers or removing sections), but leave others to fill in the translations. This way at least it is easy to see where translations are missing. But probably best to get their preference on how they'd prefer to track changes, they have a hard enough task as-is..

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 26, 2018

FWIW my preference would be to update the structure of the Chinese docs when we can (e.g. adding empty headers or removing sections), but leave others to fill in the translations.

Sounds good 👍 . Let me do it like this in future PRs.

Why? The defaults are different.

It's a bit unusual to document properties in derived classes again just because they have a different value. I've never seen this in a software project. And I think in this particular case, this information is not valuable for the user.

@WestLangley
Copy link
Collaborator

FWIW my preference would be to update the structure of the Chinese docs when we can (e.g. adding empty headers or removing sections), but leave others to fill in the translations.

I very much disagree with that. When we ask another contributor to update the docs for their PR, are we going to make sure to ask them to update the structure of the Chinese docs, too? I hope not.

@donmccurdy
Copy link
Collaborator

I agree that’s not necessary, but wouldn’t say it’s an argument do discourage others from updating the Chinese docs if they choose to.

@mrdoob
Copy link
Owner

mrdoob commented Nov 27, 2018

Updating the chinese docs whenever possible sounds good to me. But it definitely won't scale if we start getting more languages.

As per the actual issue... I can't think of any reason why lights need to be in Material and not in ShaderMaterial...?

@mrdoob mrdoob added this to the r99 milestone Nov 27, 2018
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 27, 2018

I can't think of any reason why lights need to be in Material and not in ShaderMaterial...?

WebGLRenderer uses material.lights at several places in order to distinct between lit and unlit materials e.g.:

if ( material.lights ) {
// wire up the material to this renderer's lighting state
uniforms.ambientLightColor.value = lights.state.ambient;
uniforms.directionalLights.value = lights.state.directional;
uniforms.spotLights.value = lights.state.spot;
uniforms.rectAreaLights.value = lights.state.rectArea;
uniforms.pointLights.value = lights.state.point;
uniforms.hemisphereLights.value = lights.state.hemi;
uniforms.directionalShadowMap.value = lights.state.directionalShadowMap;
uniforms.directionalShadowMatrix.value = lights.state.directionalShadowMatrix;
uniforms.spotShadowMap.value = lights.state.spotShadowMap;
uniforms.spotShadowMatrix.value = lights.state.spotShadowMatrix;
uniforms.pointShadowMap.value = lights.state.pointShadowMap;
uniforms.pointShadowMatrix.value = lights.state.pointShadowMatrix;
// TODO (abelnation): add area lights shadow info to uniforms
}

@mrdoob mrdoob modified the milestones: r99, r100 Nov 29, 2018
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 14, 2018

@mrdoob Is it okay to merge this PR? I think the new description in docs/api/en/materials/Material.html is definitely an improvement.

@mrdoob mrdoob modified the milestones: r100, r101 Dec 31, 2018
@mrdoob mrdoob modified the milestones: r101, r102 Jan 31, 2019
@mrdoob mrdoob modified the milestones: r102, r103 Feb 28, 2019
@mrdoob mrdoob modified the milestones: r103, r104 Mar 27, 2019
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 9, 2019

Closing due to merge conflicts. If there is any interest in changing the description of Material.lights like suggested in this PR, I'll reopen and update it.

@Mugen87 Mugen87 closed this Apr 9, 2019
@looeee
Copy link
Collaborator

looeee commented Apr 9, 2019

I think it's good practice to document when a property on the base class is overwritten by a derived class, so I don't support this change.

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.

5 participants