Skip to content

Commit

Permalink
Fix fog removal from scene (#5418)
Browse files Browse the repository at this point in the history
* Remove fog object from scene when removing `fog` component

* No need to update materials when scene fog changes

---------

Co-authored-by: Noeri Huisman <[email protected]>
  • Loading branch information
mrxz and mrxz authored Dec 19, 2023
1 parent 2152bc6 commit 8e4f387
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 28 deletions.
6 changes: 3 additions & 3 deletions src/components/scene/fog.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ module.exports.Component = register('fog', {
// (Re)create fog if fog doesn't exist or fog type changed.
if (!fog || data.type !== fog.name) {
el.object3D.fog = getFog(data);
el.systems.material.updateMaterials();
return;
}

Expand All @@ -46,10 +45,11 @@ module.exports.Component = register('fog', {
* Remove fog on remove (callback).
*/
remove: function () {
var el = this.el;
var fog = this.el.object3D.fog;
if (!fog) { return; }
fog.far = 0;
fog.near = 0.1;

el.object3D.fog = null;
}
});

Expand Down
10 changes: 0 additions & 10 deletions src/systems/material.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,16 +222,6 @@ module.exports.System = registerSystem('material', {
});
},

/**
* Trigger update to all registered materials.
*/
updateMaterials: function (material) {
var materials = this.materials;
Object.keys(materials).forEach(function (uuid) {
materials[uuid].needsUpdate = true;
});
},

/**
* Track textures used by material components, so that they can be safely
* disposed when no longer in use. Textures must be registered here, and not
Expand Down
16 changes: 1 addition & 15 deletions tests/components/scene/fog.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ suite('fog', function () {
var self = this;

el.addEventListener('loaded', function () {
self.updateMaterialsSpy = self.sinon.spy(el.systems.material, 'updateMaterials');
// Stub scene load to avoid WebGL code.
el.hasLoaded = true;
el.setAttribute('fog', '');
Expand All @@ -28,10 +27,6 @@ suite('fog', function () {
assert.ok(this.el.object3D.fog);
});

test('triggers material update when adding fog', function () {
assert.ok(this.updateMaterialsSpy.called);
});

test('updates fog', function () {
var el = this.el;
el.setAttribute('fog', 'color: #F0F');
Expand Down Expand Up @@ -67,16 +62,7 @@ suite('fog', function () {
test('removes fog when detaching fog', function () {
var el = this.el;
el.removeAttribute('fog');
assert.equal(el.object3D.fog.far, 0);
assert.equal(el.object3D.fog.near, 0.1);
});

test('removes exp. fog when detaching fog', function () {
var el = this.el;
el.setAttribute('fog', 'type: exponential');
el.removeAttribute('fog');
assert.equal(el.object3D.fog.far, 0);
assert.equal(el.object3D.fog.near, 0.1);
assert.equal(el.object3D.fog, null);
});
});
});

0 comments on commit 8e4f387

Please sign in to comment.