KCL refactor: Solid should not have a Sketch field #9641
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
adamchalmers
force-pushed
the
achalmers/sketch-base-codex
branch
from
f6c2241 to
f7a59b7
Compare
| (KclValue::Solid { value }, Property::String(prop), false) if prop == "sketch" => Ok(KclValue::Sketch { | ||
| value: Box::new(value.sketch), | ||
| (KclValue::Solid { value }, Property::String(prop), false) if prop == "sketch" => { | ||
| let source_range = SourceRange::from(self.clone()); |
There was a problem hiding this comment.
You should be able to avoid cloning the entire AST node with something like this.
| let source_range = SourceRange::from(self.clone()); | |
| let source_range = SourceRange::from(self); |
| .collect(), | ||
| constrainable: false, | ||
| (KclValue::Sketch { value: sk }, Property::String(prop), false) if prop == "tags" => { | ||
| Ok(tags_object_from_map(&sk.tags, SourceRange::from(self.clone())).continue_()) |
|
|
||
| pub(crate) fn merge_tags<'a>(&mut self, tags: impl Iterator<Item = &'a TagIdentifier>) { | ||
| for t in tags { | ||
| match self.tags.get_mut(&t.value) { |
There was a problem hiding this comment.
Could probably use the Entry API here.
Most solids are based on a sketch, but there's really no need to have the full sketch as a field of the solid. It's really only used for two things:
acan become the faceawhen extrudedAs part of Serena's in-progress work for extruding faces (#8661), this won't be the case -- the solid extruded from a face won't be based on a sketch.
So I am refactoring it to use a new type SketchBase instead. It's like Sketch but it only has a subset of its fields. In the future, we can make SketchBase an enum, so that solids created through other means have a different set of fields.