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

Reverse cleanup of #13543 #13859

Closed
wants to merge 3 commits into from
Closed

Reverse cleanup of #13543 #13859

wants to merge 3 commits into from

Conversation

paulmasson
Copy link
Contributor

@mrdoob the supposedly minor cleanup on #13543 forces the end user to work in a way that IMHO disrupts how a mathematically inclined person thinks about functions. A parametrization function returns the coordinates corresponding to the input parameters: it doesn't modify the parameters. I should think someone using ParametricGeometry would expect a function to behave like a mathematical function.

With the cleanup reversed, the end user still has the option to modify a third argument if desired for efficiency, but doesn't force that person to think non-mathematically. I would really like to know whether or not this cleanup was strictly necessary from a performance point of view.

@WestLangley
Copy link
Collaborator

WestLangley commented Apr 13, 2018

three.js no longer will instantiate a new Vector3 every time a user evaluates a function.

If the user is able, the user can instantiate one Vector3 instance and reuse it.

This pattern is extensively used in this library. For example:

https://github.com/mrdoob/three.js/blob/dev/src/math/Ray.js#L40-L51

@pailhead
Copy link
Contributor

pailhead commented May 10, 2018

Not all functional paradigms can be applied to three. This is an anti-pattern with threejs

@Mugen87
Copy link
Collaborator

Mugen87 commented May 17, 2018

Closing. Merging this would be a return to the old system.

@Mugen87 Mugen87 closed this May 17, 2018
@paulmasson
Copy link
Contributor Author

Then there should be an indication for the end user as to why it simply fails: #14085.

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.

4 participants