fenomas / noa

Experimental voxel game engine.
MIT License
611 stars 87 forks source link

Typescript migration branch #135

Closed fenomas closed 3 years ago

fenomas commented 3 years ago

This issue is to track the status of the typescript migration work from @SamKirkland.

I'm definitely interested in trying this out, but also not sure I want to change languages. However either way I'm heads-down on my game client work at the moment so I have not yet done all the stuff I'd need to do to build against a TS library. So for now I'm merging the PR (mostly sight unseen) into its own branch, so that anyone who wants to build their project against it can do so.

Please let me know if anything about the current branch setup prevents TS users from building against this branch...

Patbox commented 3 years ago

I think it would be useful to have already transpiled copy in the repo, so users can use use npm i.

Also after using transpiled version, I'm getting some errors, but it might be something I did wrong. image

fenomas commented 3 years ago

Weird, I'm certain I replied to this.... I think I was on shaky wifi at the time.

I'm afraid I haven't used TS and don't have a toolchain set up for it, so I can't really build or test anything. I can pull in a PR for one of course if it's useful though.

No idea about the error though. It might be the same as #142, which seemed (?) to be some kind of npm bug, which seemed to go away once you run npm i a second time.

braebo commented 3 years ago

Edit: I have a clone where I've gotten noa running with vite/rollup - it runs beautifully! I didn't have to change much.

My comment earlier was dumb, and this engine is brilliant 🤯. I would love to see it Typescript too! I'd be happy to contribute.

braebo commented 3 years ago

FYI, my vite/rollup version is using Svelte for UI. Many consider Vite (built on rollup) the successor to Webpack- but it only supports ES6. So the only real difference in my clone is the change of all uses of require() to import (and const/let in place of var which isn't necessary but helps squeeze out a bit more performance). Aside from that, I had to do some minor modifications to micro-game-controller, the Buffer hack, and another hack that sticks DebugLayer and DebugLayerTab onto the global oject to get @babylonjs/inspector working.

I also added the sprint and crouch found in one of the TODOs.

Not sure if the clone would be useful to you or anyone else, but if so, let me know!

Regardless, thank you for your work on this awesome project!! You're a wizard with this stuff.

fenomas commented 3 years ago

Hi, sorry for the slow replies. The main reason I'm hesitant about converting to TS is that, noa being strictly a dependency, it would (I assume?) mean that all client apps would have to convert to TS as well (or at least start building via the TS compiler) even if they aren't currently.

But the ideal situation would be if client apps using TS get the benefits of using noa as if it was a TS library, while non-TS projects can still build noa directly without tsc.

Can someone who's more familiar with TS confirm that that makes sense and is possible? I guess this would be done by adding definition files to noa, which would hopefully be autogenerated from the library source. Or is there more to it than that?

If this is the right way to go, it would be very helpful if a TS user could create some kind of "sample TS game client" repo, which functions like noa-examples but uses TS. Something I could clone and npm i and then see whether it's building correctly, getting the right autocorrect, and so on.

@FractalHQ can you advise?

Patbox commented 3 years ago

Hi, sorry for the slow replies. The main reason I'm hesitant about converting to TS is that, noa being strictly a dependency, it would (I assume?) mean that all client apps would have to convert to TS as well (or at least start building via the TS compiler) even if they aren't currently

That's not the case, if library is pre compiled to JS before being publisher (most do that, including BabylonJS).

It would benefit both, as TS definition files would be more accurate and autocomplition would work better.

fenomas commented 3 years ago

Ah, of course that makes more sense. Is it set in stone that client apps always import prebuilt JS, or could a JS app import prebuilt files while a TS client skips that and imports from the TS?

Patbox commented 3 years ago

Generally most will use prebuild JS, as it's easier to import (npm) and it's supported by all bundlers. There won't really be a difference while developing with TS files anyway, as definitions will be identical. Only case where it will matter is when someone copies into project/modifies engine itself, but it shouldn't be a problem

fenomas commented 3 years ago

Thanks. And to check, this JS prebuild pattern will work normally even though noa imports from Babylon as a peer dependency, and Babylon won't be present during the pre-build step? Or does that change things?

Patbox commented 3 years ago

It will complain while compiling/writing code for sure, but downloading it while still declaring it as peerDep should work just fine

fenomas commented 3 years ago

Wouldn't that mean that most of Babylon would get included in the final bundle twice, being imported into both the prebuilt engine bundle and the client code?

Patbox commented 3 years ago

If you use tsc for that it shouldn't bundle anything (it only transpiles it to js and d.ts files). Distribution itself should still look the same

fenomas commented 3 years ago

(removing this as it's no longer current)

fenomas commented 3 years ago

Hi, I've pushed a big revision in this direction. It would be great if anyone using noa with a typescript client could check whether it breaks your game.

The code is now merged into branch #develop. Everything is still JS, but it's now typescript-friendly JS with .d.ts type declarations that are built from the library source instead of being statically hand-authored. So, TS users should hopefully be able to import from this branch, and get quality code hints (in VSCode anyway) that will stay updated going forward.

Note, the code will also compile with tsc; if you want to you can set up your local rep to emit code instead of just type declarations. I don't thin, there are any particular advantages to this though.

As a bonus, there's now a real API reference built with typedoc, inside docs/API. It's not completely terrible like the current API reference 👍

There should be only one breaking change in the update:

That's it. Feedback from any typescript users would be really helpful!

Patbox commented 3 years ago

I didn't have time to test it, but at first glance it looks like major improvement. I plan to start rewriting my game soon so it lined up perfectly :)

fenomas commented 3 years ago

Thanks for looking. It would be great if you get a chance to try building with it, I'd like to push this out as a new version update once someone can confirm it doesn't break their builds.

Patbox commented 3 years ago

First test with pre-rewrite VoxelSrv:

fenomas commented 3 years ago

@Patbox Thanks for checking. On the last two, I did a pass over everything marking optional parameters - hopefully should be right now.

On the 1st and 3rd, do those prevent you from building, or are they just missing type information? Those types are coming from type files that I build with tsc and am hoping not to need to hand-author (but maybe that's not possible).

2nd point, sorry you lost me.

Patbox commented 3 years ago

1st and 3rd is just missing type info, it builds just fine.

For 2nd, it looks like setPosition (and set/getBlock) was changed with this commit to only allow arrays (before it allowed passing x, y, z by itself), but later this commit reverted this change for set/getBlock, but not setPosition

fenomas commented 3 years ago

Ah I missed that. Okay, I pushed that revert. The missing types, I'll leave for now - the right fix is to revise the ECS library so that it emits proper types of its own.

Thanks for checking, if it's otherwise okay for you I'll push this to the main branch soon (way overdue for a release...)

Patbox commented 3 years ago

I didn't find any other problems, so you can push it!

fenomas commented 3 years ago

Thanks again for checking!

I'll close this issue for now, but please open new ones down the road for any specific typescript-interop issues (like exposing better type decs for dependencies).