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

Allow components to store any datatype #138

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

Conversation

memorycode
Copy link
Contributor

@memorycode memorycode commented Nov 16, 2024

This PR adds support for non-table components. Calling Components.Model(model) will return a table that has a special marker on it that tells the World it should be unwrapped on insertion. I explicitly chose this strategy because it means that we can do not need to store this table in archetypes.

Unfortunately, we still need to store Components.Model(model) initially inside of a table because we need a way to get the component ID and Luau does not have tuples.

@memorycode memorycode marked this pull request as draft November 16, 2024 16:36
lib/Component.luau Outdated Show resolved Hide resolved
lib/Component.luau Outdated Show resolved Hide resolved
@memorycode memorycode changed the title Add support for non-table components Allow components to store any datatype Nov 16, 2024
@memorycode memorycode marked this pull request as ready for review November 16, 2024 23:03
@memorycode memorycode marked this pull request as draft November 16, 2024 23:05
@memorycode memorycode marked this pull request as ready for review November 17, 2024 16:59
@memorycode memorycode requested a review from a team November 17, 2024 17:01
Copy link
Member

@LastTalon LastTalon left a comment

Choose a reason for hiding this comment

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

I haven't had time to read the full changes yet. I have gotten to some of the central changes though.

lib/Component.luau Outdated Show resolved Hide resolved
lib/Component.luau Outdated Show resolved Hide resolved
@memorycode memorycode requested a review from LastTalon November 18, 2024 22:31
@LastTalon
Copy link
Member

Could you also update the description of the PR with a brief description of how this PR achieves what it says in the title in case anyone looks back at it later?

@memorycode
Copy link
Contributor Author

Could you also update the description of the PR with a brief description of how this PR achieves what it says in the title in case anyone looks back at it later?

Updated the description.

@memorycode
Copy link
Contributor Author

@LastTalon, I started updating the docs but I'm still not sure what to call everything. As it stands, the docs for Component say that it returns a ComponentInstance, but this is not true anymore.

@LastTalon
Copy link
Member

but this is not true anymore.

How so? It seems to me like it does.

@memorycode
Copy link
Contributor Author

but this is not true anymore.

How so? It seems to me like it does.

Well, it can return a ComponentInstance, if we want to consider that a ComponentInstance. But I don't think we should consider it a ComponentInstance because we don't want people interacting with it in the same way they interact with table components.

@LastTalon
Copy link
Member

This doesn't seem complicated to me. We clearly just have two types of component instance now. What else would have you it return?

It seems to me it returns a ComponentInstance still. It's just that that type is now a union of TableComponentInstance | ValueComponentInstance.

@memorycode
Copy link
Contributor Author

This doesn't seem complicated to me. We clearly just have two types of component instance now. What else would have you it return?

It seems to me it returns a ComponentInstance still. It's just that that type is now a union of TableComponentInstance | ValueComponentInstance.

Just updated the docs to what I believe they could potentially look like. Curious if this is what you had in mind.

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

Successfully merging this pull request may close these issues.

3 participants