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

Suggestion: Remove the mandatory target argument in the THREE.Ray: .at() and similar APIs #13655

Closed
3 of 13 tasks
wesleyted00 opened this issue Mar 21, 2018 · 10 comments
Closed
3 of 13 tasks

Comments

@wesleyted00
Copy link

wesleyted00 commented Mar 21, 2018

Description of the problem

The optional target in the API like THREE.Ray: .at() has become mandatory since r91. It seems the change was discussed in #12231.

However, I suggest that the target argument can be removed. The reasons are:

  1. The API will be simple and natural. For example, ray.at(0.3) will return a vector object, which is very natural and intuitive.
  2. The reason mentioned in the "Proposal: eliminate the optional target #12231" is not solid. The "methods do not instantiate objects except in constructors and in methods such as clone()" policy is not necessary. Actually, most of APIs create new objects, such as Object3D.js::translateX/rotateZ and on.
Three.js version
  • Dev
  • r91
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@WestLangley
Copy link
Collaborator

ray.at(0.3) will return a vector object, which is very natural and intuitive.

That is true, but it can also lead to unnecessary garbage collection.

It is your responsibility to instantiate objects at the application level. You can create one Vector3 instance and reuse it, for example.

Actually, most of APIs create new objects, such as Object3D.js::translateX/rotateZ

Those are closures. Only one instance of the Vector3 is instantiated, and that instance reused in subsequent calls.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 22, 2018

I think the API is fine as it is.

@wesleyted00
Copy link
Author

  • The expressiveness and naturalness should be much more important for API products.

resultVec = myRay.at(0, resultVec); looks not natural/elegant.

  • "Unnecessary garbage collection". Garbage collection is a gift from Javascript and other high-level languages. How to make GC much more efficient and how to make a better application architecture should not be the focus of API since API has no enough knowledge to decide.

var resultVec = new THREE.Vector3(); myRay.at(0, resultVec);. This does not reduce the garbage collection a lot. The object is created just at a difference place.

  • "your responsibility to instantiate objects at the application level". This is also very weird in modern languages. (In old C++ world, this pattern could be applied to help or force programmers manage memory manually). In a real world, most of API (including the built-in Javascript APIs) may return new objects. If this pattern is enforced, then many many other APIs have to be changed, such as
    raycaster:intersectObjects( objects, recursive, optionalTarget) .

  • "Vector3 is instantiated, and that instance reused in subsequent calls.". In var resultVec = myRay.at(0), the newly created resultVec will of course be reused by application after. Otherwise, why does the application call this API?

@wesleyted00
Copy link
Author

wesleyted00 commented Mar 26, 2018

Please reopen this issue so at least to keep the conversation open. Also, there is no harm to leave it open.

@WestLangley @Mugen87
Thank you.

@WestLangley
Copy link
Collaborator

@wesleyted00 Sorry, this issue was discussed extensively in #12231, and a decision has been made.

@wesleyted00
Copy link
Author

But my concerns in my last post were not discussed much in that thread.

  1. Why APIs should not allocate new objects inside?
  2. How to avoid allocating new objects in all APIs? Is it possible since most of APIs create new objects.
  3. You guys think resultVec = myRay.at(0, resultVec); is elegant?

Thank you.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 10, 2018

But my concerns in my last post were not discussed much in that thread.

Correct because you will find your answers in #12231. With all due respect, I think there is no need to start a new discussion about that topic.

@WestLangley
Copy link
Collaborator

You can write it like this:

myRay.at( 0, resultVec );

@wesleyted00
Copy link
Author

You can write it like this:
myRay.at( 0, resultVec );

Thank you for your comments. @WestLangley

I guess the complete code should be like this:
var resultVec = new THREE.Vector3();
myRay.at( 0, resultVec );

It looks not natural to me. Do you think it is really a good idea to let function argument(s) to receive the return value(s)?

@WestLangley
Copy link
Collaborator

It is a trade-off. You can now do this:

var resultVec = new THREE.Vector3(); // create once and reuse it

myRay.at( 0, resultVec );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants