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

Renderers: move to es6 class syntax #19008

Closed
wants to merge 6 commits into from
Closed

Renderers: move to es6 class syntax #19008

wants to merge 6 commits into from

Conversation

DefinitelyMaybe
Copy link
Contributor

@DefinitelyMaybe DefinitelyMaybe commented Apr 1, 2020

This Pull request is not ready yet.
There are a couple more scripts from the renderers folder that can be converted.

relates to #18863

edit: have resolved the initial issue.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 1, 2020

materials / texture3d is a good example to try because it's not continually calling the depreciated function.

Can you please share the line of the example that calls initMaterial().

To be clear: I'm not sure you have understood what I have mentioned in one of your earlier posts. I don't see much sense to make such PRs since the class migration has been stopped right now. I appreciate your effort but nobody will review these PRs. Besides, if the migration is started, it seems necessary that the core team performs this change. At least in the library's core. Otherwise a lot of stuff gets broken like in your math PR (#18863).

@DefinitelyMaybe
Copy link
Contributor Author

hey. I'm all good with these PRs sitting here and I can revisit my math PR later too. I'm sure you know more than me about what direction the development should go in. last word I remember on classes was:
#6419 (comment)

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 8, 2020

I've downloaded your branch and realized that it's not even possible to build three.js.

Error: super() call outside constructor of a subclass
[ROLLUP] src/renderers/WebGLMultisampleRenderTarget.js (19:2)

That demonstrates that all your changes are untested. I'm sorry to say this but I think it's better if you stop making PRs since they are not helpful in the ES6 migration process.

@Mugen87 Mugen87 closed this Apr 8, 2020
@DefinitelyMaybe
Copy link
Contributor Author

interesting. I've been checking the unit tests for all of my PRs.
could you please explain the steps you took to get to that error?

@DefinitelyMaybe
Copy link
Contributor Author

there's also the fact that after after making my changes I ran them on a couple of examples via the local dev server.

ok, how about this. assuming I did something wrong, it'd be more appropriate to say hey I couldn't quite get thing's to work, could you please get back to me on these errors. Saying stop making PRs because they're not helpful is ... more unhelpful.

@mrdoob
Copy link
Owner

mrdoob commented Apr 9, 2020

@DefinitelyMaybe

Sorry about that. As you can see, we are having issues reviewing PRs quickly these days. Creating more issues and more PRs that don't come from an agreed discussion is more work for us and it's likely that they'll get unreviewed and eventually outdated.

Using #19041 as an example, I'm sorry you spent the time but it was not something we considered an issue. In fact, that change would create more issues in our workflow and break links for the whole community.

When contributing to OS projects, unless you're fixing an obvious bug, it's better to follow conversations a bit to see what the group is trying to do. Otherwise, not only the proposed PRs may not be merged, but other PRs that are good to go can get delayed because we couldn't handle the load and we were not able to review them.

Hope you understand.

@DefinitelyMaybe
Copy link
Contributor Author

Apology accepted.

To put things in perspective, I've never gone this far into contributing to an OS project before and a lot of what may be considered obvious is not for me. Hopefully, ya'll have some patience for a new dev.

it's better to follow conversations a bit to see what the group is trying to do.

Honestly, that was already my step 0. Read through to the bottom of the various issues I was interested in and tried to do what I thought was progress.

From a distance, reviewing PRs isn't the only thing in need of some TLC at the moment.

@DefinitelyMaybe DefinitelyMaybe deleted the WebGL-Renderers--move-to-es6-classes branch June 6, 2020 01:17
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