Closed
Bug 569714
Opened 14 years ago
Closed 14 years ago
Pass webgl gl-get-calls test
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
People
(Reporter: bjacob, Assigned: bjacob)
Details
Attachments
(3 files)
7.00 KB,
patch
|
Details | Diff | Splinter Review | |
12.24 KB,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
12.22 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
Note: this is NOT the same test as in bug 567499 !! Despite the similar name. This patch makes this test fully pass, except for a bug in the test itself at the end: shouldBe('context.getParameter(context.SHADER_BINARY_FORMATS)', '[]'); There is no SHADER_BINARY_FORMATS constant in WebGL.
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Comment 2•14 years ago
|
||
Forgot to mention: the test's URL is: https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/fast/gl-get-calls.html
Ah, so this is the one that was checked in by accident. Ok; let's just do followup patches -- the changes aren't bad, just have smoe problems. So, in the IDL, this currently looks like: void getParameter(in WebGLenum pname); Where the return type is 'void' because we just do magic here and set the JS retval directly. However, that was when we only returned simple types; now, we need to return, e.g., a nsIWebGLTexture in some cases. So the easiest thing to do would be to have this return nsIVariant. With a nsIVariant return, take a look at xpcom/ds/nsIVariant.idl; you'll need to create a new nsIWritableVariant and call the appropriate set function, and then set the retval to it. We won't need any of the NativeCallContext stuff or anything like that any more in the function. For returning arrays, use SetAsArray, e.g.: PRBool fourBools[4]; ...->SetAsArray(nsIDataType::VTYPE_BOOL, nsnull, 4, reinterpret_cast<void *>(fourBoolds)); For returning webgl objects, you can use SetAsISupports(obj);
Summary: Pass Khrono's gl-get-calls test → Pass webgl gl-get-calls test
Assignee | ||
Comment 4•14 years ago
|
||
OK, here's the patch. I also fixed other pnames, and wrote to public_webgl about the ones that I really can't fix.
Attachment #450216 -
Flags: review?(vladimir)
Comment on attachment 450216 [details] [diff] [review] patch using nsIVariant Looks good, with these fixes: >+ nsCOMPtr<nsIWritableVariant> wrval = do_CreateInstance("@mozilla.org/variant;1"); Add a NS_ENSURE_SUCCESS(wrval, NS_ERROR_FAILURE); after this >- //case LOCAL_GL_IMPLEMENTATION_COLOR_READ_TYPE: >- //case LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT: >+ case LOCAL_GL_IMPLEMENTATION_COLOR_READ_TYPE: >+ case LOCAL_GL_IMPLEMENTATION_COLOR_READ_FORMAT: These aren't valid enums on the desktop; leave them commented out until we figure out what to do with the whole set of 4. > // javascript integer values. We just return them as double's and javascript doesn't care. Can you fix the typo here, while you're touching this code? "double's" -> "doubles" >- case LOCAL_GL_COLOR_WRITEMASK: // 4 bools >+ case LOCAL_GL_COLOR_WRITEMASK: // array 4 uint8's These really are 4 booleans, even though you need to grab them as 4 uint8 values. >+ realGLboolean gl_bv[4] = { 0 }; >+ gl->fGetBooleanv(pname, gl_bv); >+ // it might be relatively safe to assume that realGLboolean is 1 byte, >+ // but it's probably not entirely safe and this is not perf-critical anyway. >+ PRUint8 pr_bv[4] = { gl_bv[0], gl_bv[1], gl_bv[2], gl_bv[3] }; ^ PRBool, not PRUint8 >+ wrval->SetAsArray(nsIDataType::VTYPE_UINT8, nsnull, VTYPE_BOOL, not UINT8. You want these to be bools on the JS side, not 0/1 values.
Attachment #450216 -
Flags: review?(vladimir) → review+
Assignee | ||
Comment 6•14 years ago
|
||
The new patch addresses your comments, except: > Add a NS_ENSURE_SUCCESS(wrval, NS_ERROR_FAILURE); after this This gave me a compiler error, but wrval!=nsnull worked. >> + PRUint8 pr_bv[4] = { gl_bv[0], gl_bv[1], gl_bv[2], gl_bv[3] }; > > ^ PRBool, not PRUint8 > >> + wrval->SetAsArray(nsIDataType::VTYPE_UINT8, nsnull, > > VTYPE_BOOL, not UINT8. You want these to be bools on the JS side, not > 0/1values. The WebGL spec really says Uint8Array for GL_COLOR_WRITEMASK. And the gl-get-calls really wants numbers, not bools. Are they wrong?
(In reply to comment #6) > The new patch addresses your comments, except: > > > Add a NS_ENSURE_SUCCESS(wrval, NS_ERROR_FAILURE); after this > > This gave me a compiler error, but wrval!=nsnull worked. Sorry, my bad - NS_ENSURE_TRUE(wrval, NS_ERROR_FAILURE); > > >> + PRUint8 pr_bv[4] = { gl_bv[0], gl_bv[1], gl_bv[2], gl_bv[3] }; > > > > ^ PRBool, not PRUint8 > > > >> + wrval->SetAsArray(nsIDataType::VTYPE_UINT8, nsnull, > > > > VTYPE_BOOL, not UINT8. You want these to be bools on the JS side, not > > 0/1values. > > The WebGL spec really says Uint8Array for GL_COLOR_WRITEMASK. And the > gl-get-calls really wants numbers, not bools. Are they wrong? Hmm -- I think so, the GL spec specifies that these are booleans (though in C they're returned as 8-bit valus) -- the glColorMask entry point takes booleans, so I think it should be bools. Send mail to the list?
Assignee | ||
Comment 9•14 years ago
|
||
ok, mail sent.
Assignee | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ab07b8f1593d
Attachment #448835 -
Flags: review?(vladimir)
You need to log in
before you can comment on or make changes to this bug.
Description
•