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 all commits
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
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
74 changes: 28 additions & 46 deletions src/utils/material.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,40 @@ function setTextureProperties (texture, data) {
var offset = data.offset || {x: 0, y: 0};
var repeat = data.repeat || {x: 1, y: 1};
var npot = data.npot || false;
var anisotropy = data.anisotropy || 0;
var anisotropy = data.anisotropy || THREE.Texture.DEFAULT_ANISOTROPY;
var wrapS = texture.wrapS;
var wrapT = texture.wrapT;
var magFilter = texture.magFilter;
var minFilter = texture.minFilter;

// To support NPOT textures, wrap must be ClampToEdge (not Repeat),
// and filters must not use mipmaps (i.e. Nearest or Linear).
if (npot) {
texture.wrapS = THREE.ClampToEdgeWrapping;
texture.wrapT = THREE.ClampToEdgeWrapping;
texture.magFilter = THREE.LinearFilter;
texture.minFilter = THREE.LinearFilter;
wrapS = THREE.ClampToEdgeWrapping;
wrapT = THREE.ClampToEdgeWrapping;
magFilter = THREE.LinearFilter;
minFilter = THREE.LinearFilter;
}

// Don't bother setting repeat if it is 1/1. Power-of-two is required to repeat.
// Set wrap mode to repeat only if repeat isn't 1/1. Power-of-two is required to repeat.
if (repeat.x !== 1 || repeat.y !== 1) {
texture.wrapS = THREE.RepeatWrapping;
texture.wrapT = THREE.RepeatWrapping;
texture.repeat.set(repeat.x, repeat.y);
}
// Don't bother setting offset if it is 0/0.
if (offset.x !== 0 || offset.y !== 0) {
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.

guard 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 guard actually prevented offset from being set back to 0/0 after being set to something else. On top of that, changing the offset doesn't require a texture update, so there's nothing gained by skipping this.

texture.offset.set(offset.x, offset.y);
wrapS = THREE.RepeatWrapping;
wrapT = THREE.RepeatWrapping;
}

// Only set anisotropy if it isn't 0, which indicates that the default value should be used.
if (anisotropy !== 0) {
// Apply texture properties
texture.offset.set(offset.x, offset.y);
texture.repeat.set(repeat.x, repeat.y);

if (texture.wrapS !== wrapS || texture.wrapT !== wrapT ||
texture.magFilter !== magFilter || texture.minFilter !== minFilter ||
texture.anisotropy !== anisotropy) {
texture.wrapS = wrapS;
texture.wrapT = wrapT;
texture.magFilter = magFilter;
texture.minFilter = minFilter;
texture.anisotropy = anisotropy;
texture.needsUpdate = true;
}
}
module.exports.setTextureProperties = setTextureProperties;
Expand Down Expand Up @@ -127,43 +137,15 @@ module.exports.updateMap = function (shader, data) {
module.exports.updateDistortionMap = function (longType, shader, data) {
var shortType = longType;
if (longType === 'ambientOcclusion') { shortType = 'ao'; }
var el = shader.el;
var material = shader.material;
var rendererSystem = el.sceneEl.systems.renderer;
var src = data[longType + 'Map'];

var info = {};
info.src = src;
info.src = data[longType + 'Map'];

// Pass through the repeat and offset to be handled by the material loader.
info.offset = data[longType + 'TextureOffset'];
info.repeat = data[longType + 'TextureRepeat'];
info.wrap = data[longType + 'TextureWrap'];

if (src) {
if (src === shader[longType + 'TextureSrc']) { return; }

// Texture added or changed.
shader[longType + 'TextureSrc'] = src;
el.sceneEl.systems.material.loadTexture(src, info, setMap);
return;
}

// Texture removed.
if (!material.map) { return; }
setMap(null);

function setMap (texture) {
var slot = shortType + 'Map';
material[slot] = texture;
if (texture && COLOR_MAPS.has(slot)) {
rendererSystem.applyColorCorrection(texture);
}
if (texture) {
setTextureProperties(texture, data);
}
material.needsUpdate = true;
handleTextureEvents(el, texture);
}
return module.exports.updateMapMaterialFromData(shortType + 'Map', 'src', shader, info);
};

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

test('updates texture when repeat changes', function (done) {
var imageUrl = 'base/tests/assets/test.png';
el.setAttribute('material', '');
assert.notOk(el.components.material.material.texture);
el.setAttribute('material', 'src: url(' + imageUrl + ')');
el.addEventListener('materialtextureloaded', function (evt) {
var loadedTexture = evt.detail.texture;
assert.ok(el.components.material.material.map === loadedTexture);
var version = loadedTexture.version;
el.setAttribute('material', 'repeat', '2 2');
assert.ok(el.components.material.material.map.version > version);
done();
});
});

test('removes texture when src attribute removed', function (done) {
var imageUrl = 'base/tests/assets/test.png';
el.setAttribute('material', '');
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