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

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

Merged
merged 3 commits into from
Apr 25, 2018

Conversation

Tugark
Copy link
Contributor

@Tugark Tugark commented Apr 19, 2018

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
Copy link
Member

Son-HNguyen commented Apr 23, 2018

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
Copy link
Contributor Author

Tugark commented Apr 23, 2018

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
Copy link
Contributor Author

Tugark commented Apr 23, 2018

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 and cec9035 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
Copy link
Member

thomashkolbe commented Apr 23, 2018 via email

@Son-HNguyen
Copy link
Member

Son-HNguyen commented Apr 23, 2018

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
Copy link
Contributor Author

Tugark commented Apr 24, 2018

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 Son-HNguyen changed the base branch from master to update-cesium-1.44 April 25, 2018 07:24
@Son-HNguyen Son-HNguyen merged commit 74a7ec2 into 3dcitydb:update-cesium-1.44 Apr 25, 2018
@Son-HNguyen
Copy link
Member

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 Tugark deleted the update-cesium-1.44 branch April 25, 2018 09:13
@Tugark Tugark restored the update-cesium-1.44 branch April 25, 2018 09:13
@Tugark
Copy link
Contributor Author

Tugark commented May 2, 2018

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
Copy link
Member

Son-HNguyen commented May 2, 2018

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 #24 (comment), 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

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

Successfully merging this pull request may close these issues.

None yet

3 participants