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

Standardize coordinate conversion methods.. #86

Closed
mramato opened this issue Jun 27, 2012 · 10 comments
Closed

Standardize coordinate conversion methods.. #86

mramato opened this issue Jun 27, 2012 · 10 comments

Comments

@mramato
Copy link
Contributor

mramato commented Jun 27, 2012

Right now, each coordinate type has it's own methods for how conversion to other types works. This can also create circular dependencies when both types want to offer conversion routines; for example adding a toCartesian3 method to Spherical and a toSpherical method to Cartesian3 is not possible without hacking around the require system.

As part of the viewer branch pull request, I added a new object, CoordinateConversions, which makes a good place to store all such routines without the circular dependencies. It also makes it easier for users to find the correct method. For example, creating a Spherical out of a Cartesian3 maps to the sphericalToCartesian3 method (and vice-versa). I think all similar methods should be moved into this object and follow the same pattern.

@shunter
Copy link
Contributor

shunter commented Jun 27, 2012

Maybe I'm just turning into too much of a functional programmer, but I see no reason why these couldn't just be free functions. This avoids pulling in tons of dependencies if you only need to convert one coordinate type.

@mramato
Copy link
Contributor Author

mramato commented Jun 27, 2012

I'm not against the idea of free functions, as long as we reach a consensus; whatever we decide is fine with me. I just want consistency for the sake of a clean API. Where would the free functions go though? Lots of little files in Core? Me and @pjcozzi were just discuss this in the viewer pull, but I better to discuss it here.

I'd even be fine with having them on the objects themselves, but the circular dependency stuff is problematic.

@shunter
Copy link
Contributor

shunter commented Jun 27, 2012

We should definitely have a discussion. My thoughts are to avoid function container objects that don't serve any other purpose, after all, this isn't C# or Java, this is JavaScript, where functions are first-class, so I would just have lots of little files, yes. If they really need to be grouped, then that seems like a good reason to introduce sub-modules, e.g. Core/Coordinates/sphericalToCartesian3.

Instead of using object literals to group functions, make the functions the modules and group them into folders if strongly needed. Anyway, that's just my opinion.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 27, 2012

My gut against separate functions is discoverability - how does a user know what conversions are available? @shunter's idea to introduce a sub-module is good, and certainty solves the problem for AMD users (who probably need it solved more than Cesium.js users who may be writing less serious apps).

However, this creates yet another object creation paradigm. Do users use new, a static create function, a create function on another object (context.createXXX), or a free-floating function?

Is the circular dependency a show stopper for static creation functions? If so, I think @shunter's approach is reasonable.

@shunter
Copy link
Contributor

shunter commented Jun 27, 2012

Avoiding the circular dependency requires e.g. Spherical.createFromCartesian3 static create functions. "from" direction is fine, "to" direction is not (because "to" requires knowing the constructor to use).

@mramato
Copy link
Contributor Author

mramato commented Jun 27, 2012

So one way to do it is to always specify a "from" function but never specify "to" functions. As long as a user knows this, functions on the objects themselves are workable and there are no dependency issues. I think I might like this approach the best.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 27, 2012

@shunter do you agree with @mramato here? It's just a slight shift in thinking - from to/from conversions to constructing an object.

@shunter
Copy link
Contributor

shunter commented Jun 27, 2012

Seems fine. I think avoiding create in the name gives flexibility for in-place conversion into existing objects which we want to have available. Spherical.fromCartesian3(cartesian, result), etc.

@mramato
Copy link
Contributor Author

mramato commented Jun 28, 2012

Sounds like we're all in agreement. I'll probably work on this sometime soon.

mramato added a commit that referenced this issue Jul 13, 2012
1. All prototype functions now have static equivalents.
2. All functions now do proper error checking.
3. All functions no longer require Cartesian3 instances and instead will work with any object that has an x, y, and z properties.
4. All functions that produce a Cartesian3 now take an optional result parameter.
5. Code coverage for Cartesian3 is now 100%
6. Remove a few unused functions.
7. Add several functions to Cartesian2 that already existed on Cartesian3 (everything but cross).

These changes are related to issues #71, #84, #86 (those issues will remain open until we address them for every Core type).
mramato added a commit that referenced this issue Jul 13, 2012
1. All prototype functions now have static equivalents.
2. All functions now do proper error checking.
3. All functions no longer require Cartesian4 instances and instead will work with any object that has an x, y, z, and w properties.
4. All functions that produce a Cartesian4 now take an optional result parameter.
5. Code coverage for Cartesian4 is now 100%
6. Remove Math.angleBetween because it already exists on the Cartesian functions.

These changes are related to issues #71, #84, #86 (those issues will remain open until we address them for every Core type).
@mramato
Copy link
Contributor Author

mramato commented Aug 15, 2012

Since all of our core types now have fromXXX methods for conversions, I'm going to close this out.

@mramato mramato closed this as completed Aug 15, 2012
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