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

Scripting Support #99

Closed
wants to merge 3 commits into from
Closed

Conversation

zicklag
Copy link

@zicklag zicklag commented Jun 7, 2020

Hey there 👋

Here's a PR that attempts to migrate from using TypeId to using StorageId in an effort to start to address #96.

I may have missed a spot here or there, and this doesn't address anything an API change to facilitate creating custom components yet ( necessarily ) it just switches everything I could find that should take a StorageId instead of a TypeId.

Definitely needs some review, but all of the tests pass.

Comment on lines +31 to +32
// TODO: Re-include the TypeIdHasher as a StorageIdHasher
storages: UnsafeCell<HashMap<StorageId, Storage /*, BuildHasherDefault<TypeIdHasher>*/>>,
Copy link
Author

Choose a reason for hiding this comment

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

Do we need to re-implement our own hash now that we are using StorageId now, or is the default one going to be fine?

Copy link
Owner

Choose a reason for hiding this comment

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

The default one is (and always was) fine. It was a good opportunity to learn about Hash and Hasher but it's probably a good time to remove the custom hasher, it'll simplify things.

Copy link
Author

Choose a reason for hiding this comment

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

OK, I'll just remove that, then.

@leudz
Copy link
Owner

leudz commented Jun 8, 2020

I think we have to discuss the api before I can merge this PR. Based on the api, if some part can't be used with ffi views, it'll make more sense to use TypeId in this part of the codebase.

I think a good example of this is systems, they can't work without types. I think ffi systems will do the same thing as workloads.

@zicklag
Copy link
Author

zicklag commented Jun 8, 2020

That makes sense. I've got a plan for a prototype API for scripted systems and components in mind so I'll rename this PR to "Scripting Support" and get those changes up when I have them so we can discuss it.

I'm not sure if I'm missing something huge or not, but it looks like it won't be too difficult. So far the code-base has made complete sense to me which is great. I wasn't sure if I would be completely lost if I looked into it.

Anyway we'll see how well that goes. 😃 👍

@zicklag zicklag changed the title Migrate From TypeId to StorageId WIP: Scripting Support Jun 8, 2020
@zicklag zicklag marked this pull request as draft June 8, 2020 17:24
@zicklag zicklag changed the title WIP: Scripting Support Scripting Support Jun 8, 2020
@leudz
Copy link
Owner

leudz commented Aug 11, 2021

I'm closing it but I'll take another look inside when I finally try to do scripting 😅

@leudz leudz closed this Aug 11, 2021
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.

2 participants