-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
WebGL stub for unit tests #4827
Conversation
Out of curiosity, could this be used in application-level unit tests for code that uses Cesium as well? |
@markerikson yes, the app's tests would just pass in the WebGL stub when constructing Cesium's |
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.
Looks really nice overall with some good cleanup in some specs.
cubeMap = new CubeMap({ | ||
context : context, | ||
context : cxt, |
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.
Is the abbreviation to avoid overlap with the var context
at the top?
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.
Yes.
expect(context.readPixels()).toEqual([0, 0, 255, 255]); | ||
fragmentShader : fs, | ||
uniformMap : uniformMap | ||
}).contextToRender([0, 0, 255, 255]); |
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.
Is this test actually checking that there are mipmaps present? Beyond the scope of this PR?
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.
Yes, beyond the scope. Update #4817 if you want, but it could be too WebGL implementation-specific to bother.
context : context, | ||
vertexShaderSource : vs, | ||
fragmentShaderSource : fs | ||
}); | ||
sp.destroy(); |
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.
For this and below why destroy the shader program here?
@@ -538,6 +533,10 @@ defineSuite([ | |||
}); | |||
|
|||
it('fails to link', function() { | |||
if (webglStub) { | |||
return; // WebGL Stub does not return vertex attribute and uniforms in the shader |
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.
Comment doesn't really apply to this case and the two above.
expect(pixel).toEqual([0, 255, 0, 255]); | ||
renderMaterial(material1, false, function(rgba) { | ||
expect(rgba).toEqual([0, 255, 0, 255]); | ||
}); |
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.
All these callbacks just check the color, could the expected color be sent to renderMaterial
instead?
stub.vertexAttrib4f = noop; | ||
stub.vertexAttrib4fv = noop; | ||
stub.vertexAttribPointer = noop; | ||
stub.viewport = noop; |
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.
I didn't look closely - does this include WebGL2 functions?
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.
No, that is part of #4817.
|
||
function getStubWarning() { | ||
//>>includeStart('debug', pragmas.debug); | ||
throw new DeveloperError('A stub for this get/is function is not defined. Can it use getStub() or does it need a new 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.
Is this warning message a TODO?
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.
This is for if we start using get*
WebGL functions that we are not currently using, we may may need to implement a custom stub for them. I would rather throw this exception so we know to make the change then try to fly by with undefined
, 0
, false
, {}
, etc.
```javascript | ||
expect(context).toReadPixels([0, 0, 0, 255]); | ||
|
||
expect(context).notToReadPixels([0, 0, 0, 0]); |
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.
Should be [0, 0, 0, 255]
ClearCommand.ALL.execute(context); | ||
expect(context.readPixels()).toEqual([0, 0, 0, 0]); | ||
ClearCommand.ALL.execute(cxt); | ||
expect(cxt).toReadPixels([0, 0, 0, 0]); |
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.
A few places expect [0, 0, 0, 0] instead of [0, 0, 0, 255]. Is there a reason for the discrepancy?
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.
Yes, ClearCommand.ALL
clears alpha to 0.0
.
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.
It is because alpha : true
is used when creating the context.
I cleaned this up in this PR; when I first wrote the tests....6 years ago...for some reason I had the tests default to alpha : true
even though Cesium did not (or maybe it did, then I set it to false
because I heard it made browser compositing faster). Anyway, I think these are all correct.
@@ -775,7 +820,7 @@ defineSuite([ | |||
// I believe different GL implementations are allowed to AA | |||
// in different ways (or at least that is what we see in practice), | |||
// so verify it at least rendered something. | |||
expect(context.readPixels()).not.toEqual([0, 0, 0, 0]); | |||
expect(context).notToReadPixels([0, 0, 0, 0]); |
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.
[0, 0, 0, 255]
There are several places where the vertex shader is sent in as |
|
Do you mean that the default value for |
Thanks for the review, updated. |
@lilleyse for when you merge this into Search/replace
Search and replace by hand
There will also be some other less general cases. Make sure to test in the browser and at the command-line, both with and without using the WebGL stub. |
Yeah you can disregard my comment. I was referring to the fact that those vertex shaders are identical to the default vertex shader in |
In
rather than calling |
Besides that everything looks solid. |
Thanks, updated. |
One last thing - this seems worthy of updating CHANGES.md |
Whoops, added to master: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/CHANGES.md |
Ok. Merging now. I'll get to the |
Fixes #4281. Future work is #4817.
See the updated Testing Guide:
This PR includes some test cleanup, but any additional cleanup is not in the scope of this PR.
Expect some random failures on CI that we'll sort out...have seen just one so far.
TODO
3d-tiles
branch and friendsLabelCollection
CV test, Fix LabelCollection CV bounding sphere test #4816