JaimeGensler / thyseus

An archetypal Entity Component System, built entirely in Typescript
MIT License
74 stars 3 forks source link

Add array support to run #14

Closed kayhhh closed 1 year ago

kayhhh commented 1 year ago

untested

kayhhh commented 1 year ago

I can add addSystems() array support, probably good to have all system APIs like this to support the same syntax of accepting both plain systems and arrays.

kayhhh commented 1 year ago

Will assume this includes addSystemsToSchedule()

kayhhh commented 1 year ago

Hm, so I did find an issue with the run changes I added. Using arrays like run([systemA]).last() throws an error, because run is returning SystemConfig[], not a singular config. I can just remove array support here unless you can think of a way to resolve it. run.chain should still work fine.

Also sidenote, is there a reason for dist to be committed? I typically run build to catch errors like this (and it is how I found it now), but its a bit annoying to have to remove my files after the build so they aren't committed.

JaimeGensler commented 1 year ago

I can just remove array support here unless you can think of a way to resolve it

Wondering if it's reasonable for SystemConfig itself to accept multiple systems, so run could pass the array directly to SystemConfig and all systems can be configured together. There may be some issues with this I haven't thought of, but it's a potential solution.

Also sidenote, is there a reason for dist to be committed?

Originally I thought it'd make it easier to keep an eye on build output for anything problematic/odd, but I've also felt lately it's more tedious than helpful. Feel free to add it to the .gitignore here & delete the folder in this PR, or I'll do it in a follow-up.

kayhhh commented 1 year ago

Well I started letting SystemConfig take in arrays and it seems to build ok, but theres a lot of logic im not too sure about, particularly in the executors. I can commit it if you want to take a look but it started to enter the territory of me just guessing what should be done lol. Might be better to make a separate PR for it if you think its worth pursuing since it seems to be a bit involved.

JaimeGensler commented 1 year ago

That makes sense! Expanding support also brings up a lot of questions around things like permitted depth of nesting, which requires a bit more thought. Going to re-review a little later and should be able to get this merged, thanks! 🙌

kayhhh commented 1 year ago

Sounds good. I removed array support from run(...) since it was causing issues, should be working for run.chain, .after, and .before.