#8162 Predictive ids #8332
#8162 Predictive ids #8332
Conversation
… needed to work with the new engine's new id generation
… graph generation for walls yet
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| ast, | ||
| item_count, | ||
| exec_artifacts, | ||
| &id_generator, |
There was a problem hiding this comment.
This is passed to artifacts_to_update so the original Segments can be created with the new ids.
| // If we are creating a new body we need to preserve its new id. | ||
| if extrude_method != ExtrudeMethod::New { | ||
| sketch.id = face.solid.sketch.id; | ||
| if generate_predictive_ids { |
There was a problem hiding this comment.
This is just a temporary flag to enable the new id generation (turned on for extrude only), or keep using the previous code.
It can be deleted if we change this for all sweep operations.
| return_arr.push(Artifact::Path(inner_path_artifact)) | ||
| } | ||
|
|
||
| if let ModelingCmd::Extrude(kcmc::Extrude { .. }) = cmd { |
There was a problem hiding this comment.
This is where the artifact graph is populated with the extrusion related artifacts.
CodSpeed Instrumentation Performance ReportMerging #8332 will degrade performances by 18.58%Comparing Summary
Benchmarks breakdown
|
| let face_id = id_generator.get_face_id(); | ||
| let next_face_id = id_generator.get_next_face_id(path.seg_ids.len() as u32); | ||
| let curve_id = id_generator.get_curve_id(); | ||
| let opposite_edge_id = id_generator.get_opposite_edge_id(); | ||
|
|
||
| let adjacent_edge_id = id_generator.get_adjacent_edge_id(); |
There was a problem hiding this comment.
Is the order these are called important, does it have to generate them in the same order as the engine?
There was a problem hiding this comment.
I answered my own question. They are all derived on the current segment index, so the order of these don't mater.
And later in the loop id_generator.next_edge(); is how it progresses the segment index . . . I think.
There was a problem hiding this comment.
Yes, exactly! It would be cleaner to use something like an Iterator in this particular code location but there are others where it was much less disruptive to the existing code to have a manual way to step the index:
https://github.com/KittyCAD/modeling-app/pull/8332/files#diff-5bd2a5278e05b85f158d1fbb3f690cbd1fddf47dc21d035baa23cdcf1bde16daR632
2 participants
Pair of https://github.com/KittyCAD/engine/pull/3625/files
Definitely not ready for merge, but ready for review. When ready it needs to be deployed carefully in sync with the above PR.
This PR generates ids in the artifact graph for extrusion without calling
Solid3dGetExtrusionFaceInfoandSolid3dGetAdjacencyInfo.I discussed with @Irev-Dev and our preference is to have this enabled for all sweep-like operations and with that remove the above commands completely. This PR only deals withs the actual extrude operation for now but it's an easy change to enable it for the other sweeps.
There is a known bug which is yet to be discovered whether it's in this PR or the engine PR: certain extruded faces (at most 2 walls) are not selectable because the frontend generated ids are not matching the engine generated ones.