| Summary: | Must enable GL_VERTEX_PROGRAM_POINT_SIZE during initialization | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||||||
| Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | cmarrin, commit-queue, dglazkov, fishd, kbr, oliver, zmo | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Kenneth Russell
2010-04-06 16:40:45 PDT
Created attachment 52988 [details]
patch
Comment on attachment 52988 [details] patch > Index: LayoutTests/fast/canvas/webgl/point-size.html > =================================================================== > --- LayoutTests/fast/canvas/webgl/point-size.html (revision 0) > +++ LayoutTests/fast/canvas/webgl/point-size.html (revision 0) > @@ -0,0 +1,113 @@ > +<html> > +<head> > +<link rel="stylesheet" href="../../js/resources/js-test-style.css"/> > +<script src="../../js/resources/js-test-pre.js"></script> > +<script src="resources/webgl-test.js"></script> > +<script src="resources/utils3d.js"></script> > +<script id="vshader" type="x-shader/x-vertex"> > +attribute vec3 pos; > +attribute vec4 colorIn; > +uniform float pointSize; > +varying vec4 color; > + > +void main() > +{ > + gl_PointSize = pointSize; > + color = colorIn; > + gl_Position = vec4(pos.xyz, 1.0); > +} > +</script> > + > +<script id="fshader" type="x-shader/x-fragment"> > +varying vec4 color; > + > +void main() > +{ > + gl_FragColor = color; > +} > +</script> > + > +<script> > +var successfullyParsed = false; > + > +// These two declarations need to be global for "shouldBe" to see them > +var gl = null; > +var pixel = [0, 255, 0]; > + > +function init() > +{ > + if (window.layoutTestController) { > + layoutTestController.overridePreference("WebKitWebGLEnabled", "1"); > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); > + } This more complex framework (using layoutTestController.waitUntilDone()) isn't needed for this test. It is only needed when resources, like textures, are being downloaded in the background and the test needs to wait for them to download in order to run to completion. You can use tex-sub-image-2d.html as the basis for this test. We should use the simpler framework so it will be easier to port these to the Khronos site later. > + > + description('Verify GL_VERTEX_PROGRAM_POINT_SIZE is enabled in WebGL'); > + > + runTest(); > +} > + > +function runTest() > +{ > + shouldBeNonNull("gl = initWebGL('testbed', 'vshader', 'fshader', ['pos', 'colorIn'], [0, 0, 0, 1], 1, 0)"); > + gl.disable(gl.BLEND); > + gl.clear(gl.COLOR_BUFFER_BIT); > + > + var vertices = new WebGLFloatArray([ > + 0.5, 0.5, 0.0]); > + var colors = new WebGLUnsignedByteArray([ > + 255, 0, 0, 255]); > + > + var colorOffset = vertices.byteLength; This test should query gl.getParameter(gl.ALIASED_POINT_SIZE_RANGE) and see whether the range covers [1.0, 2.0]. If it turns out the implementation doesn't support a size greater than 1.0, the test should report that it passes. While this behavior won't work with the -expected.txt file, it will be semantically correct and will work within the Khronos conformance test harness. > + var vbo = gl.createBuffer(); > + gl.bindBuffer(gl.ARRAY_BUFFER, vbo); > + gl.bufferData(gl.ARRAY_BUFFER, colorOffset + colors.byteLength, gl.STATIC_DRAW); > + gl.bufferSubData(gl.ARRAY_BUFFER, 0, vertices); > + gl.bufferSubData(gl.ARRAY_BUFFER, colorOffset, colors); > + > + gl.vertexAttribPointer(0, 3, gl.FLOAT, false, 0, 0); > + gl.enableVertexAttribArray(0); > + gl.vertexAttribPointer(1, 4, gl.UNSIGNED_BYTE, true, 0, colorOffset); > + gl.enableVertexAttribArray(1); > + > + var locPointSize = gl.getUniformLocation(gl.program, 'pointSize'); A comment (or debug statement) would be helpful here, like "Draw a point of size 1 and verify it doesn't touch any other pixels." > + gl.uniform1f(locPointSize, 1.0); > + gl.drawArrays(gl.POINTS, 0, vertices.length / 3); > + var buf = gl.readPixels(0, 0, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE); > + pixel[0] = buf[0]; > + pixel[1] = buf[1]; > + pixel[2] = buf[2]; > + shouldBeTrue("pixel[0] == 0 && pixel[1] == 0 && pixel[2] == 0"); > + > + gl.clear(gl.COLOR_BUFFER_BIT); A comment (or debug statement) would be helpful here, like "Draw a point of size 2 and verify it fills the appropriate region." > + gl.uniform1f(locPointSize, 2.0); > + gl.drawArrays(gl.POINTS, 0, vertices.length / 3); > + buf = gl.readPixels(0, 0, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE); I think you should read the pixel at (0, 1) (or (1, 0)) rather than (0, 0). I believe that one should be guaranteed to be filled with exactly the intended color. > + pixel[0] = buf[0]; > + pixel[1] = buf[1]; > + pixel[2] = buf[2]; > + shouldBeTrue("pixel[0] > 0 && pixel[0] <= 255 && pixel[1] == 0 && pixel[2] == 0"); > + > + successfullyParsed = true; > + var epilogue = document.createElement("script"); > + epilogue.onload = finish; > + epilogue.src = "../../js/resources/js-test-post.js"; > + document.body.appendChild(epilogue); > +} > + > +function finish() { > + if (window.layoutTestController) { > + layoutTestController.notifyDone(); > + } > +} > +</script> > +</head> > +<body onload="init()"> > +<canvas id="testbed" width="2px" height="2px"></canvas> > +<div id="description"></div> > +<div id="console"></div> > +</body> > +</html> Created attachment 53019 [details]
revised patch : responding to Ken Russell's review
Created attachment 53021 [details]
revised patch: update the expected test output
Created attachment 53022 [details]
revised patch: modified a few comments in the test
Comment on attachment 53022 [details]
revised patch: modified a few comments in the test
Looks great. Excellent work on the test case for this.
Comment on attachment 53022 [details]
revised patch: modified a few comments in the test
I don't really understand what this does, or "why" we need to enable this. I believe you both, would just be nice to document "why" in the ChangeLog.
(In reply to comment #7) > (From update of attachment 53022 [details]) > I don't really understand what this does, or "why" we need to enable this. I > believe you both, would just be nice to document "why" in the ChangeLog. The description in the bug report concisely describes why it is needed. Is that sufficient? Comment on attachment 53022 [details]
revised patch: modified a few comments in the test
rs=me given kbr approves.
Comment on attachment 53022 [details] revised patch: modified a few comments in the test Clearing flags on attachment: 53022 Committed r57694: <http://trac.webkit.org/changeset/57694> All reviewed patches have been landed. Closing bug. |