Comments (20)
The static versions are useful because user's don't need to allocate, for example, Cartesian3
. Instead, they might have parsed JSON and just have an object literal with x
, y
, and z
properties, or they have some other object that has x
, y
, and z
properties and others but it still works with our math functions.
from cesium.
I understand the reasoning, and we can certainly have static versions of every method too if we think it's worth while. In my experience so far, however, I haven't needed the static versions of anything and it just seems like a lot of redundant code to have both. I think the static versions it the type of thing that sounds really useful, but in practice doesn't really add a lot of value (but adds a lot of code). I could be convinced otherwise though.
from cesium.
Another thought. If we're smart, we would implement the prototype versions in terms of the static versions to avoid duplicate code. However, this adds an extra method call to the prototype versions; which unless the compiler is doing some inlining would certainly make the prototype versions slower. Since the static versions would be faster, it would make sense to use them everywhere at that point, and then get rid of the prototype versions. Of course this is some conjecture on my part; if the compilers are fast enough to eliminate that extra method call, this point is moot.
from cesium.
For Cesium users, they are very useful. I wanted them several times when dog-fooding. In fact, we decided that all public interfaces should take Cartesian3
or an object with x
, y
, and z
properties, which is why many functions allocate a temporary Cartesian3
. @kristiancalhoun made the change. This was a baby-step to static functions.
I'm not sure about the inlining. I also argued against static functions because it is double the amount of code to write, document, test, etc., but it is only for a handful of classes, so I suppose we can justify it. @shunter and I have debated this several times over the past six months or so.
from cesium.
Yeah, I've talked to @shunter a lot about it too. As I said, I have no problem having both, so here's my proposal.
- All of our core types should have their last parameter be an optional result parameter as I defined above.
- All of our core types should work whether the passed in object is an instance of the type, or just and object that has the same properties
- We should have both static and prototype versions of all methods on core types.
Assuming we all agree on this, it's something I want to work on right after the viewer pull request is in.
from cesium.
Supporting (2) sounds a bit painful, right? Sometimes there will be:
if (result) { if (result === this) { // ... } else { // ... } } else { // ... }
Maybe that case is rare enough. Have you looked at other popular JavaScript vector and matrix libraries? If after looking at them, you still propose the above, I am OK with it.
from cesium.
The logic can be simplified a lot. At the start:
if (typeof result === 'undefined')
result = new Cartesian3();
Then, make all of the internal work within the methods functional instead of object-oriented, that is, use the static versions internally.
from cesium.
I haven't looked at the other libraries, but I'll check them out. I've already implemented my strategy in a few classes and it's not hard at all and works really well. For a simple example, see the new Spherical class in the viewer branch:
https://github.com/AnalyticalGraphicsInc/cesium/blob/viewer/Source/Core/Spherical.js
For something more complicated, I've also made some change to Quaternion, check out Quaternion.prototype.multiply. (there's no static version yet)
https://github.com/AnalyticalGraphicsInc/cesium/blob/viewer/Source/Core/Quaternion.js
I don't plan on doing any more modifications to the viewer branch, as my goal was to do the minimum needed for the branch. I'll do everything else on a new branch.
from cesium.
@shunter Does that work all the time? The following fails when left
or right
equals result
. It's a read after write hazard.
Cartesian3.prototype.cross = function(other, result) { if (typeof result === 'undefined') { result = new Cartesian3(); } Cartesian3.cross(this, other, result); return result; }; Cartesian3.cross = function(left, right, result) { result.x = left.y * right.z - left.z * right.y; result.y = left.z * right.x - left.x * right.z; result.z = left.x * right.y - left.y * right.x; };
from cesium.
Your implementation of Cartesian3.cross is bad, it should do the property look ups first, this one works in your example. This is also probably a little faster (though negligibly so), since it eliminates multiple property look ups.
Cartesian3.cross = function(left, right, result) { var leftX = left.x var leftY = left.y var leftZ = left.z var rightX = right.x var rightY = right.y var rightZ = right.z result.x = leftY * rightZ - leftZ * rightY; result.y = leftZ * rightX - leftX * rightZ; result.z = leftX * rightY - leftY * rightX; };
from cesium.
Actually, my Cartesian3.cross
implementation was by design. Maybe the copies are faster. I can't rationalize about JavaScript performance. In C++, the function would be inlined along with other functions and all those copies would result in lots of register pressure which decreases the amount of parallelism gained by out of order and super scalar execution. In JavaScript? No idea what code is generated. Is it enough to matter? Probably not. I'm sold.
from cesium.
Cartesian3.prototype.cross = function(other, result) {
return cross(this, other, result);
};
var cross = Cartesian3.cross = function(left, right, result) {
if (typeof result === 'undefined') {
result = new Cartesian3();
}
var leftX = left.x;
var leftY = left.y;
var leftZ = left.z;
var rightX = right.x;
var rightY = right.y;
var rightZ = right.z;
result.x = leftY * rightZ - leftZ * rightY;
result.y = leftZ * rightX - leftX * rightZ;
result.z = leftX * rightY - leftY * rightX;
return result;
};
from cesium.
And by "bad" I meant incorrect for its purpose, I didn't mean to call your code "bad".
from cesium.
Looks like me and @shunter implemented the same exact functions independently of each other. I'm going to name this "proof by duplication".
from cesium.
@mramato I based mine on yours, just to get it as concise I can. Sorry to invalidate your proof.
from cesium.
Long story short, yes, I'm in full support of this, but, as I originally stated, you have to agree it is quite a bit more than our current code:
Cartesian3.prototype.cross = function(other) { return new Cartesian3( this.y * other.z - this.z * other.y, this.z * other.x - this.x * other.z, this.x * other.y - this.y * other.x); };
from cesium.
@pjcozzi I definitely agree with you and I'm glad we've reached a consensus overall. I blame Javascript :)
@shunter Too bad. I was being lazy, so thanks for putting the whole thing together.
from cesium.
Might be worth looking at these too - http://stepheneb.github.com/webgl-matrix-benchmarks/matrix_benchmark.html
from cesium.
Awesome, thanks. I had previously checked out some of these library and all of the fast ones followed patterns similar to what I laid out above, so I think we're on the right track. Looks like this site is available on github here: https://github.com/stepheneb/webgl-matrix-benchmarks It would be great for someone to add Cesium to the mix, this way we know how were stand (and what's possible).
from cesium.
I'm closing this issue. While memory pressure is still a problem in some areas of the code-base, as a team we are much more aware of them and fix them as we come across them. Core itself is in pretty good shape. There's no need to have an overly general issue like this.
from cesium.
Related Issues (20)
- Add `cspell` checking
- Orthographic camera projection HOT 4
- Can the options in WebMapServiceImageryProvider support customTags field? HOT 6
- `Unhandled promise rejection: undefined thrown` in CI
- External tilesets are disallowed inside multiple contents
- valid glTF fails to load in CesiumJS HOT 3
- Can't find ion-sdk-widgets/Source/TransformEditor HOT 1
- RequetErrorEvent during Jest testing. HOT 1
- Request a function `Cartesian3.intersect` HOT 1
- Move `Viewer` functionality to `CesiumWidget` class HOT 1
- `WallGraphics`'s Property `outlineWidth` do not work HOT 2
- PostProcessStage not work correctly HOT 4
- Documentation / TypeScript definition error/regression HOT 1
- Unable to zoom in after setting the relative height of vertical exaggeration HOT 2
- Render polygons according to orientation and kilometers HOT 2
- Add support for glTF KHR_materials_anisotropy extension
- Property Textures refer to all meshes
- InterpolateHeight compute height and returns undefined HOT 2
- Support For PBF HOT 4
- When using polygon to draw an arch, it will collapse. HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from cesium.