-
-
Notifications
You must be signed in to change notification settings - Fork 24
Added sketch origin and sketch 3d printing functionality #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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Gip-Gip, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the geometric modeling capabilities by introducing a robust 3D origin and transformation system for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Review by RecurseML
🔍 Review performed on 278e025..acff835
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (7)
• src/mesh/flatten_slice.rs
• src/mesh/mod.rs
• src/mesh/vertex.rs
• src/sketch/extrudes.rs
• src/sketch/hershey.rs
• src/sketch/offset.rs
• src/sketch/shapes.rs
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.
Code Review
This pull request introduces an origin to sketches and meshes, allowing them to be positioned and oriented in 3D space. The changes are extensive, touching CSG operations, extrusions, and conversions to propagate this new origin information. My review found a few issues. There are critical bugs in the new transformation macros that would lead to incorrect geometry when a 180-degree rotation is needed. Additionally, the revolve function has been partially updated but is missing the application of the origin transform to its generated geometry, making it inconsistent with other extrusion functions. I've provided detailed comments and suggestions for these issues.
src/sketch/mod.rs
Outdated
| out_vertex.pos.coords = quat2 * $in_vertex.pos.coords; | ||
| out_vertex.normal = quat2 * $in_vertex.normal; |
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.
There's a bug in the apply_origin_transform! macro when two quaternions are used for rotation. The second rotation quat2 is applied to the original vertex position and normal ($in_vertex), overwriting the result of the first rotation (quat1). It should be applied to the already-rotated out_vertex to compose the rotations correctly.
| out_vertex.pos.coords = quat2 * $in_vertex.pos.coords; | |
| out_vertex.normal = quat2 * $in_vertex.normal; | |
| out_vertex.pos.coords = quat2 * out_vertex.pos.coords; | |
| out_vertex.normal = quat2 * out_vertex.normal; |
src/sketch/mod.rs
Outdated
| out_point.coords = quat1 * $in_point.coords; | ||
|
|
||
| if let Some(quat2) = quat2 { | ||
| out_point.coords = quat2 * $in_point.coords; |
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.
Similar to the apply_origin_transform! macro, there's a bug here. The second rotation quat2 is applied to the original point coordinates ($in_point.coords), overwriting the result of the first rotation. It should be applied to the already-rotated out_point.coords.
| out_point.coords = quat2 * $in_point.coords; | |
| out_point.coords = quat2 * out_point.coords; |
| polygons: new_polygons, | ||
| bounding_box: OnceLock::new(), | ||
| metadata: self.metadata.clone(), | ||
| origin: self.origin, |
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 origin field is now correctly propagated to the Mesh created by revolve. However, the origin_transform is not being applied to the vertices of the revolved geometry. This means the resulting mesh will be located at the world origin, not at the sketch's origin, which is inconsistent with extrude_vector.
To fix this, self.origin_transform should be passed down to the revolve_ring and build_cap_polygon helper functions. Then, the apply_origin_transform! macro should be used on each created Vertex, similar to how it's done in extrude_geometry.
|
Fixed the quat bugs, I'm going to need to look further into the revolve function though it looks like it will have to take more finesse. |
An origin is now stored in a private field in sketches that allows the end user to place and extrude sketches at any coordinate in 3d space. Along with the origin is another private field which stores the transform data that is needed to transform each point when meshing.
High-level PR Summary
This PR adds the ability to position and orient 2D sketches in 3D space before extrusion by introducing an
originfield (stored as aVertex) to bothSketchandMeshstructures. The implementation includes transformation logic using quaternions to rotate sketch geometry according to the origin's normal vector, along with a newbuild_graphic_line_stringsmethod for 3D visualization of sketch edges. The origin and its pre-computed transformation data are propagated through all sketch and mesh operations including CSG operations, extrusions, and conversions.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
src/mesh/vertex.rssrc/sketch/mod.rssrc/sketch/shapes.rssrc/sketch/hershey.rssrc/sketch/extrudes.rssrc/mesh/mod.rssrc/mesh/flatten_slice.rssrc/sketch/offset.rs