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

Cleanup ShaderNode #23820

Merged
merged 3 commits into from
Mar 31, 2022
Merged

Cleanup ShaderNode #23820

merged 3 commits into from
Mar 31, 2022

Conversation

LeviPesin
Copy link
Contributor

Related issue: -

Description

Cleanup ShaderNode.

@sunag
Copy link
Collaborator

sunag commented Mar 31, 2022

Let's avoid declaring ShaderNode outside of ShaderNode.js and float, uint, etc outside of ShaderNodeElements.

@sunag sunag closed this Mar 31, 2022
@sunag sunag reopened this Mar 31, 2022
@LeviPesin
Copy link
Contributor Author

Let's avoid declaring ShaderNode outside of ShaderNode.js

Why? I think it would be better not to export NodeHandler and ShaderNodeScript directly.

float, uint, etc outside of ShaderNodeElements.

I did not revert this change here (I only made a cacheMaps object from cacheMaps).

@sunag
Copy link
Collaborator

sunag commented Mar 31, 2022

Why? I think it would be better not to export NodeHandler and ShaderNodeScript directly.

It would be the correct representation of file and class.
There are also plans to turn ShaderNode into an extended Node.

I did not revert this change here (I only made a cacheMaps object from cacheMaps).

Yes, yes!! So I reopened, that part was great. :)

@LeviPesin
Copy link
Contributor Author

OK, is it better now?

@sunag sunag merged commit 0f28222 into mrdoob:dev Mar 31, 2022
@sunag
Copy link
Collaborator

sunag commented Mar 31, 2022

Thanks!

@sunag sunag added this to the r140 milestone Mar 31, 2022
@LeviPesin LeviPesin deleted the cleanup-shadernode branch March 31, 2022 16:29
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Cleanup ShaderNode

* Change

* Fix
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