search

KittyCAD / modeling-app

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

Sending selections to engine loops over all artifacts #868

Open Irev-Dev opened 1 year ago

Irev-Dev commented 1 year ago

search for TODO #868 in src/lib/selections.ts

We loop over all artifacts to find source ranges that over lap with the current selection, is fine for now, but will become a problem with very complex scenes.

This approach is used is because selections are nothing more than source ranges, so cursor positions is the source of truth for what is selected, such that when a user changes their cursor (or we update the user cursor from a click of something in the engine) we then tell the engine what to show as selected.

It might be worth abstracting the artifact map a little to maintain two mappings, the current id, to artifact, and another sourceRange to artifactId, though I'm not sure how to do this since the sourceRange is a start and end position. Perhaps maintaining an ordered array of source ranges would work since it's impossible to have two highlighted sections of code that overlap? that way we can binary tree search the list?

Irev-Dev commented 3 months ago

artifact map does need a overhaul.

getting a better data-structure that allows artifacts to be looked up by either ID or sourcerange efficiently (i.e. ordered-by-source-range/binary-tree idea above). But the artifact map is also doing double duty as it handles all of the async stuff of pending artifacts too. The artifact map should probably only have the final artifacts, and have something else handle pending messages.

For context the artifact map is one of the oldest parts of the code based and it has been pretty flexible through a lot of changes, but it's doing more than it should image

Irev-Dev commented 2 weeks ago

@jtran is working on making artifact Id stable between executions, with that and other things in the pipeline like https://github.com/KittyCAD/modeling-app/pull/3836 I think this will get resolved by those efforts.

Linking to the above PR to remember to close if that does resolve this issue