Skip to content
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

Sweep operation only works for sketches that are symmetric to the x-axis #230

Closed
hannobraun opened this issue Feb 21, 2022 · 0 comments · Fixed by #284
Closed

Sweep operation only works for sketches that are symmetric to the x-axis #230

hannobraun opened this issue Feb 21, 2022 · 0 comments · Fixed by #284
Labels
topic: core Issues relating to core geometry, operations, algorithms type: bug Something isn't working

Comments

@hannobraun
Copy link
Owner

A sketch has a defined top and bottom side. The top side points upwards, along the z-axis.

When sweeping a sketch into a 3D shape, the original sketch is re-used for the top and bottom face of the new shape:

  • For the top face, the original sketch needs to be cloned and transformed.
  • For the bottom face, the sketch can stay where it is, but it needs to be turned around, so its top side is on the outside of the new shape, and its bottom side on the inside.

That second part is the problem. Did you notice how I said "turned around"? That's actually wrong. It actually needs to be mirrored (to be precise, only the sketch surface needs to be mirrored; all the edges and vertices should stay the same anyways). Turning around (the x-axis) is what the code currently does though, hence this bug.

I've modified the star model to no longer being symmetric, and it shows this bug beautifully:

Screenshot from 2022-02-21 16-36-51

It might be showing other bugs too, hard to say. We'll know more, once this one is fixed.

To get this result, I've just replaced the let radius = ... line of the star model with this:

let mut radius = if i % 2 == 0 { r1 } else { r2 };
radius *= (i + 1) as f64 / num_vertices as f64;

On way to fix this bug would be to implement #101, then apply a mirror transform to the sketch surface. Another way would be to mirror using purpose-built code, not a general transform.

This might also be an opportunity to revisit the concept of surface orientation. At this point, a normal is computed for each triangle (on the CPU), depending on how it is winded, and each pixel is then shaded according to the angle between view direction and normal (on the GPU). How pixels are shaded is currently the only result of surface orientation, but I'm pretty sure that it will become more important in the future, for example to define what is inside and what is outside a body.

So instead of making the pixels look good again, it might be worth it to think how surface orientation should be represented, and implement that. That would, as a side effect, provide a much more solid way to fix this issue.

@hannobraun hannobraun added type: bug Something isn't working topic: core Issues relating to core geometry, operations, algorithms labels Feb 21, 2022
hannobraun added a commit that referenced this issue Mar 3, 2022
This fixes #230, while making #173 worse (now all bottom faces of
extruded shapes are black). I think this is an acceptable trade-off, as
it enables some progress with the sweep code, without requiring more
infrastructure work right away.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Issues relating to core geometry, operations, algorithms type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant