-
Notifications
You must be signed in to change notification settings - Fork 1
Scalable Ambient Obscurance #10
base: master
Are you sure you want to change the base?
Conversation
geometryPass.drawElements(ground.indices); | ||
|
||
// Since we're going to use multiple pipelines, we have to unset buffers. | ||
geometryPass.cleanup(); |
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 there a better way to avoid having to call cleanup
when shader programs change without managing global state? one idea I had was to do something like this:
cgl.withPipeline(geometryPass, (p) => {
p.vertexPosition = teapot.vertexPositions;
// ...
});
so that it can automatically wrap the calls in a useProgram
and cleanup
. Thoughts on if that's any better, or if it just complicates it too much?
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 think useProgram
should come with a cleanup callback parameter. I feel like buffer management is something that should be thought about, so users should have to think about it every time they run a shader program. Though this may be cumbersome in common cases, so we can probably provide some pre-defined callbacks like clearAll
and some no-op callback.
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.
making sure you think about cleanup sounds good, maybe it only seems like an extra thing to think about in vanilla webgl because there's so much else to keep in your head at once.
Also to make sure I'm on the same page, by cleanup callback parameter do you mean something like this?
cgl.useProgram(geometryPass, (cleanup) => {
// ...
geometryPass.draw();
cleanup();
})
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 actually meant something along the lines of:
cgl.useProgram(geometryPass, cleanup)
A bit more restricted than allowing cleanup to be called anywhere, but it should always be called at the end IMO, and this way a custom cleanup function can be passed in.
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.
When would we call the callback? the next time we do a useProgram
call? (I think in the code I have right now it's geometryPass.useProgram()
but if we put it on the cgl object like cgl.useProgram(geometryPass)
then we could keep track of when the switch happens I guess.)
In terms of what would happen in cleanup, the main thing I was thinking of was to unset the framebuffer so that it stops rendering to it, but presumably you might want to render to the same buffers from another program. In the way I've got the current code, the pipeline "owns" the buffer textures, so I should definitely take them out of the shader DSL to let them be their own entities.
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.
Ah, right, forgot that useProgram
basically just sets a buffer.
I guess we need a way to represent that only a single shader pipeline is active at a time, right? And once we deactivate one pipeline, we want to run some cleanup code.
This sort of thing feels tricky to do nicely without language support for things like RAII. Do you think it would be worthwhile to make a separate DSL for WebGL configuration (basically anything that calls gl.SOME_FUNCTION
)? I feel like this would also help with other decisions, but we haven't thought a whole lot about what this will look like.
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.
Though it seems difficult to integrate that nicely, so maybe we don't need a full language. But possibly something worth thinking about.
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 think it could be useful, so here are the thoughts I have initially:
One direction could be maybe less about typechecking and more like a good set of functions and abstractions for wiring together pipelines in js. In a pretend version of this demo that has more than one material, it could maybe look like this:
const position = cgl.texture2D({format: cgl.RGBA});
const depth = cgl.texture2D({format: cgl.DEPTH});
window.requestAnimationFrame(() => {
// potential implementation:
// withPipeline(pipeline, callback) {
// pipeline.useProgram();
// gl.bindFramebuffer(gl.FRAMEBUFFER, pipeline._fb);
// callback(pipeline);
// gl.bindFramebuffer(gl.FRAMEBUFFER, null);
// }
cgl.withPipeline(geometryPassPhong, (ctx) => {
// set for writing
ctx.positionBuf = position;
ctx.depthBuf = depth;
// ...
ctx.drawElements(obj1.indices);
// ...
ctx.drawElements(obj2.indices);
});
cgl.withPipeline(geometryPassReflective, (ctx) => {
ctx.positionBuf = position;
ctx.depthBuf = depth;
// ...
ctx.drawElements(obj3.indices);
});
ctx.withPipeliene(finalPass, (ctx) => {
// set for reading
ctx.positionTexture = position;
// ...
ctx.draw(ctx.SCREEN);
});
});
Alternatively it might be useful to put the whole thing in a dsl instead of js so that we can assert that all the buffers have been assigned (and maybe explicitly pass in the screen as a render target as if it's any other buffer), but I'm not sure what a good way of doing that is because at some point we have to move the data from js land to opengl land and that will always be untypecheckable statically.
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.
The ctx
everywhere reminded me of Go :p
That being said, I think that your suggestion of a good API be a good idea. I have some more concrete ideas after looking at what you've written out here, but I'll post them tomorrow once I've worked out the details a bit more. It would be better if we don't make this a DSL because then it will be awkward to interleave with native JS.
examples/sao/sao-calder.js
Outdated
// Buffers have a format and properties that can be initialized in C struct | ||
// format, or just left as sane defaults. (In WebGL 2, the format and internal | ||
// format of a texture can be different, but in WebGL 1 they must be the same) | ||
buffer rgba diffuseBuf { |
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.
Considerations I had for doing this:
- shouldn't have to specify a bunch of properties if the defaults are good
- if you do want to specify properties, it should be declarative
- it should be clear what property is being specified (e.g. with key-value pairs instead of positional arguments)
- we need a type for the buffer
- all buffers have the same size
- there is a max number of buffers
- you can optionally have one depth buffer
- you don't need a depth buffer, but rn if you forget to add one, you just get super weird visuals with no depth ordering, so it should be easy to do and hard to forget
I'm not certain this way of setting up buffers totally covers this. Mostly the last point about forgetting a depth buffer isn't really covered. Thoughts on how to do that?
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.
Could we model properties for different buffer types (like TEXTURE_2D, TEXTURE_CUBE_MAP) with something like a JSON object? Of course, we'd need to figure out a way to make sure everything is well-typed, but it would be a familiar format, and easy to declaratively assign property values.
I actually like the idea of providing some sort of configuration schema object, because then it can also be documentation in code for what different properties are (vs reading it in a spec).
Also, I don't think this belongs in a shader? (or is this some WebGL 2.0-like feature)
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.
The reason I put it in a shader is that it means we can see it at compile time. The problem with putting it anywhere else is that we'd need to parse the javascript fully to be able to make any assertions on it, and even then it'd be weird because of things like aliasing the variable. That's not a very good reason to add it to the shader though, because you're right, it really doesn't belong there. Are we ok having it be a runtime check and not a compile time 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.
Well, "compile time" here is really still at run time, right? I think it's fine for it to be outside of the DSL realm, since we already have other things that are outside of the DSL. The fact that this needs to be type-checked could require that we require all values to be wrapped in type objects to annotate them properly, and verify them against a schema object. Don't worry if it doesn't make sense since it's a half-baked idea, but I think that, now that we've sort of decided on making the DSL specifically for shaders, we can move configuration outside of the DSL (though we need to figure out how to a nice way way to type-check this config code).
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.
By forcing them to be created all at once by the shader that forces them all to be the same size, but honestly it's plausible you'd want to use the same program to render once to a set of textures A and then a second time to a different set of textures B. There would be no easy way to check that statically, so maybe we can just commit to a dynamic check for buffer sizes and make a decent js API for initializing textures. Something like:
const someTexture = cgl.texture2D({format: cgl.RGBA});
// assigning to a buffer property (as opposed to a normal texture) automatically
// connects it as a color attachment
geometryPass.diffuseBuf = someTexture;
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.
by "them" I mean the buffers (e.g. if youre rendering to color, position, and normal buffers from one shader, you all have to be the same resolution.)
Do we get much benefit from statically typechecking a call like the texture2D
one above if it'd be in the initialization part of your js code? like is it worth trying to statically check the config?
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.
Hm, I guess we don't get much benefit from config object checks, since WebGL doesn't do its own checks (to my knowledge).
To check that buffers have the same size, we could make type wrapper objects and wrap buffers in them. These can be checked to verify that all the buffers have the same size. I don't know what this will look like exactly, but maybe something like:
colorBuffer = cgl.BufferV3(...)
positionBuffer = cgl.BufferV3(...)
...
// check that colorBuffer and positionBuffer are the same size
(I'm still not sure what you mean by rendering to a buffer, so the code sample above isn't complete)
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.
By rendering to a buffer, I mean a potentially offscreen framebuffer (the canvas has a framebuffer, but maybe you want to render to a side buffer first, and then read that buffer and use that when writing to the screen's framebuffer.) The contents of the framebuffer gets written to a texture, so we would have to check that if you are writing to more than one texture in a shader, all the textures have the same size. (also, additional constraint, WebGL 1 doesn't let you render to the screen directly if you are also writing to an offscreen texture.)
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 think having a lightweight wrapper object around textures that stores the type info would be good, that's all we'd need to do dynamic checking.
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.
Ah, makes sense. Thanks for the explanation.
// Use `dynamic` to tell the type system to just assume the value will be an int | ||
// at runtime so that the template can be filled now | ||
width: cgl.dynamicInt(canvas.width), | ||
height: cgl.dynamicInt(canvas.height), |
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.
additional note: technically you can swap out the buffers dynamically to ones with different sizes (as long as the whole set of new buffers all have the same size as each other). The way I've set this up doesn't allow that. Do you think that's something we should support?
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.
Let's avoid it for now until we come across an interesting use case for it. Best to start simple and work on extensions from there.
shaderPipeline.resolution = [stage.width, stage.height]; | ||
shaderPipeline.draw(4); // is there a better way to get # vertices? | ||
|
||
// Calder knows the size each item should be and the length of the buffer so we shouldn't need to specify number of vertices |
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.
Just a nitpick, but this is a pretty long line. Do you think we could establish a max line length for .js
files. Something like 120 characters per line?
Thoughts?
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.
Agreed on max line length. I'd even prefer something a bit smaller, maybe 100 characters?
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.
How do you guys feel about adding eslint to the project?
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.
The more linters the better! (though no jslint pls)
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'll take another look at this later, but I left some comments. Looks great so far!
examples/sao/sao-calder.js
Outdated
// Buffers have a format and properties that can be initialized in C struct | ||
// format, or just left as sane defaults. (In WebGL 2, the format and internal | ||
// format of a texture can be different, but in WebGL 1 they must be the same) | ||
buffer rgba diffuseBuf { |
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.
Could we model properties for different buffer types (like TEXTURE_2D, TEXTURE_CUBE_MAP) with something like a JSON object? Of course, we'd need to figure out a way to make sure everything is well-typed, but it would be a familiar format, and easy to declaratively assign property values.
I actually like the idea of providing some sort of configuration schema object, because then it can also be documentation in code for what different properties are (vs reading it in a spec).
Also, I don't think this belongs in a shader? (or is this some WebGL 2.0-like feature)
geometryPass.drawElements(ground.indices); | ||
|
||
// Since we're going to use multiple pipelines, we have to unset buffers. | ||
geometryPass.cleanup(); |
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 think useProgram
should come with a cleanup callback parameter. I feel like buffer management is something that should be thought about, so users should have to think about it every time they run a shader program. Though this may be cumbersome in common cases, so we can probably provide some pre-defined callbacks like clearAll
and some no-op callback.
// Use `dynamic` to tell the type system to just assume the value will be an int | ||
// at runtime so that the template can be filled now | ||
width: cgl.dynamicInt(canvas.width), | ||
height: cgl.dynamicInt(canvas.height), |
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.
Let's avoid it for now until we come across an interesting use case for it. Best to start simple and work on extensions from there.
Demo: https://codepen.io/davepvm/full/zWdepm
This is a WebGL implementation of the SAO algorithm for ambient occlusion.
Considerations: