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

Use the new proj4.js #1228

Merged
merged 19 commits into from Jul 10, 2014
Merged

Use the new proj4.js #1228

merged 19 commits into from Jul 10, 2014

Conversation

@ahocevar
Copy link
Member

@ahocevar ahocevar commented Nov 5, 2013

This change updates ol3 to work with the new proj4.js. Proj4js versions < 2.2 are no longer supported.

@twpayne
Copy link
Contributor

@twpayne twpayne commented Nov 5, 2013

Did Proj4js really break all backwards compatibility with a point release?

Should we really drop all support for earlier versions Proj4js?

Is proj4js hosted on a CDN somewhere? The GitHub folks tend not to like people using raw.github.com for content delivery.

@ahocevar
Copy link
Member Author

@ahocevar ahocevar commented Nov 5, 2013

The API is still backwards compatible, but the namespace has changed. Anyway, we'll see that we can get the new proj4.js hosted on a CDN. @calvinmetcalf, any ideas on this?

When done, I'll update the urls. And I see no reason why it would make sense to maintain support for earlier versions of Proj4js.

@probins
Copy link
Contributor

@probins probins commented Nov 5, 2013

Proj4js versions < 1.3 are no longer supported

just a quick comment. I'd suggest adding something to this effect to the api docs: ol3 handles 4326/3857 transform; for other projections, load proj4.js together with appropriate def(s); not compatible with older versions of Proj4js. I was going to add something to the docs based on @twpayne's comment on #1222 but there's no point if this new code is ready to roll.

@probins
Copy link
Contributor

@probins probins commented Nov 5, 2013

the namespace has changed

also, defs is now a function instead of an array

@probins
Copy link
Contributor

@probins probins commented Nov 5, 2013

proj4.js hosted on a CDN

why not just submit to cdnjs?

@calvinmetcalf
Copy link

@calvinmetcalf calvinmetcalf commented Nov 5, 2013

so fyi the only version we published besides 1.3 was 1.1, so it would be more accurate to say we depreciated version 1.1, which in hindsight probobly shouldn't have been published anyway as it was very alpha quality)

@ahocevar
Copy link
Member Author

@ahocevar ahocevar commented Nov 5, 2013

Not true @calvinmetcalf. There was 1.0 and 1.1 (same API as 1.0) published off SVN, and 1.0 has been used in production and together with other libraries for by many and for years.

@calvinmetcalf
Copy link

@calvinmetcalf calvinmetcalf commented Nov 5, 2013

Ah woops I stated counting based off the wrong thing. Yeah add a note saying proj4 isn't backwards compatible with proj4js.

Only a related note, going forward it's probably a bad idea for you guys to rely on objects being plane objects as opposed to function or anything else that is an object but has a different type. That's rarely explicitly documented and can be avoided with duck typing via 'obj && obj.prop' style tests.

@probins
Copy link
Contributor

@probins probins commented Nov 7, 2013

I think you can remove proj from examples/wmts-capabilities which afaics is just parsing text :-)

@ahocevar
Copy link
Member Author

@ahocevar ahocevar commented Nov 18, 2013

This needs more work - I'll update the pull request to work with Proj4js 2.0.0.

@@ -11,9 +11,9 @@ goog.require('ol.source.WMTS');
// CRS. We include the Proj4js definition of that projection to prevent
// Proj4js from requesting that definition from spatialreference.org.

This comment has been minimized.

@probins

probins Dec 8, 2013
Contributor

this whole business can be removed, can't it? AFAICS, the new proj doesn't do anything with spatialreference.org

This comment has been minimized.

@calvinmetcalf

calvinmetcalf Dec 8, 2013

Yup, you may still need to include the epgs code thought
On Dec 8, 2013 11:47 AM, "Peter Robins" notifications@github.com wrote:

In examples/wmts-ign.js:

@@ -11,9 +11,9 @@ goog.require('ol.source.WMTS');
// CRS. We include the Proj4js definition of that projection to prevent
// Proj4js from requesting that definition from spatialreference.org.

this whole business can be removed, can't it? AFAICS, the new proj doesn't
do anything with spatialreference.org


Reply to this email directly or view it on GitHubhttps://github.com//pull/1228/files#r8185293
.

@bartvde bartvde mentioned this pull request Dec 9, 2013
@ximex
Copy link

@ximex ximex commented Jan 15, 2014

Proj4js 2.1.0 released and should push to cdnjs.

If you update this please change from raw.github to the cdn

Edit: Create Issue for cdnjs: cdnjs/cdnjs#2499

PS: rename Soldier-Boy -> ximex

@ahocevar
Copy link
Member Author

@ahocevar ahocevar commented Jan 15, 2014

@Soldier-Boy Thanks for the hint, will do. As stated above, this needs more work anyway.

@pagameba
Copy link
Member

@pagameba pagameba commented May 17, 2014

A note about Proj4js, its use of Array methods map, filter, reduce, forEach, indexOf, Array.isArray, make it incompatible with IE8 and earlier which negates some of the benefit of #1605.

@calvinmetcalf
Copy link

@calvinmetcalf calvinmetcalf commented May 17, 2014

I will point out that Microsoft no longer supports ie8 and there are
several fine es5 shims that fix this
On May 17, 2014 7:13 PM, "Paul Spencer" notifications@github.com wrote:

A note about Proj4js, its use of Array methods map, filter, reduce,
forEach, indexOf, Array.isArray, make it incompatible with IE8 and earlier
which negates some of the benefit of #1605#1605.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1228#issuecomment-43426765
.

@pagameba
Copy link
Member

@pagameba pagameba commented May 19, 2014

@calvinmetcalf I long for the day when IE users upgrade like other users do and we don't have to even think about supporting browsers before the current version :) Shims in conditional comments FTW.

@ahocevar
Copy link
Member Author

@ahocevar ahocevar commented Jul 7, 2014

I updated the pull request based on @probins's suggestions. Transparent proj4js integration is still there, but much simpler, and the API was updated to make working with custom coordinate transform functions easier.

This is ready for review.

@probins
Copy link
Contributor

@probins probins commented Jul 8, 2014

thanks @ahocevar. At first glance, this looks vg. I'll pull in your branch and take a more detailed look.

@elemoine
Copy link
Member

@elemoine elemoine commented Jul 8, 2014

I'm interested to have a look at this. Hope to do this tomorrow or Thursday.

@@ -30,6 +30,7 @@
"taffydb": "~2.7.0",
"temp": "~0.7.0",
"underscore": "~1.6.0",
"walk": "~2.3.3"
"walk": "~2.3.3",
"proj4": "~2.1.4"

This comment has been minimized.

@probins

probins Jul 8, 2014
Contributor

I think these are alpha sequence

This comment has been minimized.

@ahocevar

ahocevar Jul 8, 2014
Author Member

What does that mean? Should I remove the ~?

This comment has been minimized.

@calvinmetcalf

calvinmetcalf Jul 8, 2014

The Tilda ~ means 1.1.x or I will take patches but not backwards compatible
changes, usually you want a carrot ^ which is 1.x.x or I will take all non
breaking changes
On Jul 8, 2014 5:40 AM, "Andreas Hocevar" notifications@github.com wrote:

In package.json:

@@ -30,6 +30,7 @@
"taffydb": "~2.7.0",
"temp": "~0.7.0",
"underscore": "~1.6.0",

  • "walk": "~2.3.3"
  • "walk": "~2.3.3",
  • "proj4": "~2.1.4"

What does that mean? Should I remove the ~?


Reply to this email directly or view it on GitHub
https://github.com/openlayers/ol3/pull/1228/files#r14644507.

This comment has been minimized.

@probins

probins Jul 8, 2014
Contributor

what I meant was that proj4 begins with 'p' - it should be further up the list



/**
* The validity extent for the SRS.
* @type {ol.Extent|undefined}
* The forward transform function that takes a {@link ol.Coordinate} as argument

This comment has been minimized.

@probins

probins Jul 8, 2014
Contributor

just to make this clear, after 'function' I would add '(that is, from the first projection to the second projection)' or something like that

@probins
Copy link
Contributor

@probins probins commented Jul 9, 2014

one difference to note between using custom transforms and Proj4js is that the Proj4js code adds transforms between/among all supported projections, although you could fake that with custom by transforming via 4326

@ahocevar
Copy link
Member Author

@ahocevar ahocevar commented Jul 9, 2014

I think I addressed all comments now. The docs and tutorial improvements that @probins suggests above deserve a separate pull request in my opinion.

@elemoine
Copy link
Member

@elemoine elemoine commented Jul 9, 2014

Thanks for your effort @ahocevar. I'll have another look tomorrow.

@probins
Copy link
Contributor

@probins probins commented Jul 9, 2014

The docs and tutorial improvements that @probins suggests above deserve a separate pull request in my opinion.

yes, I wasn't meaning they needed to be addressed here

*/

// Convert WGS lat/long (° dec) to CH y
function WGStoCHy(lat, lng) {

This comment has been minimized.

@elemoine

elemoine Jul 10, 2014
Member

My editor's linter complains that these functions are used before they are defined. We could move them at the beginning of the file but this will make the example's code harder to read and understand. So let's keep them here. My eyes will bleed each time I open this file but I guess it's ok :-)

This comment has been minimized.

@ahocevar

ahocevar Jul 10, 2014
Author Member

You're right, and the reason why I decided to move the swisstopo code to the bottom was readability of the example code.

This comment has been minimized.

@elemoine

elemoine Jul 10, 2014
Member

Agree.

This comment has been minimized.

@calvinmetcalf

calvinmetcalf Jul 10, 2014

@elemoine set the "predef":"nofunc" option on your linter for this project (assuming it's jshint)

This comment has been minimized.

@elemoine

elemoine Jul 10, 2014
Member

I already did :-) Thanks.

This comment has been minimized.

@probins

probins Jul 10, 2014
Contributor

you could put this code in a separate file

This comment has been minimized.

@elemoine

elemoine Jul 10, 2014
Member

If the file is not compiled it will externs. We do that already for things that are in resources/example-behaviour.js.

But in this case I find it's good to have these function in the example's js file, because it gives more context to the reader, and thereby makes things easier to understand. So I'd rather keep them here.

CHtoWGSlng(coordinate[0], coordinate[1]),
CHtoWGSlat(coordinate[0], coordinate[1])
];
});

This comment has been minimized.

@elemoine

elemoine Jul 10, 2014
Member

Would you cherry-pick https://github.com/elemoine/ol3/commit/6524124a763a7185e948408f4a5b9cf65d786ff3 for more explanations on the use of a custom projection and custom transforms in this example?

@elemoine
Copy link
Member

@elemoine elemoine commented Jul 10, 2014

I've just added a comment suggesting to cherry-pick https://github.com/elemoine/ol3/commit/6524124a763a7185e948408f4a5b9cf65d786ff3 from my branch.

This looks really good and is a great improvement to the library. +1 for merging.

@probins
Copy link
Contributor

@probins probins commented Jul 10, 2014

This looks really good and is a great improvement to the library.

👍

@ahocevar
Copy link
Member Author

@ahocevar ahocevar commented Jul 10, 2014

Thanks for all the reviews, waiting for Travis now 😄.

ahocevar added a commit that referenced this pull request Jul 10, 2014
@ahocevar ahocevar merged commit 9640a08 into openlayers:master Jul 10, 2014
1 check passed
1 check passed
@ahocevar
continuous-integration/travis-ci The Travis CI build passed
Details
@ahocevar ahocevar deleted the ahocevar:new-proj4js branch Jul 10, 2014
@probins
Copy link
Contributor

@probins probins commented Jul 10, 2014

@ahocevar if you're not already doing something, I can work on improving the docs. Should have something for review by tomorrow

@ahocevar
Copy link
Member Author

@ahocevar ahocevar commented Jul 10, 2014

Please go ahead @probins, thanks!

projection = projections[code];
if (ol.ENABLE_PROJ4JS && !goog.isDef(projection) &&
goog.isFunction(proj4)) {
var def = proj4.defs(code);

This comment has been minimized.

@probins

probins Jul 10, 2014
Contributor

I think this is wrong; I think this should read ... proj4(code). proj4.defs(code) returns null

Not sure how I managed to miss this whilst testing yesterday :-(

This comment has been minimized.

@ahocevar

ahocevar Jul 10, 2014
Author Member

This is correct. We want the definition, not the WGS84->projection forward/inverse. And proj4.defs('unknown') returns undefined, not null.

This comment has been minimized.

@probins

probins Jul 10, 2014
Contributor

yeah, you're right. There's something else wrong in my app ...

This comment has been minimized.

@probins

probins Jul 10, 2014
Contributor

have to use proj4 2.2; I had 2.1 in there from last month, which doesn't work :-(

This comment has been minimized.

@ahocevar

ahocevar Jul 10, 2014
Author Member

Yes, this is expected. Maybe also a documentation task...

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

Successfully merging this pull request may close these issues.

None yet

8 participants