mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.73k stars 35.38k forks source link

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

Closed wesleyted00 closed 6 years ago

wesleyted00 commented 6 years ago
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 https://github.com/mrdoob/three.js/issues/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 "https://github.com/mrdoob/three.js/issues/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
Browser
OS
Hardware Requirements (graphics card, VR Device, ...)
WestLangley commented 6 years ago

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 commented 6 years ago

I think the API is fine as it is.

wesleyted00 commented 6 years ago

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.

wesleyted00 commented 6 years ago

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 commented 6 years ago

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

wesleyted00 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

You can write it like this:

myRay.at( 0, resultVec );
wesleyted00 commented 6 years ago

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 commented 6 years ago

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

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

myRay.at( 0, resultVec );