Skip to content
This repository was archived by the owner on Apr 9, 2024. It is now read-only.

Filled circle and Catenary primitives #64

Merged
merged 6 commits into from
Nov 12, 2022

Conversation

HenryWConklin
Copy link
Contributor

For #60

@HenryWConklin HenryWConklin changed the title Add more geometry primitives Filled circle and Catenary primitives Nov 10, 2022
@HenryWConklin
Copy link
Contributor Author

Screenshot from 2022-11-10 07-30-54
Screenshot from 2022-11-10 07-35-02

@HenryWConklin HenryWConklin marked this pull request as ready for review November 10, 2022 15:36
Copy link
Owner

@setzer22 setzer22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! 😄 See my comment below for a potential improvement

Vec3::new(xz.x, y, xz.y)
}
};
Line::build(position, segments)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I'm thinking... 🤔 Would it be easy to analytically compute normal and tangent vectors for each point? Because if so, we could add one extra parameter to add those channels and nodes like Extrude along curve would work out of the box with catenary curves, which would be pretty cool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's pretty easy to compute. Would that go in Line::build? If there's an example somewhere of filling in those channels I can add that tonight/tomorrow.

Copy link
Owner

@setzer22 setzer22 Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an example in resample_curve (edit_ops.rs:1846), but that one's probably a bit convoluted. The whole idea with mesh channels is to allow attaching arbitrary data to vertices, halfedges and faces. You would need something like:

// Create the channel, and fetch by id
let tangent_ch_id = mesh.channels.ensure_channel::<VertexId, Vec3>("tangent");
// Channels have interior mutability, hence the `read_*` and `write_*` variants to access them.
let mut tangent_ch = mesh.channels.write_channel(tangent_ch_id).unwrap();

// Then, you can use it like a map:
let v: VertexId = todo!("obtain it somehow");
tangent_ch[v] = Vec3::new(1.0, 2.0, 3.0);
dbg!(tangent_ch[v]);

Channels are like a sort of "loose" map in that you can always fetch from it even if you've never inserted a key before (and you get Default::default() back) and you can always set a key as long as it's a valid vertex for the mesh. They're based on slotmap::SecondaryMap.

As you say, a good place for this code would be the Line::build function, you can create these channels right below the call to write_positions (FYI, vertex positions are just one of the channels that are allocated by default in a mesh, there's nothinc special about them other than the read_positions / write_positions convenience methods). The function could take two (perhaps optional?) functions for tangent and normal values. Feel free to refactor things a bit to make it fit, as I have the impression doing that might make the code a bit awkward 🤔

I'm not sure what we should do with other line constructors that don't have a well-defined notion of normals (like the regular Line node, or Line from points). In those cases, we can either compute some normals by picking an arbitrary direction like one of the 3 main axis, or else we could make the tangent / normal channels optional and not compute them for those constructors. I'm not sure how useful this sort of default would be.

Fixes "extrude along curve" and other operations for line/curve
primitives.

Also, add computed normals and tangents to catenary curve
@HenryWConklin
Copy link
Contributor Author

Screenshot from 2022-11-11 08-47-23

@HenryWConklin HenryWConklin reopened this Nov 11, 2022
@HenryWConklin
Copy link
Contributor Author

HenryWConklin commented Nov 11, 2022

Had some git issues, but I think this PR is good to go now.

Also, I'll comment that there's a mismatch in the defaults for circle and extrude along curve. The circle needs to be rotated first and the faces flipped on the extrude node to get what you expect. May be a larger issue so I didn't want to mess with it too much.

Copy link
Owner

@setzer22 setzer22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 Ready to merge!

@setzer22
Copy link
Owner

Hmm 🤔 I just tested this locally and while it works well, there's an annoying glitch when extruding:
image

The normals look right (not shifted in the Z direction), so this could be a bug in "Extrude Along Curve". Need to investigate further, but unless you have an idea of what's going on there, I think we can merge this PR and convert this into an issue so we can tackle it later. The catenary node is super useful and I don't want to drag this more than necessary 😄

Also, I'll comment that there's a mismatch in the defaults for circle and extrude along curve. The circle needs to be rotated first and the faces flipped on the extrude node to get what you expect. May be a larger issue so I didn't want to mess with it too much.

Yes, I'm aware of this. I wonder what the most user-friendly solution would be. The problem seems to be the "Extrude Along Curve" node assuming the cross section is looking in the X direction, while most primitives like Circle or Quad are constructed looking up. Maybe I should just add an extra rotation matrix there to correct for this 🤔.

@setzer22
Copy link
Owner

setzer22 commented Nov 12, 2022

After further exploration, I think there's something fishy with those normals 🤔. See how there's this unwanted rotation at the start that moves forward as I extend the curve?

2022-11-12.13-10-55.mp4

This is the code that extrude along curve uses to rotate the cross section at edit_ops:1572 (that dbg! was added by me during debugging):

            let normal = normal_ch[v];
            let tangent = tangent_ch[v];
            dbg!(normal.dot(tangent));
            let cotangent = normal.cross(tangent);
            let (_, rotate, _) = glam::Affine3A::from_cols(
                cotangent.into(),
                normal.into(),
                tangent.into(),
                glam::Vec3A::ZERO,
            )

When using a regular Line, the dot is zero as expected (normal and tangent should be orthogonal). But with the catenary, I'm getting different non-zero values depending on the endpoints. Looks like the computation for the normals might be a bit off?

Now that we have gizmos, I'm going to work on some API that lets us do some debug drawing to better see what's going on, I've been meaning to do that for a while.

Also, add a fallback for when we can't find a solution for the x offset in the catenary.
@HenryWConklin
Copy link
Contributor Author

HenryWConklin commented Nov 12, 2022

Yeah, that looks like the tangent is wrong, I was getting that earlier but thought it was better.

Fixed with some simpler code that I should have tried first. Also added a fix for when the endpoints line up vertically and the math stops working.

catenary-tangents.mp4

Screenshot from 2022-11-12 05-12-55

Let me know if you spot anything else weird.

@setzer22
Copy link
Owner

Amazing! Thanks for the hard work 😄

@setzer22 setzer22 merged commit fa0160b into setzer22:main Nov 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants