Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heuristic shouldn't add overall general space-views if the existing views cover it in aggregate #5167

Open
jleibs opened this issue Feb 9, 2024 · 2 comments
Labels
😤 annoying Something in the UI / SDK is annoying to use 🪳 bug Something isn't working
Milestone

Comments

@jleibs
Copy link
Member

jleibs commented Feb 9, 2024

However, there are still transitive states during startup when the heuristic will propose a "bad" space-view with / as the root.

Consider the logging sequence:

img1/detections
img2/detections
img1
img2

Once the two images have been logged, the image-split heuristic kicks in and we end up with two views:

img1/**
img2/**

However, even if we have this blueprint initialized, we hit the following issue:

First, img1/detections on its own proposes img1/detections/** which is redundant with img/** and correctly ignored.

Next, the introduction of img2/detections causes the proposal of /** as a common ancestor of img1/detections and img2/detections

Unfortunately, the logic from #5164 doesn't work here since /** isn't covered by either of the existing space-views. This causes /** to get added back to the scene.

Possible solutions

One option is to do a redundancy analysis that actually considers the concrete entities that would be added. In this case both entities are covered by existing views, even though /** would match many things that aren't. We could do something like add a adds_entities: Vec<EntityPath> to the RecommendSpaceViews result and filter that against all the existing space-views. This is probably the easiest thing to do if the performance cost isn't too bad.

A different approach might be to find a place in the blueprint to store some kind of state in the blueprint that explicitly tracks /** as a split-path for 2D views. Basically the heuristic could check something in the blueprint and conclude the space should continue to be split for this blueprint even though the images don't actually exist yet in this particular recording. This would have the added benefit that img3/detections would continue to do the right thing, even if img3 hadn't been logged yet.

@jleibs jleibs added 🪳 bug Something isn't working 😤 annoying Something in the UI / SDK is annoying to use labels Feb 9, 2024
@Wumpf Wumpf added this to the Triage milestone Feb 12, 2024
@jleibs
Copy link
Member Author

jleibs commented Aug 7, 2024

Note, there is a particularly annoying interaction here with #5165

The analysis for splits leverages store subscribers, but since these leak from recording to recording, opening, then closing, then opening the same recording can produce different heuristic results.

@abey79
Copy link
Member

abey79 commented Aug 7, 2024

I ran into this today, staring with no app.ron nor any saved blueprint:

issue_5167.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😤 annoying Something in the UI / SDK is annoying to use 🪳 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants