search

KittyCAD / modeling-app

The KittyCAD modeling app.
https://kittycad.io/modeling-app/download
MIT License
429 stars 37 forks source link

Default planes should be distinguishable at runtime #4504

Open jtran opened 1 week ago

jtran commented 1 week ago

Right now, we're going based off of the source range, which seems really fragile. See https://github.com/KittyCAD/modeling-app/pull/4481#discussion_r1841024676.

franknoirot commented 1 week ago

This issue brings to light that we have been needlessly tracking the default planes as state within engineConnectionManager and indirectly within kclManager, where we have methods for showPlanes and hidePlanes. Here is my understanding after talking to the team, and my game plan for a refactor:

State of affairs

  1. Way back we were creating the default planes within TS, and passing them into the engine as pre-existing items during execution. This is why there is this state managed by engineConnectionManager, and why there is special logic for the default planes outside of the core WASM-side KCL execution in run_with_session_data
  2. While the IDs of the default planes have been managed by engineConnectionManager, their visibility is not tracked anywhere, and has been mutated as a side effect of state transitions within modelingMachine; namely when the AST is emptied or populated, and when sketch mode is awaiting a surface selection ( the "sketch no face" state).

Refactor plan

  1. Make the default planes created as a "prelude" of run_with_session_data, and include those commands in the same batch (remove the flush currently in the default_planes construction).
  2. Make the artifactGraph honor default planes if they are in the modeling batch (which they always will be due to 1)
  3. Remove any reference to default planes from engineConnectionManager or kclManager
  4. Remove special handling of default_planes from wasm-lib
  5. Create a derived visibilityMap from artifactGraph and tracks relevant artifacts' visibility (planes only for now), removing if they don't exist in the artifactGraph
  6. Move hidePlanes and showPlanes into engineConnectionManager, and make it act on the visibilityMap
  7. Fix the fragile line in @jtran's comment to refer to the artifactGraph to determine if a plane is default or not
Irev-Dev commented 3 days ago

Btw, I made these rust changes in my multi-profile PR.

I need the above for how I'm currently grouping profiles, but can work with an alternative.