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

support uniform texture and matrix array #1162

Merged
merged 2 commits into from Jul 12, 2019
Merged

support uniform texture and matrix array #1162

merged 2 commits into from Jul 12, 2019

Conversation

jianhuang01
Copy link
Contributor

Background

Need support binding texture and matrix array

Change List

  • add uniform names for array

@coveralls
Copy link

coveralls commented Jul 11, 2019

Coverage Status

Coverage increased (+0.03%) to 46.493% when pulling 6ab346d on uniform_array into 9a4606c on master.

@Pessimistress
Copy link
Collaborator

Add a unit test?

@jianhuang01
Copy link
Contributor Author

Add a unit test?

of course.

this._uniformSetters[name] = getUniformSetter(gl, location, info, isArray);
if (info.size > 1) {
for (let l = 0; l < info.size; l++) {
location = gl.getUniformLocation(this.handle, `${name}[${l}]`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to load data for the entire array at once, rather than treating them as separate uniforms. I.e. these two are equivalent for a uniform uniform vec2 myVec2s[2]

const location = gl.getUniformLocation(program, "myVec2s");
gl.uniform2fv(location, new Float32Array([1, 2, 3, 4]);
const location1 = gl.getUniformLocation(program, "myVec2s[0]");
gl.uniform2fv(location1, new Float32Array([1, 2]);
const location2 = gl.getUniformLocation(program, "myVec2s[1]");
gl.uniform2fv(location2, new Float32Array([3, 4]);

But I prefer the former because it involves fewer API calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not only for texture array but also for matrix array, for matrix array we need bind them one by one anyway, so this change is need anyway. For texture array there are ways to improve just as you proposed, we can add a new change on the texture binding part later on because there are more changes involved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Matrices can be handled in the same way, you pass the data for all matrices at the same time.

The texture case is more complicated though, since we'd need a change in luma to take an array of textures and turn it into an array of texture unit values. So I'm ok with this update.

@jianhuang01 jianhuang01 merged commit aa7e1d7 into master Jul 12, 2019
@jianhuang01 jianhuang01 deleted the uniform_array branch July 12, 2019 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants