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

Remove attributes array and cleanup Shader #5310

Merged
merged 2 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 14 additions & 22 deletions src/core/shader.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,8 @@ Shader.prototype = {
* Called during shader initialization and is only run once.
*/
init: function (data) {
this.attributes = this.initVariables(data, 'attribute');
this.uniforms = this.initVariables(data, 'uniform');
this.uniforms = this.initUniforms();
this.material = new (this.raw ? THREE.RawShaderMaterial : THREE.ShaderMaterial)({
// attributes: this.attributes,
uniforms: this.uniforms,
glslVersion: this.raw || this.glsl3 ? THREE.GLSL3 : null,
vertexShader: this.vertexShader,
Expand All @@ -62,18 +60,18 @@ Shader.prototype = {
return this.material;
},

initVariables: function (data, type) {
initUniforms: function () {
var key;
var schema = this.schema;
var variables = {};
var varType;

for (key in schema) {
if (schema[key].is !== type) { continue; }
if (schema[key].is !== 'uniform') { continue; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should 'is' property be removed from schema and all shaders updated accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did contemplate that as well, but I think it's safer to leave it. It can also serve some purpose. Say for example you rename or deprecate a uniform on your shader. Now you can add a property to the schema under the old name, but not mark it as a uniform. By overloading the update method you can then print a warning message and delegate to the old behaviour without it ending up as a uniform.

varType = propertyToThreeMapping[schema[key].type];
variables[key] = {
type: varType,
value: undefined // Let updateVariables handle setting these.
value: undefined // Let update handle setting these.
};
}
return variables;
Expand All @@ -86,36 +84,30 @@ Shader.prototype = {
* @param {object} data - New material data.
*/
update: function (data) {
this.updateVariables(data, 'attribute');
this.updateVariables(data, 'uniform');
},

updateVariables: function (data, type) {
var key;
var materialKey;
var schema = this.schema;
var variables;
var uniforms = this.uniforms;

variables = type === 'uniform' ? this.uniforms : this.attributes;
for (key in data) {
if (!schema[key] || schema[key].is !== type) { continue; }
if (!schema[key] || schema[key].is !== 'uniform') { continue; }

if (schema[key].type === 'map') {
// If data unchanged, get out early.
if (!variables[key] || variables[key].value === data[key]) { continue; }
if (!uniforms[key] || uniforms[key].value === data[key]) { continue; }

// Special handling is needed for textures.
materialKey = '_texture_' + key;

// We can't actually set the variable correctly until we've loaded the texture.
this.setMapOnTextureLoad(variables, key, materialKey);
this.setMapOnTextureLoad(uniforms, key, materialKey);

// Kick off the texture update now that handler is added.
utils.material.updateMapMaterialFromData(materialKey, key, this, data);
continue;
}
variables[key].value = this.parseValue(schema[key].type, data[key]);
variables[key].needsUpdate = true;
uniforms[key].value = this.parseValue(schema[key].type, data[key]);
uniforms[key].needsUpdate = true;
}
},

Expand All @@ -141,11 +133,11 @@ Shader.prototype = {
}
},

setMapOnTextureLoad: function (variables, key, materialKey) {
setMapOnTextureLoad: function (uniforms, key, materialKey) {
var self = this;
this.el.addEventListener('materialtextureloaded', function () {
variables[key].value = self.material[materialKey];
variables[key].needsUpdate = true;
uniforms[key].value = self.material[materialKey];
uniforms[key].needsUpdate = true;
});
}
};
Expand All @@ -170,7 +162,7 @@ module.exports.registerShader = function (name, definition) {
});

if (shaders[name]) {
throw new Error('The shader ' + name + ' has been already registered');
throw new Error('The shader ' + name + ' has already been registered');
}
NewShader = function () { Shader.call(this); };
NewShader.prototype = Object.create(Shader.prototype, proto);
Expand Down
4 changes: 2 additions & 2 deletions src/shaders/msdf.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ module.exports.Shader = registerShader('msdf', {

fragmentShader: FRAGMENT_SHADER,

init: function (data) {
init: function () {
this.uniforms = THREE.UniformsUtils.merge([
THREE.UniformsLib.fog,
this.initVariables(data, 'uniform')
this.initUniforms()
]);
this.material = new THREE.ShaderMaterial({
uniforms: this.uniforms,
Expand Down
4 changes: 2 additions & 2 deletions src/shaders/sdf.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,10 @@ module.exports.Shader = registerShader('sdf', {

fragmentShader: FRAGMENT_SHADER,

init: function (data) {
init: function () {
this.uniforms = THREE.UniformsUtils.merge([
THREE.UniformsLib.fog,
this.initVariables(data, 'uniform')
this.initUniforms()
]);
this.material = new THREE.ShaderMaterial({
uniforms: this.uniforms,
Expand Down
32 changes: 3 additions & 29 deletions tests/core/shader.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ suite('shader', function () {
assert.ok(shader.prototype.vertexShader);
assert.ok(shader.prototype.fragmentShader);
assert.notOk(shader.prototype.uniforms);
assert.notOk(shader.prototype.attributes);
});

test('shader instance receives methods and properties', function () {
Expand All @@ -49,7 +48,6 @@ suite('shader', function () {
assert.equal(instance.vertexShader, shader.prototype.vertexShader);
assert.equal(instance.fragmentShader, shader.prototype.fragmentShader);
assert.equal(Object.keys(instance.uniforms).length, 0);
assert.equal(Object.keys(instance.attributes).length, 0);
assert.ok(instance.material);
});

Expand Down Expand Up @@ -94,8 +92,7 @@ suite('shader data binding', function () {
src: {type: 'map', is: 'uniform'},
otherMap: {type: 'map', is: 'uniform'},
vec2Uniform: {type: 'vec2', default: {x: 1, y: 2}, is: 'uniform'},
vec2Attribute: {type: 'vec2', default: {x: 3, y: 4}, is: 'attribute'},
vec2Neither: {type: 'vec2', default: {x: 5, y: 6}}
vec2NotUniform: {type: 'vec2', default: {x: 5, y: 6}}
}
});

Expand Down Expand Up @@ -126,8 +123,6 @@ suite('shader data binding', function () {
assert.ok(updateSpy.calledOnce);
// The value won't be assigned until the texture loads.
assert.ok(instance.uniforms['src']);
assert.notOk(instance.attributes && (instance.attributes['map'] ||
instance.attributes['src']));
});

test('src loads inline video', function (done) {
Expand All @@ -152,8 +147,6 @@ suite('shader data binding', function () {
assert.ok(updateSpy.calledOnce);
// The value won't be assigned until the texture loads.
assert.ok(instance.uniforms['src']);
assert.notOk(instance.attributes && (instance.attributes['map'] ||
instance.attributes['src']));
});

test('otherMap loads inline video', function (done) {
Expand All @@ -178,8 +171,6 @@ suite('shader data binding', function () {
assert.ok(initSpy.calledOnce);
assert.ok(updateSpy.calledOnce);
assert.ok(instance.uniforms['otherMap']);
// The value won't be assigned until the texture loads.
assert.notOk(instance.attributes && instance.attributes['otherMap']);
});

test('vec2Uniform parameter is uniform', function () {
Expand All @@ -194,25 +185,9 @@ suite('shader data binding', function () {
assert.ok(instance.uniforms['vec2Uniform']);
assert.equal(instance.uniforms['vec2Uniform'].value.x, 1);
assert.equal(instance.uniforms['vec2Uniform'].value.y, 2);
assert.notOk(instance.attributes['vec2Uniform']);
});

test('vec2Attribute parameter is attribute', function () {
assert.notOk(initSpy.called);
assert.notOk(updateSpy.called);
el.setAttribute('material', 'shader: testShader');
const material = el.components.material;
const instance = material.shader;
assert.ok(instance);
assert.ok(initSpy.calledOnce);
assert.ok(updateSpy.calledOnce);
assert.ok(instance.attributes['vec2Attribute']);
assert.equal(instance.attributes['vec2Attribute'].value.x, 3);
assert.equal(instance.attributes['vec2Attribute'].value.y, 4);
assert.notOk(instance.uniforms['vec2Attribute']);
});

test('vec2Neither parameter is neither uniform nor attribute', function () {
test('vec2NotUniform parameter is not a uniform', function () {
assert.notOk(initSpy.called);
assert.notOk(updateSpy.called);
el.setAttribute('material', 'shader: testShader');
Expand All @@ -221,7 +196,6 @@ suite('shader data binding', function () {
assert.ok(instance);
assert.ok(initSpy.calledOnce);
assert.ok(updateSpy.calledOnce);
assert.notOk(instance.attributes['vec2Neither']);
assert.notOk(instance.uniforms['vec2Neither']);
assert.notOk(instance.uniforms['vec2NotUniform']);
});
});
Loading