3dcitydb / 3dcitydb-web-map

Cesium-based 3D viewer and JavaScript API for the 3D City Database
Apache License 2.0
377 stars 125 forks source link

Replaced removed loading functions with Cesium.Resource (Required for Cesium1.44) #28

Closed Tugark closed 6 years ago

Tugark commented 6 years ago

Hi all

As mentioned in #27 , some changes regarding the loading of external resources are necessary to update to the most recent Cesium version (which is 1.44). In fact, it's not that much, but they are still breaking, since the old functions have been completely removed.

This PR includes those changes that were necessary and from my tests, the 3dwebclient viewer works as before. As you specified, I did not include the updated Cesium version. (Which is yet another thing I'd like to work on - as mentioned in #24 . Using node correctly, we could simplify deployments of the 3D Client, by not having to also distribute the Cesium libraries along with the code :-))

Please let me know should you find any issues, so I can update them.

Best, Lukas

Son-HNguyen commented 6 years ago

Dear Lukas,

thank you for the pull request!

I've just tested your codes and apparently the problem regarding the loading functions still persists (i.e. building thematic data stored in e.g. a Google Fusion table could not be retrieved due to bad queries, namely an empty GMLID exists in "SELECT * FROM table_id WHERE GMLID = "). I suspect there are more broken/deprecated functions than those updated by this pull request. I will definitely do some more tests on the change logs.

Did the loading functions from the Google Fusion table work on your machine?

Regards, Son

Tugark commented 6 years ago

Hi Son

Thanks for testing my code!

No, I did not test the Google Fusion tables, since I don't have any test data for this.

As you can see, in my PR, I've updated line 980 from

Cesium.loadJson(queryLink).then(function (data) {

to the new way by adding

new Cesium.Resource({ url: queryLink }).fetch({ responseType: 'json' }).then(function(data) {

The same fix also applies to loading the JSON files for adding KML layers. There, it works without issues.

From what I see in the code, this does not change anything in the behaviour of how the data is accessed. If the GMLID is empty, though, it could be that the issue lies deeper, i.e. that no GMLID can be retrieved. As such, the issue could rather be found in the geocoder widget, since the glmId is elicited there and supplied to zoomToObjectById, from where the fetchDaraFromGoogleFusionTable is fetched.

Do you have any easy means for me to also test this with google fusion tables?

I'll do some more tests and come back if I find something else.

Thanks and best regards, Lukas

Tugark commented 6 years ago

Hi Son

I think I might have fixed the issue. The problem was with the hardcoded queryLink that was supplied as of now. The fixes are commited in 291a7d78380e071bdb95f32ef8e400330673d876 and cec903543d38370b3fc54cf8de537c1425c29d08 on the branch.

Even though I do not have any "actual" data, I tried doing it with using the ThematicUrlLink from your NewYork example. Looking at my console, I get successful request to the Google Fusion Table, without any result (since I don't have the right GMLIDs ;-)).

Please let me know if this fixed the behaviour for you.

On another note, I've also noticed that you are using Cesium.jsonp(), e.g. on line 943 in script.js. Interestingly enough, this function has been removed since Cesium 1.17, so it should not have been working in your recent update to 1.35, right? Should this be fixed as well by adding a respective Resource call?

Best,

Lukas

thomashkolbe commented 6 years ago

Hi,

the 3D webclient project of our 3D city model of New York City uses Google Fusiontables to provide access to the thematic data. The configuration can be used to test the new version: http://www.gis.bgu.tum.de/projekte/new-york-city-3d/#c1799

Best regards Thomas Kolbe

Am 23.04.18 um 13:29 schrieb Tugark:

Hi Son

I think I might have fixed the issue. The problem was with the hardcoded queryLink that was supplied as of now. The fixes are commited in 291a7d7 https://github.com/3dcitydb/3dcitydb-web-map/commit/291a7d78380e071bdb95f32ef8e400330673d876 and cec9035 https://github.com/3dcitydb/3dcitydb-web-map/commit/cec903543d38370b3fc54cf8de537c1425c29d08 on the branch.

Even though I do not have any "actual" data, I tried doing it with using the ThematicUrlLink from your NewYork example. Looking at my console, I get successful request to the Google Fusion Table, without any result (since I don't have the right GMLIDs ;-)).

Please let me know if this fixed the behaviour for you.

On another note, I've also noticed that you are using |Cesium.jsonp()|, e.g. on line 943 in |script.js|. Interestingly enough, this function has been removed since Cesium 1.17, so it should not have been working in your recent update to 1.35, right? Should this be fixed as well by adding a respective Resource call?

Best,

Lukas

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/3dcitydb/3dcitydb-web-map/pull/28#issuecomment-383543246, or mute the thread https://github.com/notifications/unsubscribe-auth/AGiF1hPoGuP0hcyQRJWRdtIbT8k0Fdqjks5trbsPgaJpZM4TbvQU.

--


Prof. Dr. Thomas H. Kolbe Email: thomas.kolbe@tum.de Chair of Geoinformatics WWW : http://www.gis.bv.tum.de Faculty of Civil, Geo and Secretary: +49 89 289-22578 and Environmental Engineering Phone: +49 89 289-23888 Technische Universität München Fax : +49 89 289-22878 Arcisstraße 21
80333 München, Germany

Chairman of the Round Table Geoinformation Systems (Runder Tisch GIS e.V.) http://www.rundertischgis.de/

The Chair of Geoinformatics has won the award "Selected Landmark 2016" in the national contest "Germany - Land of Ideas" http://www.gis.bgu.tum.de/en/projects/sddi/

Son-HNguyen commented 6 years ago

Hi Lukas,

thank you for the commits. Yes it seems to be working now! So the problem is indeed the SQL query syntax used in the earlier version that is now deprecated in 1.44 using the new loading functions as you pointed out.

Regarding the Cesium.jsonp() function: this is mainly used for the Google Spreadsheet tables. Perhaps it can also be fixed using the Cesium.Resource.fetchJsonp(options) function.

I'll be testing the codes though if any bug is still there.

Best regards, Son

Tugark commented 6 years ago

Hi Son

Yes, it could be fixed by using Cesium.Resource.fetchJsonp(options), though you would also have to update the queries. I think the correct way would be to replace

var metaLink = 'https://spreadsheets.google.com/feeds/worksheets/' + spreadsheetKey + '/public/full?alt=json-in-script';

Cesium.jsonp(metaLink).then(function (meta) {

by this (since there are queryparameters yet again):

var metaLink = 'https://spreadsheets.google.com/feeds/worksheets/' + spreadsheetKey + '/public/full';

new Cesium.Resource.fetchJsonp({ ({ url: metaLink, queryParameters: {alt: 'json-in-script'} }) }).then(function(meta) {

(only for the first example, there are 3 nest jsonp calls)

However, since I don't have any means of testing this (i.e. I'm not sure if we would first have to create a Cesium.Resource and then call that resource object's fetchJsonp method; however, according to the docs, fetchJsonp() should create a resource!) and since I don't see if it is used anywhere in the code, I do not create a pull request.

I guess that function could also be removed from script.js?

Best,

Lukas

Son-HNguyen commented 6 years ago

Hi Lukas,

I merged your pull request into the 3dcitydb:update-cesium-1.44 branch. There we can test and experiment more.

Best, Son

Tugark commented 6 years ago

Hi Son

Thanks for merging it. I have not found any issues since; the only issue I found was the Credit links which have been fixed with your recent merge from the 1.41 branch.

Do you have any specific tests that you'd like me to perform, so we can move on with updating it?

By the way, I'm still working on a webpack version or rather, an npm version. It's not that easy, but it might work. As such, we would also get rid of having to include the Cesium build, since an easy npm install 3dcitydb would do the trick. The main issue is the "missing" documentation of the JS code - do you plan on creating something, or should I work on that as well? :-)

Once again, thanks and best regards,

Lukas

Son-HNguyen commented 6 years ago

Hi Lukas,

thanks for the help. To ensure we don't overlook anything, I'm looking at the breaking changes and deprecated functions listed in the change logs from Cesium 1.41 up to 1.44. For example, since 1.42 the clock does not animate by default. Cesium 1.43 added some broken changes (again) to the credits. Perhaps you could take care of the clock, while I'm fixing the credits. I suppose it should not be too time-consuming.

As Zhihang said in https://github.com/3dcitydb/3dcitydb-web-map/issues/24#issuecomment-370224026, we currently do not have any plans on using Webpack. But we'd love to see your progress on this.

Which "missing" documentation do you mean? The documentation for the source codes of the 3DCityDB-Web-Map-Client?

Also: I guess you an employee of geoProRegio AG in Switzerland (according to your profile)?

Best, Son