-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Disable unused vertexAttribute to avoid environment-dependent vanishing bugs #5970
Disable unused vertexAttribute to avoid environment-dependent vanishing bugs #5970
Conversation
Prepare an array to record the availability of registers to prevent the same register from being disabled more than once.
In the enableAttrib function, when enabling a register, tell the renderer that it is available by using the location number as an argument.
If the registers corresponding to the attributes that are not used for drawing are left open, the data still stored there will cause problems. So it asks the renderer if the register corresponding to it is open, and if it is, it closes it and notifies the renderer and shader of that state.
Use === instead of ==.
remove trailing space
I haven't read it properly yet, but I found a function in Three.js that looks like this: function disableUnusedAttributes() {
const newAttributes = currentState.newAttributes;
const enabledAttributes = currentState.enabledAttributes;
for ( let i = 0, il = enabledAttributes.length; i < il; i ++ ) {
if ( enabledAttributes[ i ] !== newAttributes[ i ] ) {
gl.disableVertexAttribArray( i );
enabledAttributes[ i ] = 0;
}
}
} This operation makes sense if enableAttributes is 1 and newAttributes is 0 before assigning 0 (since assigning 0 is meaningless). The arguments for these arrays are, I guess, the numbers of the registers (since we are assigning their values to the functions). |
This works great, and the code is very concise! Also, I tested using a shader that references a disabled attribute like I mentioned in my comment on #5968, and it works just fine: https://editor.p5js.org/davepagurek/sketches/vrgwcBpJh One thing I was wondering about is how this handles shaders whose attributes are in different locations and if things might break. However, since we loop through all the buffers each time, each buffer's new location will end up properly enabled/disabled. In the future if we allow custom attributes (#5120) that aren't checked in every render, then we'll have to make sure those get disabled too when we switch to a shader or model that doesn't use them, but that'll be a problem for future me 😅 We can maybe add a unit test or two making sure we put the right values in |
I found a bug. With this logic, if another shader has disabled a register it should use, and if that shader doesn't know about it, it won't be able to re-enable it, and the register will remain disabled forever. In the patch I prepared, the lightingShader uses aTexCoord at number 2, and the colorShader uses aVertexColor at number 2. First, the lightingShader is used to draw the background, and then the colorShader is used to draw the spheres. Register 2 is disabled because the sphere does not have aVertexColor, but the lightingShader does not know about it, so it remains disabled. To avoid this, in the enableAttrib function, it is necessary to consider not only the enabled information of the corresponding shader, but also the enabled state of the corresponding register. |
If the shader doesn't know that the register has been disabled by another shader, it can't enable it. Therefore, even if the corresponding shader does not know about it, if the register used by it is disabled, enable processing should be performed.
good catch, and thanks for looking into that some more! I didn't look closely enough at the code that enables attributes to notice that it was only conditionally calling |
Since only the global array (named "registerEnabled") needs to know the enabled/disabled state of a register, we eliminate the "enabled" property of attribute.
It is impossible for the shader itself to know when a register is available or not. I decided to leave that to the global array. So I'm going to remove that property.
Here is a test to try rewriting the previous code: debug_test What is causing this bug? This is because the shader maintains information about the enabled/disabled state of registers, and p5.js can only enable registers. First, what does shader attributes enabled mean? It means that the register corresponding to the shader's attribute is enabled and that the register contains some number. Whatever the number, attribute is enabled if the register is enabled. Because that data is used during the draw call. But the shader's attribute's enabled property is not true. It's valid, but it's still false. In order to do this properly, it is necessary to create a horizontal connection in the shader and some other method is required, but it is much easier to manage the on/off of the register globally. For that reason, I decided to prepare an array called "registerEnabled" to manage the flags. Also, this problem is caused by the shader not being fed data from the geometry and leaving a register that should not be used open even though it contains bad data, so it must be closed. By the way, regarding the unit test, I'm thinking of making sure that the value of this global flag is set properly. Please wait for a while... |
I created a test like this: suite('Test for register availability', function() {
test('register enable/disable flag test', function(done) {
const renderer = myp5.createCanvas(16, 16, myp5.WEBGL);
// geometry without aTexCoord.
const myGeom = new p5.Geometry(1, 1, function() {
this.gid = 'registerEnabledTest';
this.vertices.push(myp5.createVector(-8, -8));
this.vertices.push(myp5.createVector(8, -8));
this.vertices.push(myp5.createVector(8, 8));
this.vertices.push(myp5.createVector(-8, 8));
this.faces.push([0, 1, 2]);
this.faces.push([0, 2, 3]);
this.computeNormals();
});
myp5.fill(255);
myp5.directionalLight(255, 255, 255, 0, 0, -1);
myp5.triangle(-8, -8, 8, -8, 8, 8);
// get register location of
// lightingShader's aTexCoord attribute.
const attributes = renderer._curShader.attributes;
const loc = attributes.aTexCoord.location;
assert.equal(renderer.registerEnabled[loc], true);
myp5.model(myGeom);
assert.equal(renderer.registerEnabled[loc], false);
myp5.triangle(-8, -8, 8, 8, -8, 8);
assert.equal(renderer.registerEnabled[loc], true);
done();
});
}); This test draws a triangle with aTexCoord and a custom square geometry without aTexCoord. After drawing the triangle, the availability of the register used by the lightingShader's aTexCoord should be true if the registerEnabled value is set appropriately. And after drawing geometry without aTexCoord, the register should be closed and this value should be false. Finally, if we draw the triangle with aTexCoord again, it should return true again. |
I decided to use a global flag to manage the enabled/disabled state of the registers used to store shader attributes. This value will be true if the geometry supplies a value, false if it does not. Therefore, I prepared a geometry with aTexCoord and a geometry without it, and tested whether the value was properly switched.
I accidentally wrote "myp5.createVector" as "createVector". Just a typo. Excuse me.
I have a few concerns that I would like to mention. Also, You mentioned custom attributes, but I made a sample before. What I did was add a new one to the Retained mode RenderBuffer by pushing. |
Ah I see, it's not getting recognized as an attribute because it's getting optimized away. If, in the fragment shader, I reference
Ah I see, as long as we're adding custom buffers to the global list of buffers, then everything should be good, as it will make p5 check for is existence and disable it if you render subsequent geometry that doesn't have data for that buffer. Looks good! |
I think I've done everything I can. I've been trying all week to find the cause of this phenomenon. I was able to identify the cause of the problem and create an example to reproduce it. I made a patch and tried to see if my sketch in existing OpenProcessing didn't cause any problems. |
@@ -563,9 +563,11 @@ p5.Shader.prototype.enableAttrib = function( | |||
const loc = attr.location; | |||
if (loc !== -1) { | |||
const gl = this._renderer.GL; | |||
if (!attr.enabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, we don't need this any more per shader attribute, now that we have a global one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for diagnosing the issue and coming up with this solution! The code makes sense, and I haven't been able to break it in my tests when switching between different shaders/geometry, so I think this is good to go!
I later learned that "register" is not an official term, the official term is "vertexAttribute". I apologize for any confusion this may have caused. However, as for the variable name, I think that "register" is easier to understand in the sense that it is the place where information about the buffer is registered, so I would like to leave it as it is. |
Resolves #5968
In some environments, such as my favorite Android phone, rendering in webgl of p5.js may not be rendered correctly.
For example, see the sketch below.
minimum_bug_demo
My Android doesn't show the blue square in the middle.
On the other hand, my laptop at home displays squares.
Problems like this occur when some shader attributes are unused and the corresponding registers are not closed, causing the data in them to be corrupted. Therefore, I would like to propose a specification change to close unused registers each time so that such a thing does not occur.
It also records information about whether a register is closed or not, so that unnecessary closing instructions are not executed.
Changes:
PR Checklist
npm run lint
passes