-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Require result parameter #1161
Comments
As with the prototype changes, I like this change for all Cesium code in master however I am a little nervous about its effect on usability for our end developers. However, I'm fine with making the change and waiting to see if people complain about it. |
As for usability, it is better overall because there is only one way to do things; a user doesn't have to decide. Yes, the result parameter is strange at first, but so is the lack of operator overloads. We are playing the hand we were dealt with JavaScript for performance. I should also mention that I've advocated several times to still assign the return value to the result argument in case the result is
I'm a bit on the fence, but we should consider doing this everywhere. |
I assume you're saying that your above code is okay, but the bug was doing the below (where an undefined result would never get a value)? If so, then I agree, the above pattern is better. (and I assume has negligible performance implications.)
|
Yeap. |
Low-hanging fruit: in addition to the fundamental types, |
FIY, @hpinkos is looking at the scope of this to see if it is reasonable. |
|
Thanks @hpinkos |
As discussed on the forum, we should have our fundamental types (matrices, cartesians, quaternions, etc.) require result parameters.
We saw in the Cygnus simulation that just one unused result parameter in the right spot can result in a significant performance drop, e.g., 30 to 20 fps. We need to be fast by default.
Although not part of this issue, I will also make sure that rendering the default scene does not allocate any memory once terrain and imagery are loaded for a given view.
CC #1082.
The text was updated successfully, but these errors were encountered: