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

Fix viewer.zoomTo and viewer.flyTo imagery when using terrain. #6895

Merged
merged 3 commits into from
Aug 8, 2018

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Aug 8, 2018

Realized Viewer had the same problem as Geocoder when zooming to imagery extents. This PR mostly moves code around.

  1. Pull computeFlyToLocationForRectangle out of Geocoder and make it a standalone private function. Add tests for it too (can't believe you guys let me get away with no tests in my other PR) 😮 .
  2. Have Viewer call the function when zooming or flying to terrain.

Part of #5942

1. Pull `computeFlyToLocationForRectangle` out of Geocoder and make it
a standalone private function. Add tests for it too.
2. Have Viewer call the function when zooming or flying to terrain.
@cesium-concierge
Copy link

Thanks for the pull request @mramato!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@mramato mramato requested a review from hpinkos August 8, 2018 00:53
@hpinkos
Copy link
Contributor

hpinkos commented Aug 8, 2018

Do you think it'd be fairly easy to fix this now too? #4327

@hpinkos
Copy link
Contributor

hpinkos commented Aug 8, 2018

This all looks good to me @mramato, thanks!

@hpinkos hpinkos merged commit 79d7443 into master Aug 8, 2018
@hpinkos hpinkos deleted the imagery-layer-terrain branch August 8, 2018 14:34
@mramato
Copy link
Contributor Author

mramato commented Aug 8, 2018

Do you think it'd be fairly easy to fix this now too? #4327

Maybe, but it's not as straightforward. The problem there is that we don't know we are zooming to the billboard, the entity could have some stuff on the ground and some stuff not. We might be able to do something though, I'll add some thoughts to the issue.

@keikland
Copy link

keikland commented Sep 5, 2018

Thanks for v1.49, with many appreciated bug fixes and enhancements. Preliminary testing exposed an issue with the geocoder fly-to functionality though, with Cesium failed to render immediately into flight (default function). See screen shot for error message.

2018-09-05_23-15-57

Some initial testing exposed that this is only a problem with the packaged browser minified Cesium js. I manually uglified/minified the unminified version on the Skalman site, and found no problems.

I'll see if I can probe further into this, but having to work with the minified code makes it very difficult. Could there be another issue similar to the Cesium base directory previously?

Kjell

@hpinkos
Copy link
Contributor

hpinkos commented Sep 6, 2018

@keikland we have seen crashes like that from time to time before, but we don't have any issues open currently. Can you please open a new issue with steps for reproducing the error? It's weird that it only happens with the minified version.

@keikland
Copy link

keikland commented Sep 6, 2018

Hi @hpinkos . i get the error in a rather large app and will need to boil it down to a minimal version to reproduce the problem for you. Need to figure that out. The special part of the setup is that geocoding is implemented as a bespoke feature with several sources and added "manually" during startup. The code follows the the Cesium default implementation pretty much to the letter, but has some additional controls. to get around some Cesium issues. Here it is, and I'll revert with a new issue and a boiled-down version that hopefully reproduces.

// Define function to initialize the search feature (called from xxx.js onload.)
function addGeocoder(viewer, geocoderServer) {
// Create and insert the search box container
var geocoderContainer = document.createElement('div');
geocoderContainer.className = 'cesium-viewer-geocoderContainer';
$('div.'+defaultCesiumToolbarClass).prepend(geocoderContainer);
var geocoder = new Cesium.Geocoder({
container: geocoderContainer,
geocoderServices: (Cesium.isArray(geocoderServer) ? geocoderServer : [geocoderServer]),
scene: viewer._cesiumWidget.scene
});

// Subscribe to search + flight completion
geocoder._viewModel._complete.addEventListener(function() { 
  	if(viewer._geocoder._viewModel.isSearchInProgress) {  // Cesium bug? isSearchInProgress does not get reset when we use keys to select
		viewer._geocoder._viewModel._searchCommand();
  	}
  	$('div.cesium-widget').focus();  // Give focus back to Cesium so that mouse clicks will work there
	getMapCenter(currentpos);
	updateScene();
});

// Subscribe to search so that we can clear the trackedEntity when it is clicked.
viewer._eventHelper.add(geocoder.viewModel.search.beforeExecute, Cesium.Viewer.prototype._clearObjects, this);

// Make geocoder search a part of the Cesium viewer object.
viewer._geocoder = geocoder;
return geocoder;

}

@keikland
Copy link

Hi @hpinkos . Reverted to the standard minified Cesium.js and stepped through my startup code to check if timing could be an issue, and things worked. I then ran without any breaks, and things worked too. This can only mean that dynamically changing external content loaded at startup is to blame, and I suspect rendering of complex KML-based NOAA data (hurricanes and ice). It would also indicate that it has nothing to do with changes in v1.49! I'll need to wait for the problem to return for me to further zoom in on the problem.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 11, 2018

Thanks for the update @keikland! Let me know if there's anything we can do to help =)

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

Successfully merging this pull request may close these issues.

4 participants