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

[WIP] JSM: Move nodes/* #16054

Closed
wants to merge 4 commits into from
Closed

[WIP] JSM: Move nodes/* #16054

wants to merge 4 commits into from

Conversation

donmccurdy
Copy link
Collaborator

All files in examples/js/nodes/* were already ES modules. This PR moves them to the jsm/ folder, adds imports from the main threejs library, and fixes a few stray THREE.*Node references that needed to be imports. With that, it should be possible to import them in the same way as other jsm/ examples:

import { Scene, Mesh, BufferGeometry } from 'three';
import { StandardNodeMaterial, FloatNode, ... } from 'three/examples/jsm/nodes/Node.js';

var scene = new Scene();
var mesh = new Mesh( new BufferGeometry(), new StandardNodeMaterial() );
scene.add( mesh );

I've only fixed the first two examples, and will fix the remaining five if we want to proceed with this PR. Note that I've removed the original js/ files, because this change was not fully automated, and it probably isn't worth the effort to do so.

/cc @sunag

@sunag
Copy link
Collaborator

sunag commented Mar 25, 2019

@donmccurdy Very niceee, thanks!

Leveraging the subject my doubts about ES modules... Is there any demarcation today that would prevent we advance in class instead of prototype?

@sunag
Copy link
Collaborator

sunag commented Mar 25, 2019

I think that THREE.Nodes.js can be removed since it has lost its function.

@donmccurdy
Copy link
Collaborator Author

Is there any demarcation today that would prevent we advance in class instead of prototype?

I'd be fine with using ES6 Classes in the nodes/ folder, unless others think it's important to keep the whole codebase more consistent for now?

@sunag
Copy link
Collaborator

sunag commented Mar 25, 2019

Because this PR is still a WIP? I think is important merge for add futures updates.

@donmccurdy
Copy link
Collaborator Author

I will finish this PR soon. 👍

After that, whatever you prefer on ES6 Classes is OK with me.

@donmccurdy
Copy link
Collaborator Author

Hm, I'm not sure I can convert postprocessing / nodes / pass without also converting postprocessing/* to ES modules. Those files are heavily dependent on one another, and as a result we need to either:

(a) modify modularize.js to handle JSM-to-JSM dependencies
(b) do a one-time conversion of postprocessing/* to jsm/ and start generating the js/ version from that
(c) ???

I think I prefer (b), but it requires #15599 or something similar. Maybe that can be simplified a bit; I'll comment there.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 20, 2019

Hm, I'm not sure I can convert postprocessing / nodes / pass without also converting postprocessing/* to ES modules.

The postprocessing classes are now available as modules 😇 .

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 26, 2019

@donmccurdy Do you think you are able to finish this PR in the next time 😇? The node material files and the related examples are one of the last todos in our JSM/module task list, see #16688

BTW: It could make sense to start from scratch since many things changed in the last weeks. If you are busy with other stuff, I can do this, too.

@donmccurdy
Copy link
Collaborator Author

@Mugen87 the next thing I want to finish up is #15249, and it will be at least late July before I could take another look at this PR unfortunately. I'm not sure how much has changed that would affect the PR, but if you are able to have a look please do! 🙂

@Mugen87 Mugen87 closed this Jun 26, 2019
@donmccurdy donmccurdy deleted the jsm-nodes branch June 26, 2019 16:34
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