-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Proj4JS and Custom Projections in Columbus View #6986
Conversation
Thanks for the pull request @likangning93!
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Getting an |
This sounds vaguely familiar. I think you are right and that it's directly related to the data uri format. Can you try with an external js file and see if it works? Once we are sue it's the data uri, we might be able to tweak things to get it to work on all browsers (without resulting to an external file). |
Seems to work fine with an external file, here on deployed Sandcastle. |
@shunter do you remember something about data uris being cross origin on IE 11 or anything odd about data uris in IE11 in general? |
The data URL in the example is faulty because it contains spaces and newlines, etc. The result is that it bypasses the data URL detection in Resource.js. URLs must be encoded correctly. And yes, data URLs are not same-origin with anything in some browsers when loaded via XHR, which is why I added that special detection and handling in #1533 in the first place. |
Cool thanks, shame on my for not looking at the sample code better. |
43131e1
to
187963c
Compare
187963c
to
358308b
Compare
or, shame on me for not knowing how to URI. |
ef00d04
to
66c0646
Compare
and
Please scope this accordingly based on customer interest, e.g., it is OK to not support these or not support them well if initial customers are OK with it. Then roadmap them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I mentioned this in another PR but proj4.js is 169 KB. What does it gzip to? When does Cesium load it, e.g., only when requested with a web worker? Are we OK with the size?
- I did not get a chance to run this. Please please please profile representative cases.
} | ||
|
||
// Add lat/long points | ||
for (var x = -175; x < 180; x += 10) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but would call these locals lon
and lat
since that is what they are.
|
||
var projectionText = | ||
'function projectionFactory(callback) {\n' + | ||
' function project(longitude, latitude, height, result) {\n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below have to use scalars and arrays due to the web workers, right? They can't use Cesium's cartesian and cartographic types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not due to web workers, I just think it's more flexible to use something built into the language when interfacing. Users might pull in other vector libraries, so imposing Cesium types didn't seem appropriate. It might even create confusion, like give an impression that all Cesium types are accessible from the callback out-of-the-box (they aren't).
I also wonder if that could mess with callbacks written in Typescript, but I don't know enough about that to say for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno - we use Cesium types throughout Cesium for interfaces that we implement and/or interfaces our users implement. Seems arbitrary to make this different.
} | ||
|
||
// Add lat/long points | ||
for (var x = -175; x < 180; x += 10) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment. Can also remove the code comment on the line above.
Source/Core/CustomProjection.js
Outdated
CustomProjection.prototype.unproject = function(cartesian, result) { | ||
//>>includeStart('debug', pragmas.debug); | ||
if (!this._ready) { | ||
throw new DeveloperError('CustomProjection is not loaded. User CustomProjection.readyPromise or waith for CustomProjection.ready to be true.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waith
"wait?"
Source/Core/CustomProjection.js
Outdated
var deferred = when.defer(); | ||
var buildPlugin; | ||
(function() { | ||
eval(scriptText + 'buildPlugin = ' + functionName + ';'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1) we're sure about this? and (2) this didn't require a jsHint workaround?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't require a jsHint workaround, and our eslint doesn't disallow eval because it was "likely never to come up".
This could use more thought though. @mramato, @shunter, can you guys weigh in a bit here?
Just as a reminder, the goal is for users to provide their own functions for project
and unproject
, and they may be pulling in other libraries to implement those. project
and unproject
also have to function on web workers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still confused why they can't just implement a class with these functions.
Also, check out three.js for its delayed module loading architecture. Could be useful here and at a potentially larger scale in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, we use Cesium in at least one app with a content security policy that doesn't allow use of eval
, and I suspect a lot of others do as well. If the automatic reprojection breaks in that environment, it's not a disaster (for us, cause we already have a reprojection mechanism), but if Cesium fails to load because if it, that's bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you maybe just want to use JSONP or something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequireJS allows loading files after page load: https://requirejs.org/docs/api.html#afterload
I have this proof-of-concepted, but it needs another set of eyes from someone who knows more about how module systems work and how this will interact with built Cesium, @mramato are you the right person to ask?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @kring I'm going to give jsonp a try as you originally recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RequireJS route doesn't work for built Cesium.
JSONP works after some possible Resource
bugfixes but I don't have it working on web workers yet because of the dependence on window
. Maybe we can work around that using importScripts
somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really the reliance on Window that's the problem, it's the fact that document
isn't available. Ideally it would be nice to fix Resource so that it works in all cases (this could even be done in a separate PR for easier testing/review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might need a lot more guidance to overhaul Resource
, so just to keep things moving I've updated for now with JSONP and the importScripts
workaround. This doesn't work with data URIs on IE11, but that doesn't seem like a huge deal. I updated Sandcastle demo to load the same "custom projection" from a file when IE11 is detected.
@kring thanks for the JSONP suggestion, sorry I ever doubted you!
Source/Core/Proj4Projection.js
Outdated
} | ||
}); | ||
|
||
var projectionArray = [0, 0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly include "scratch" in the name.
Source/Core/Proj4Projection.js
Outdated
} | ||
|
||
// without clamp proj4 might crash | ||
projectionArray[0] = CesiumMath.clamp(CesiumMath.toDegrees(cartographic.longitude), -180 + CesiumMath.EPSILON7, 180 - CesiumMath.EPSILON7); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, include .0
when the value is intended to be floating point.
Source/Core/Proj4Projection.js
Outdated
} catch(e) { | ||
if (!this._forwardFailed) { | ||
// Log a warning the first time a projection fails | ||
console.warn('proj4js forward failed for ' + projectionArray + ' with projection ' + this._wkt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, are we sure console.warn
is the right approach compared to throwing an exception? How often is console.warn
used elsewhere in Cesium - and for what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately Proj4js frequently fails when unprojecting positions in EPSG:2039, even when they appear to be in-bounds. It may also fail for other projections if camera navigation moves outside bounds, which we must assume will happen for Columbus View.
I'll change this to oneTimeWarning
, which we use in other places.
if (serializedMapProjection.isMercator) { | ||
projection = new WebMercatorProjection(ellipsoid); | ||
} | ||
if (serializedMapProjection.isGeographic) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else
?
Can both isMercator
and isGeographic
be true at the same time? Perhaps a local enum is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're false at the same time for Custom
and Proj4js
projections. I'll switch to an enum though, that makes sense.
Source/Core/TerrainEncoding.js
Outdated
* The terrain mesh should contain projected positions for 2D space. | ||
* @type {Boolean} | ||
*/ | ||
this.has2dPositions = defined(center2D); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming convention is not consistent with center2D
, which is not consistent with Cesium's use of camelCase. Maybe this is a special case. I dunno. @lilleyse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll switch to hasPositions2D
6f80b53
to
0d2d47a
Compare
Still need to answer these questions, but technically we could remove built-in Proj4js support and just demonstrate in Sandcastle or via a blog post how users who want EPSGs and whatnot can wire Proj4js up via |
I think we should be ok on size, it's smaller than our Also I don't think we can get it to load on request like Draco, it has to be available on the main thread at app startup. Perhaps when we overhaul our module system. |
Doesn't sound great, what % increase is that for Cesium.js? To benefit what % of CesiumJS users? Maybe cleaning it up can just go on the roadmap, but I don't think we can rationale a size increase like this as best practice. |
Did you look at the three.js module / runtime loading system? |
@pjcozzi do you happen to have a reference handy for this? I found: |
@donmccurdy any chance you could point us to three.js' module system? |
three.js uses less of a system than a pattern — the core library is small (mainly just the parts that everything else depends on) and features that can be extracted are generally left for the user to include (or not) in their build. Simple example using <script src="node_modules/three/build/three.min.js"></script>
<script src="node_modules/three/examples/js/loaders/GLTFLoader.js"></script> We have ways to make this work nicely with CommonJS modules, ES6 modules, and NPM distribution, as well. In certain cases the core library will log warnings or throw errors to let users know a necessary module is missing, but generally the core library does not depend on anything else. Not sure if this answers your question, or if I've misunderstood the comments above. I can only think of one case where we do runtime script loading; in general that is not very compatible with npm distribution and build tools. TurfJS (https://github.com/Turfjs/turf) has a nice system, too — see their |
Red indicates region of the polygon that has "rectangle fragment culling," added in #6393 |
This is actually a pretty significant problem, the materials-on-ground-primitives code for 2D assumes an equirectangular projection for everything which is completely untrue here and would have been wrong at large scales for scenes using the Basically, materials on However, non-equirectangular projections will need stretched or curved planes for this to continue working properly. Yikes! [EDIT] a couple options: Another solution is to redesign Option 3 is to say that materials on |
The documentation does specify that materials on |
…ojection problems with camera
I spent some time prodding 2D/CV camera stuff, added a limiter to A few somewhat late observations about infinite scrolling in 2D:
Funny thing, right now infinite scroll will "work" for something like mollweide or Mercator limited to @bagnell the dot-product check you proposed will probably work for catching most cases, but maybe instead should we just default to |
But alas, zooming in and out of a polar projection in scene mode 2D, with rotation enabled, leads to wild rotation during the zoom. Here on Sandcastle. That's not good. |
…nd fix camera heading controls
I think this is fixed.
Mostly fixed, biggest problem was that a lot of the Also made this Sandcastle to demonstrate that setting
Mostly fixed, using larger bounding rectangles. Texture coordinates won't curve though, so the same polygon on the globe or in Geographic 2D/CV will look quite different vs. in mollweide or polar. |
I used to run into this a lot with EPSG 2039, but I can't reproduce it anymore in this branch. Maybe it went away in Proj4 2.5.0. Here's a Sandcastle that used to run into failures pretty consistently, though. |
For now let's say that we won't support projections with weird splits or multiple splits. Most "local area" EPSGs are split-free in their defined region anyway, and I think users who want, say, Dymaxion projections and stuff should have enough flexibility in
I kind of want to try this now... |
@bagnell sorry for the delayed response, but I think we're ready for another look. |
@bagnell we're also going to shift goals so this will become a base branch for "projection support" in general. Still experimenting with imagery too. |
@bagnell can you please review this? |
The only comment I have is when zooming in 2D and the heading changes. Instead, can you keep the current up vector. Otherwise, this looks good to me. |
@bagnell should be fixed! |
This looks good to me. Have you done any performance tests? |
Knew I was missing something... |
PerformanceDisclaimer that all numbers are kind of unscientific, but TL;DR performance doesn't seem to be impacted very much. All projections used mollweide ESRI:53009 FPSI wanted to see if the modified shader path/vertex attributes would impact runtime performance, but this is hard to see on most modern, mid-range computers. So I ran on a very low-power Intel machine on hand, and tested web mercator via Proj4 vs. Cesium's built-in Web Mercator, using Natural Earth in both cases. Testing was done at 1366x768. Here's what the code looked like. These numbers were the same for both in "home" views in different scenemodes:
Tile projection timeI created a branch here: https://github.com/likangning93/cesium/tree/additionalProjectionsPerf Celeron N2920:
i7 4980HQ:
|
This PR adds support for Proj4js projections and user-defined projections in 2D and Columbus View.
This still needs a lot of testing - opening a PR for early feedback on the general approach before going much further.
Known Problems/Open Questions
* [ ] projection/unprojection sometimes fails, currently caught and logged the first time, then silentno longer reproducible thanks to proj4js improvements?* [ ] wraparound in 2D broken for many projectionsdefault to rotatable 2D for proj4/custom projections* [ ] What to do about projections that split the globe somewhere other than the antimeridian?shouldn't be a problem in most cases* [ ] workers currently constantly spinning up new Projection objects, where is a good place to "cache" the projection?TODO
* [ ] assess when proj4js gets loaded, how well it gzips - [comment](#6986 (comment))Pretty Pictures
mollweide
Mt. St. Helens on mollweide
EPSG 3411 (polar)
Nonsensical custom projection