Skip to content

Unify texture loading and map property fixes #5453

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Unify texture loading and map property fixes #5453

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34bf633
Use updateMapMaterialFromData in updateDistortionMap
mrxz Feb 8, 2024
c0bc685
Allow the various maps on standard and phong to be unset
mrxz Feb 8, 2024
bf92363
Flag texture as needsUpdate when texture properties change
mrxz Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Allow the various maps on standard and phong to be unset
  • Loading branch information
@mrxz
mrxz committed Feb 8, 2024
commit c0bc685ee5caac09c0228ad7029ffe7a5b57c4d7
1 change: 0 additions & 1 deletion src/shaders/flat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ module.exports.Shader = registerShader('flat', {
*/
init: function (data) {
this.materialData = {color: new THREE.Color()};
this.textureSrc = null;
getMaterialData(data, this.materialData);
this.material = new THREE.MeshBasicMaterial(this.materialData);
},
Expand Down
10 changes: 4 additions & 6 deletions src/shaders/phong.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,19 +53,17 @@ module.exports.Shader = registerShader('phong', {
*/
init: function (data) {
this.materialData = { color: new THREE.Color(), specular: new THREE.Color(), emissive: new THREE.Color() };
this.textureSrc = null;
getMaterialData(data, this.materialData);
this.material = new THREE.MeshPhongMaterial(this.materialData);
utils.material.updateMap(this, data);
Copy link
Member

@dmarcos dmarcos Feb 9, 2024

Choose a reason for hiding this comment

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

remove because of redundant?

Copy link
Contributor Author

@mrxz mrxz Feb 9, 2024

Choose a reason for hiding this comment

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

Indeed, this is in the init method, which is followed by update which also calls updateMap. Both flat and standard let the update handle this and phong was the odd one out.

},

update: function (data) {
this.updateMaterial(data);
utils.material.updateMap(this, data);
if (data.normalMap) { utils.material.updateDistortionMap('normal', this, data); }
Copy link
Member

@dmarcos dmarcos Feb 9, 2024

Choose a reason for hiding this comment

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

checks no needed?

Copy link
Contributor Author

@mrxz mrxz Feb 9, 2024

Choose a reason for hiding this comment

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

Same as for standard.js

if (data.displacementMap) { utils.material.updateDistortionMap('displacement', this, data); }
if (data.ambientOcclusionMap) { utils.material.updateDistortionMap('ambientOcclusion', this, data); }
if (data.bump) { utils.material.updateDistortionMap('bump', this, data); }
utils.material.updateDistortionMap('normal', this, data);
utils.material.updateDistortionMap('displacement', this, data);
utils.material.updateDistortionMap('ambientOcclusion', this, data);
utils.material.updateDistortionMap('bump', this, data);
this.updateEnvMap(data);
},

Expand Down
10 changes: 5 additions & 5 deletions src/shaders/standard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ module.exports.Shader = registerShader('standard', {
update: function (data) {
this.updateMaterial(data);
utils.material.updateMap(this, data);
if (data.normalMap) { utils.material.updateDistortionMap('normal', this, data); }
Copy link
Member

@dmarcos dmarcos Feb 9, 2024

Choose a reason for hiding this comment

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

This guards no longer needed?

Copy link
Contributor Author

@mrxz mrxz Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The guards prevented these textures from being removed. Similar to how the main (diffuse) map is handled, the underlying implementation of updateMapMaterialFromData checks if the texture is added, changed or removed, or if only the texture properties are updated.

if (data.displacementMap) { utils.material.updateDistortionMap('displacement', this, data); }
if (data.ambientOcclusionMap) { utils.material.updateDistortionMap('ambientOcclusion', this, data); }
if (data.metalnessMap) { utils.material.updateDistortionMap('metalness', this, data); }
if (data.roughnessMap) { utils.material.updateDistortionMap('roughness', this, data); }
utils.material.updateDistortionMap('normal', this, data);
utils.material.updateDistortionMap('displacement', this, data);
utils.material.updateDistortionMap('ambientOcclusion', this, data);
utils.material.updateDistortionMap('metalness', this, data);
utils.material.updateDistortionMap('roughness', this, data);
this.updateEnvMap(data);
},

Expand Down
21 changes: 21 additions & 0 deletions tests/shaders/standard.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,27 @@ suite('standard material', function () {
});
});

[
{ dataName: 'normalMap', materialName: 'normalMap' },
{ dataName: 'displacementMap', materialName: 'displacementMap' },
{ dataName: 'ambientOcclusionMap', materialName: 'aoMap' },
{ dataName: 'metalnessMap', materialName: 'metalnessMap' },
{ dataName: 'roughnessMap', materialName: 'roughnessMap' }
].forEach(function (names) {
test(`can unset ${names.dataName}`, function (done) {
var el = this.el;
var imageUrl = 'base/tests/assets/test.png';
assert.isNull(el.getObject3D('mesh').material[names.materialName]);
el.setAttribute('material', names.dataName, `url(${imageUrl})`);
el.addEventListener('materialtextureloaded', function (evt) {
assert.equal(el.getObject3D('mesh').material[names.materialName], evt.detail.texture);
el.setAttribute('material', names.dataName, '');
assert.isNull(el.getObject3D('mesh').material[names.materialName]);
done();
});
});
});

test('can use spherical env maps', function (done) {
var el = this.el;
var imageUrl = 'base/tests/assets/test.png';
Expand Down