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

Auto-reset the zoom after zooming past a target. #4982

Merged
merged 3 commits into from
Feb 10, 2017
Merged

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Feb 10, 2017

Per suggestions from @duvifn and @kring in PR #4967. This is an enhancement to an already-merged fix for #4855.

This prevents the camera zoom from "locking up" by automatically selecting a new zoom start point. I tried this with the 3D model sandcastle demo, and it works well.

@emackey
Copy link
Contributor Author

emackey commented Feb 10, 2017

Fixes #4855 ("even more fixed")

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 10, 2017

@denverpierce @jsalankey @duvifn @smills2929 @keikland @kring are any of you available to test this today so we can mrge it during the bug bash?

@keikland
Copy link

Quick test, replaced the entire handleZoom() function with the new code, and gave it a shot. (Hope this was all code that needed changing for testing the zoom issue.) Unfortunately the result was :-(

  1. Zooming slowed to a crawl and then it was impossible to zoom out again.
  2. Polylines still disappear, suggesting the underlying problem is not solved.

No drop-through to the other side of the earth, though, but this medicine is probably worse than the illness....

@denverpierce
Copy link
Contributor

Pretty sure I broke my mouse wheel testing it, but the zoom seems perfect.

@denverpierce
Copy link
Contributor

@keikland
Copy link

Note that in my unsuccessful test of the attempted fix I used the v1.30 release code.

@emackey
Copy link
Contributor Author

emackey commented Feb 10, 2017

@keikland, polylines disappearing is a separate issue. You may be seeing an effect that makes them appear linked because the zoom is "picking" things, possibly picking polylines when they are visible, and perhaps missing your polylines when they have rendering problems. But the rendering problems of polylines are otherwise completely unrelated to the code that's being fixed here.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 10, 2017

Thanks for the prompt test @denverpierce @keikland!

@pjcozzi pjcozzi merged commit 4bcc68f into master Feb 10, 2017
@pjcozzi pjcozzi deleted the zoom-auto-reset branch February 10, 2017 15:59
@keikland
Copy link

Unfortunately I a simple guy not set up with Sandcastle and individual JS modules. Must therefore wait for a complete Cesium.js to do a real world test with my app.

Clearly the polylines issue I observe could be completely a different beast, but allow me the speculation and hypothesis ... even if correlation is the worst possible basis for determining causality.

@kring
Copy link
Member

kring commented Feb 10, 2017

Unfortunately I can't test this until Monday (your Sunday) but it's probably fine! I'll double check then.

@kring
Copy link
Member

kring commented Feb 10, 2017

Thanks for the additional fix @emackey!

@denverpierce
Copy link
Contributor

@keikland Compiled global Cesium.js for this branch is also found here: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/zoom-auto-reset/Build/Cesium/Cesium.js

@keikland
Copy link

@denverpierce Thanks! Tried the compiled global Cesium.js you provided with latest versions of Chrome, Opera and Firefox, loading my app. No errors in loading with any of them, but experienced the same problem with all three that I had earlier this evening, i.e being able to zoom in to a certain level (probably around 10km, then getting stuck there. Could not zoom further in or back out again. Interestingly, at that stuck zoom level I was stille able to use starburst and open an infobox. So Cesium seemed not to be locked in a loop, only in a stable "Newtonian minimum".

@duvifn
Copy link
Contributor

duvifn commented Feb 11, 2017

@emackey, thanks again!

Just in case there are still problems with those AMD cards, I made a little change in this branch.

I just copied @emackey's check to the end of this code section so there would be one more execution of this code section in case we zoomed past the target.
I didn't remove the check in the start because it's possible that previous execution of this function would take a different code path.

@emackey
Copy link
Contributor Author

emackey commented Feb 13, 2017

@duvifn that's a good change, it smooths out the one-frame hiccup that occurs when you zoom past a target. But I wonder if it's needed? It costs a (tiny) amount of performance for every frame of zoom to predict whether you're lined up for the bug on the next frame. I'm tempted to give that one a pass, unless the 1-frame hiccup is really troubling to some users. Any objection?

@keikland
Copy link

It seems that the additional change by @duvifn is quite robust per se. The amended fix still gives a zoom lockup in my app, well before getting to the target. To me this makes any potential new release with this merged a big gamble. It really should be tested on a wider basis.

I am troubled that the root cause seems not to have been identified, which to my mind simply is that Cesium gets confused (is not updated) about where ground is. Strangely, I am only aware that it happens when when a ground-clamped or near-ground entity is "in the way" of zooming.

I have had quite a bit of trouble getting Cesium to robustly returning correct ground altitude, but in this case it may actually be a diagnostic angle. I suggest that for a given known zoom problem location the computed altitude is logged to the console what the computed altitude is. If the zoom is allowed to drop though the earth while a reported altitude is above the ground, it could help the search. Conversely, if the altitude estimate goes wild, it may be correlated with the zoom problem too.

Just some thoughts.

(I am actually working on another issue right now in localizing/configuring the clock display and the rather independent, currently non-configurable/internationalizable timeline widget. I have created a modification that works for me, and I will probably revert with proposal for a PR here.)

@emackey
Copy link
Contributor Author

emackey commented Feb 13, 2017

The amended fix still gives a zoom lockup in my app

Can you post a code snippet to reproduce? What's in master now doesn't produce lockups in my testing.

To me this makes any potential new release with this merged a big gamble.

I respectfully disagree. Even if there are lockups somehow remaining, it's better than flinging to the other side of the planet.

It really should be tested on a wider basis.

Agreed. I think @kring is planning an additional test as time permits.

I am troubled that the root cause seems not to have been identified

The specific issue this targets, #4855, happens because the zoom went past whatever Cesium thought the target was. Whether or not the target was correctly computed in the first place is a separate issue requiring a separate fix, spelled out in #4368. It is possible to trigger #4855 without suffering #4368, and indeed that's what my test cases do, which is why I'm confident that #4855 is really fixed even while #4368 remains open.

@keikland
Copy link

I agree that zoom overshooting seems to be a separate issue. Right now I see zoom undershooting too, which the #4855 fix does not seem to tackle. My original vague hope was that a fix of #4368 would also fix #4855, and at least give that fix a safer ground, literally.

My app freezes with the fix, refusing also to unzoom. Unfortunately I have a big other deliverable this week, but I will certainly get to creating a code snippet. Hope it will turn out simpler to do than I fear.

@duvifn
Copy link
Contributor

duvifn commented Feb 14, 2017

@duvifn that's a good change, it smooths out the one-frame hiccup that occurs when you zoom past a target. But I wonder if it's needed?

@emackey I don't think this change is needed until we know that this is an issue.

I made it just in case this is still an issue on some machines. If scene.pickPosition returns a value that is very close to the camera we can move past the target every other call to this function, and this can make some trouble with the camera movement.

@keikland's comment looked to me like a possible evidence for that, so I put here the code to allow checking in case more users would report this (as @denverpierce wrote, the depth issue seems to be related to AMD users: 1, 2, 3?, 4?,but this still has to be proved).

@keikland
Copy link

It seems that Chromium v57.0.2987.+ has solved several obstinate problems I've seen in Cesium. Both Chrome 57 and just-released Opera 44 have this same engine, and the zooming past target/lockup and problem with disappearing polylines seem to have both been solved. Clearly appears to have been a WebGL implementation issue.

Chrome 54-56 and Opera 42-43 are problematic and Cesium users should be urged to upgrade to the latest version in my view.

The remaining problem I see on my platform is that near-polar tiles are randomly, but fairly consistently black or triangulated. I believe this is problem is being covered by some ongoing development work, though.

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.

6 participants