-
Notifications
You must be signed in to change notification settings - Fork 109
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
Caustic rendering demo #274
base: main
Are you sure you want to change the base?
Conversation
Based on https://benedikt-bitterli.me/tantalum/, this demo generates caustics by tracing the path of light through a 2D scene. All updates are done in the `Accum` writer effect, which should mean that this example can be sped up using parallelism.
@@ -48,6 +48,7 @@ code { | |||
|
|||
.plot-img { | |||
width: 80%; | |||
image-rendering: pixelated; |
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'm not sure it makes sense to turn this on globally. This makes images crisp when they get scaled up, which was useful for figuring out antialiasing and such (since the images are always upscaled to 80% width).
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 generally looks good. Some asks form me would be:
- There's a lot of inline math with no explanation nor reference. It would be good to factor those out or at least add some text. Otherwise I think it will be difficult for people to follow this example, and that's their purpose after all.
- There are a lot of int<->index casts. I wonder if we could eliminate some of those? Otherwise it doesn't seem to be a great selling point for Dex?
examples/caustics.dx
Outdated
def rasterLineSegmentInFourPixels (height:Int)?-> (width:Int)?-> (ref:Type)?-> | ||
(_: VSpace out)?=> | ||
(pixel_x: Int) -- 0 <= pixel_x < width - 1 | ||
(pixel_y: Int) -- 0 <= pixel_y < height - 1 |
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.
Those invariants are exactly the Fin width
and Fin height
invariants, so maybe take those as arguments? The unsafe call sites can still do the check, but the safe ones can skip them (and I think you have those below!)
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.
Oh, interesting. They are actually Fin (width-1)
. Would it make sense to represent the canvas size as Unit|Fin width_minus_1
in this case?
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. Maybe? But then we would have to change all the other functions (drawIntoCanvas
, rasterLineSegment
, etc) to accept non-Fin-based tables. I'm not sure if that's a net improvement or not. What do you think?
(also, technically, this function works fine if you give it things out of bounds, it just does nothing in that case. So it's not strictly speaking an invariant.)
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.
Yeah... I'm just a little concerned with the number of index<->int casts in this code. I would consider that an usual case for Dex code, and it will likely cause us to deoptimize a bunch of stuff. I'd rather remove as many of them as we can.
det = m.(0@_).(0@_) * m.(1@_).(1@_) - m.(0@_).(1@_) * m.(1@_).(0@_) | ||
(1.0/det) .* [ [m.(1@_).(1@_), -m.(0@_).(1@_)] | ||
, [-m.(1@_).(0@_), m.(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.
Maybe we should just support table unpacking syntax? You could do
[[a, b], [c, d]] = m
and then do math on the individual elements
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.
That's a great idea! Yeah, that would clean up a lot of stuff.
I agree that this isn't as well documented as it could be; I was mostly focused on trying to get something that worked. I'll take another pass through it and try to make it more readable. Regarding the int <-> index casts, it seems like there's a few different reasons I'm doing that in this code:
I suppose there are other ways of accomplishing 3 (like a while loop) but I don't have a mental model of what the performance characteristics are. For 2, I'm not sure there's much that I can do to remove those. Is dynamically indexing a table based on math something that Dex isn't expected to do well? If so, maybe this isn't a good choice of example. |
] | ||
|
||
-- TODO: deduplicate with https://github.com/google-research/dex-lang/pull/276 | ||
def atan2 (y:Float) (x:Float) :Float = |
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.
Whoops, I forgot you already added an atan2
function. The one I used is more expensive, but follows the TF and XLA implementations. I think its derivation is pretty close to the one you used, but with a higher-degree polynomial. I don't know if the XLA people had a good reason for their choice - perhaps it should depend on the floating-point precision being used.
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.
"Do what XLA does" is probably a better justification for an implementation than "use the first thing you see on stack overflow" anyways!
Based on https://benedikt-bitterli.me/tantalum/, this demo generates caustics by tracing the path of light through a 2D scene. All updates are done in the
Accum
writer effect, which should mean that this example can be sped up using parallelism.Uses a custom antialiasing function that computes line integrals within bilinearly-smoothed pixels, so the rendered outputs correspond fairly accurately to the amount of light passing through each pixel.
Example output:
Some implementation notes:
atan2
but that's not a builtin yet, so I found a cheap approximation. But this should probably be a builtin? I imagine it's just a matter of hooking it up to LLVM in the right way.m.(1@_).(1@_)
). I ended up mostly using tuples instead. Maybe this is another argument toward having a more powerful indexing syntax.