zesterer / euc

A software rendering crate that lets you write shaders with Rust
Apache License 2.0
294 stars 15 forks source link

Replaced `bresenham` with `clipline` (untested) #28

Closed nxsaken closed 11 months ago

nxsaken commented 11 months ago

Replaced the bresenham crate with clipline, which implements Bresenham's algorithm with built-in clipping. Unfortunately, I couldn't find a quick way to test it.

zesterer commented 11 months ago

Thanks for the PR! I've been meaning to give some love to the line rasteriser, it's by far the biggest thing blocking the merging of refactor.

A good way to test this is to apply the following diff to examples/wireframe.rs and then run cargo run --example wireframe.

diff --git a/examples/wireframes.rs b/examples/wireframes.rs
index 09ae1ad..d3f6da3 100644
--- a/examples/wireframes.rs
+++ b/examples/wireframes.rs
@@ -1,7 +1,7 @@
 use derive_more::{Add, Mul};
 use euc::{
     AaMode, Buffer2d, Clamped, DepthMode, Empty, Linear, Pipeline, PixelMode, Sampler, Target,
-    Texture, TriangleList, Unit,
+    Texture, TriangleList, Unit, LineTriangleList,
 };
 use minifb::{Key, MouseButton, MouseMode, Window, WindowOptions};
 use std::marker::PhantomData;
@@ -24,7 +24,7 @@ struct VertexData {
 impl<'a> Pipeline for Teapot<'a> {
     type Vertex = wavefront::Vertex<'a>;
     type VertexData = VertexData;
-    type Primitives = TriangleList; //Lines;
+    type Primitives = LineTriangleList;
     type Fragment = Rgba<f32>;
     type Pixel = u32;

I'm not sure the current change is working properly. Here's how the example looks on the refactor branch with the diff above applied.

Screenshot from 2023-10-26 09-35-47

And here's your branch with the above diff applied.

Screenshot from 2023-10-26 09-36-53

I've not had much time to look into exactly what's going on, but hopefully this is useful!

nxsaken commented 11 months ago

Thank you, I'll try to debug it!

nxsaken commented 11 months ago

@zesterer the example seems to work now, but there's a caveat: clipline draws endpoint-inclusive lines compared to bresenham which excludes the second endpoint. I haven't found an obvious fix to that yet, and I'm unsure how problematic it is for this use case (potential overdrawing? blending issues?)

zesterer commented 11 months ago

I think endpoint-inclusive is probably fine. Overdrawing issues were discussed in #17 but there's not yet been a concerted effort to come up with consistent behaviour so I think this is fine for now.

zesterer commented 11 months ago

Thanks for updating the PR, much appreciated! It's looking great now. Just two more things:

1) Any chance you could change the wireframe example to use LineTriangleList as the primitive type instead of TriangleList?

2) If you zoom into the teapot with the right mouse button, you'll find that things behind the camera seem to result in odd lines off to the side, presumably because the lines aren't being properly clipped. Any chance you'd know what's causing this?

nxsaken commented 11 months ago

I updated the example. Tried to investigate the clipping issue, but I lack 3D graphics knowledge to be able to tell whether the problem happens during the vertex operations or rasterization. Seems like the old Bresenham variant produces the same issue. I also noticed a segmentation fault, but I'm not sure what conditions trigger it (could it be related to the clipping issue because I only started getting it when I used the zoom?) I couldn't replicate the segmentation fault with the old Bresenham variant because it's too slow at that zoom level.

zesterer commented 11 months ago

Thanks! I think I'll merge this for now and try to investigate this fix in my own time. Regardless, I'm really thankful for this PR: I think this pretty much unblocks merging refactor and creating a new release now.

nxsaken commented 11 months ago

I'm so happy this was helpful! Finding this project is what prompted me to make the crate :)

zesterer commented 11 months ago

Oh wow, well thanks even more for doing that! Very much appreciated, I hope you find euc to be useful :)

zesterer commented 11 months ago

I finally got round to fixing this: it turns out that the w component of the projection matrix was producing absurd values for lines behind the camera, resulting in out of bounds lines with nonsensical coordinates after casting to isize for screen coordinates. All appears to work now! Going to start preparing for a release.

nxsaken commented 11 months ago

Awesome! Just a heads up: clipline draws lines a bit differently from the other crates (see https://github.com/nxsaken/clipline/issues/6). It probably won't be terribly noticeable, but I should definitely fix that. Also, I've released a new version with performance improvements, there are no breaking changes. There are also some benchmarks if you're interested.

zesterer commented 11 months ago

That's fine! I'm not too bothered by exactly which pixels get drawn at this stage: perhaps in the future pixel-perfect recreation of certain conditions might be a goal, but today it's not.

I'll update the version of clipline too.