Conversation
DennisJensen95
left a comment
DennisJensen95
left a comment
There was a problem hiding this comment.
This is actually not that bad, i guess claude code can do this pretty good because of the specification work. Some missing code pairs, but maybe the partial implementation with some more and proper testing would suffice and provide value? @brettfo what you think? 😊
Even if the hatch feature is partial, this PR changes add a lot of value by enabling to export polygons with holes. Would be great if it could be merged. 😀 |
|
Ye, I think however this is only partially working and might break in some situations. So it depends if you want something partially working/buggy. But i like the work 🙌 |
What is partially working? |
|
The C#/.NET version of this library has a pretty solid
The other library also has a large selection of tests that should probably be taken, too. |
|
Can this PR be merge with current implemented features and then another PR for missing hatch features? Or you prefer to have full features in this PR? I would please prefer to scope only what is already there so I'm unlocked and can use this awesome library! |
|
I think some more testing is a must. And testing more inline with other tests 😊 But I am only maintaining downstream fork. |
|
Equivalent tests of the C# implementation as given by @brettfo are now in this PR. |
Nice 👍 |
|
When can I expect this PR to be merged & pushed into a new version? |
|
I would suggest using your own fork to start. Relieves the pressure of getting it in 😊 |
|
@brettfo can i review this for approval or is it only you? 😊 |
spec/EntitiesSpec.xml
Outdated
| <Field Name="end_parameter" Code="42" Type="f64" DefaultValue="::std::f64::consts::PI * 2.0" | ||
| Comment="Ellipse end angle in radians." /> | ||
| </Entity> | ||
| <!-- HATCH --> |
There was a problem hiding this comment.
It would be nice to have a difference with what codes are left to be implemented?
There was a problem hiding this comment.
I'm not sure all of which features are not implemented, but here's a few:
- Elliptical arc and spline edges not supported in Hatch
- Gradient
samuelint
force-pushed
the
feat/support-hatch
branch
from
6b76db9 to
a7b2faf
Compare
Yes, please, I'd love some extra eyes on this and I'm glad to see you've been doing exactly that. I should be able to do a final look at this PR this weekend, but in the meantime any and all feedback is appreciated because it'll make my final approval easier, especially if forks can use this and provide notes about what works and what doesn't. |
spec/EntitiesSpec.xml
Outdated
|
|
||
| <!-- | ||
|
|
||
| HATCH |
There was a problem hiding this comment.
Can you move this section up to line 408? I'd like to keep this file sorted and there's a placeholder there for it.
There was a problem hiding this comment.
Done, that make a lot of sense
|
I've made adjustments from your comments. Now what's missing for this PR to be merged? |
|
|
||
| // Write basic hatch properties | ||
| if hatch.elevation != 0.0 { | ||
| pairs.push(CodePair::new_f64(30, hatch.elevation)); |
There was a problem hiding this comment.
The spec states that all three 10, 20, and 30 codes are required for this value, even though the 10 and 20 codes will always be 0.
| if let crate::BoundaryPathEdge::Polyline { vertex } = edge { | ||
| pairs.push(CodePair::new_f64(10, vertex.x)); | ||
| pairs.push(CodePair::new_f64(20, vertex.y)); | ||
| pairs.push(CodePair::new_f64(42, vertex.z)); |
There was a problem hiding this comment.
Code 42 is a bulge value, not a Z-value of the vertex. Check the section "Polyline boundary data group codes" on this page.
| --> | ||
| <!-- TODO MinVersion="R14" --> | ||
| <Entity Name="Hatch" SubclassMarker="AcDbHatch" TypeString="HATCH" MinVersion="R14" GenerateWriterFunction="false"> | ||
| <Field Name="elevation" Code="30" Type="f64" DefaultValue="0.0" DisableWritingDefault="true" /> |
There was a problem hiding this comment.
To account for the 10 and 20 codes, this will need to be
<Field Name="elevation" Code="10" Type="Point" DefaultValue="Point::origin()" CodeOverrides="10,20,30" />(although with a custom reader/writer function, CodeOverrides probably isn't necessary, but it's nice to have to see what might appear in the file)
This means the reader function in entity.rs won't be able to assume that all 10 and 20 codes are seed points; 10/20 should be used as elevation values until we've read code 98 indicating we're reading the seed point values.
3 participants
From issue: #25 (comment)