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

Don't associate a single archetype with two different queries #5029

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Feb 4, 2024

The newly introduced point and line visualizers for the new Scalar archetype associate two different queries with a single archetype (the first one is pov:[Scalar] comps:[Color, MarkerShape, Text], the second one is pov:[Scalar] comps:[Color, Text]).

This is a big issue since the cache currently considers each archetype to be a single unique query.
If you first query using the query that lacks MarkerShape, and then queries with the other one, then the MarkerShape will be missing since the data has been cached without it (and the cache has no reason to believe that it is missing anything nor does it have to invalidate stuff).

That's the root cause for the wave of bugs that came up at the end of last week.
#5016 and #5007 fixed the symptoms of that (which is still a good thing), but I never had the time to look into why that was happening in the first place, until now.

Now, ultimately, the right thing to do is to remove the notion of archetype from the cache entirely: the cache doesn't care about archetypes, it cares about point-of-views and queried components.
This is already what's planned: the only reason the cache is aware of archetypes right now is that it builds on the pre-existing query system which requires archetype types to be passed around.

In this specific case, though, it still seems wasteful to treat these two queries as different queries: it is much more efficient to always query for MarkerShape and not use it than it is to maintain two different caches for these two different visualizers.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@teh-cmc teh-cmc added 🪳 bug Something isn't working 💬 discussion exclude from changelog PRs with this won't show up in CHANGELOG.md 🔩 data model labels Feb 4, 2024
@teh-cmc teh-cmc added this to the 0.13 milestone Feb 4, 2024
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense and I'm ok with the workaround for the time being. but I suppose we have to update this again with #5025 which adds a query for a new component on LineVisualizer - if I understand correctly we have to add the StrokeWidth now on PointVisualizer.

I'm first landing the new stroke width and then do the change here so we have this change in a single place and can discuss it here further if needed.

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what this needs to look like then.
super brittle but I'm ok to ship this, should probably create a separate ticket and link to it.

@teh-cmc
Copy link
Member Author

teh-cmc commented Feb 5, 2024

Thanks for the updates!

super brittle but I'm ok to ship this, should probably create a separate ticket and link to it.

Yeah I only left it as brittle as is while we decided which direction to go.

If we're going with this, then let's make sure they share the same query code to begin with so we don't get any bad surprises.
We need some layer of abstraction anyway to make sure all present and future scalar visualizers benefit from the performance changes that we've started investigating in #5024.

I'll do a follow-up asap.

@teh-cmc teh-cmc merged commit c3da540 into main Feb 5, 2024
40 checks passed
@teh-cmc teh-cmc deleted the cmc/same_arch_diff_queries branch February 5, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 🔩 data model 💬 discussion exclude from changelog PRs with this won't show up in CHANGELOG.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants