Closed oliver408i closed 3 months ago
You edited the build
folder - I believe you need to edit the src
one instead. The change should also be to add import * as THREE from 'three';
at the top of every file rather than rely on window.THREE
(and to export const THREETerrain = ...
rather than assign it to window.THREETerrain
).
Sorry for the delayed review, and thanks for this effort.
arcanis is correct - for this PR to merge, the files in the src
folder need to be changed. The build
files are auto-generated, so any changes to them would get overwritten in the next build.
Beyond that, I agree that it would be preferable to convert this library to ESM style rather than add to the window
global, but I'm okay with window
as a workaround for now. However, ideally we should be able to get rid of the need to load a separate initThree.js
script first.
I will do that (the files are just concat together one by one right? I don't normally use this kind of javascript build system). I did attempt to convert this into an ESM style model but I haven't been successful by doing basic modifications (I didn't want to change too much of the original code, but it seems like that is necessary to fully convert the module). I will continue to experiment with that. Thanks for your review
Update: I managed to get a working ESM style module going, but I don't think it is possible to split it up into that many different files (Terrain is now a class).
Yeah, the build process concats files and then minifies them. I haven't touched how that works since 2015. In this case, the only symbol that would get exported is the Terrain constructor, and everything else gets added as a property on that constructor. So if all the other files get concatenated after core.js, and an export
line is added at the end, that would probably work.
I have added the code to the .mjs files in src. I'm not sure how to compile them, so I'll leave that to you (or you can let me know what to put in the config). Order is: Core.mjs
+ CoreNoise.mjs
+ Terrain.mjs
+ Export.mjs
. I also updated README to include slightly different usages for this module. Note that I included THREE.Terrain.Module.mjs
in the build folder for reference for the complete module file. I know it will get overwritten, its not meant to be permanent.
Thank you for putting the work into this and sorry for the delayed response. This has been a busy couple weeks for me.
There is a lot going on in this PR. Documentation stripped out, files combined - it makes for a difficult review because it's hard to quickly see what has changed. I also need to set up a new example to be able to test the changes. I am not going to be able to put that much time into this for now, so what I suggest is for you to fork this repo with your changes, and I will change the README here to point to your fork as a place where new development is happening with JavaScript modules. How does that sound?
Sounds good! I updated my fork's README with some more info. Let me know if I should add/change anything else. I don't have an example set up, but I do have my own game that uses the ESM version of this module (which is why i'm here in the first place), so I'll link that as a "example" for now.
Thanks for your time reviewing this PR!
Thanks. I updated the README in this repo to point to your fork.
Adds compatibility for up-to-date THREE.js and ES6 web standards (Javascript modules mostly).
Big disclaimer: this is a really stupid way to get compatibility without editing the code much, and it requires additional setup before you can use it!
Note: I only edited the output file (the non-min one), not any src files
See #38 for details on what was changed.
If you are going to use this, two changes are necessary:
window.THREE
toTHREE
(imported from an es6 module). See example belowTHREE.Terrain
, usewindow.THREETerrain
initThree.js
HTML Make sure you have an import map set up first
Then load your scripts
For a demo on how to use this, please see my game, https://github.com/oliver408i/missilesim
Additional technical notes: THREE.Math functions were changed to their identical THREE.MathUtils ones