Skip to content
This repository has been archived by the owner on Jul 16, 2024. It is now read-only.

Return chainable iterator after QueryResult:without() #90

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

Ukendio
Copy link
Contributor

@Ukendio Ukendio commented Dec 22, 2023

This PR resolves #85.

At initial commit, it passes every test. No regressions and it also works well for the intended use case of chaining with snapshot or view.

However, the code is just glued together and will require quite a bit of polishing before I am confident in shipping this out. The code changes are the minimum steps I took to ensure it passed the tests. And I would like to make sure this code is good enough before I am confident in having it reviewed.

List of things to address first

TODO

  • Should rework the QueryResultstruct. It is currently a pretty big struct of data comparatively to what it was before due to it now using methods for expand and next instead of closures and consequently had to store every external variable to be reusable.

  • Should return a NOOP query when there's no compatible archetypes.

  • Fix organization and comments. Things are a complete mess here due to the fact I had moved around a bunch of things in order to pan the code.

@Ukendio
Copy link
Contributor Author

Ukendio commented Dec 23, 2023

image

Case "QueryRework" is the changes to World's query reflected in the PR. Case "Without" is a separate change that does archetype negation for making more narrow subsets of queries #91 .

203 components, 3 archetypes. Looped a query that implements queryResult:without() 100 times.

When not using without, they are equivalent in speed, favouring "QueryRework" and "Without" due to small micro optimizations
image

@Ukendio Ukendio marked this pull request as ready for review December 23, 2023 00:43
@Ukendio Ukendio mentioned this pull request Dec 23, 2023
@@ -7,6 +7,8 @@ local assertValidComponent = Component.assertValidComponent
local archetypeOf = archetypeModule.archetypeOf
local areArchetypesCompatible = archetypeModule.areArchetypesCompatible

local QueryResult = require(script.Parent.query)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why the file was named "query" instead of "QueryResult"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because I am conflicted on what to name. I would much prefer calling the file query, but that might mean we have to rename the struct to query as well which I am not sure how I feel about that yet.

What do you suggest?

@LastTalon
Copy link
Contributor

Can we get this linked to an issue that explains what the PR is solving to keep that documented for the future?

@Ukendio
Copy link
Contributor Author

Ukendio commented Dec 30, 2023

Can we get this linked to an issue that explains what the PR is solving to keep that documented for the future?

I have now edited the comment to link the relevant issue

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QueryResult needs rework
3 participants