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

Wrong camera position in photogrammetry Sandcastle example #6962

Closed
bagnell opened this issue Aug 27, 2018 · 15 comments · Fixed by #6989
Closed

Wrong camera position in photogrammetry Sandcastle example #6962

bagnell opened this issue Aug 27, 2018 · 15 comments · Fixed by #6989

Comments

@bagnell
Copy link
Contributor

bagnell commented Aug 27, 2018

The camera view starts at the photogrammetry and then moves upward after the terrain is loaded. This is a regression in the 1.48 release.

Expected behavior in 1.47:
https://cesiumjs.org/releases/1.47/Apps/Sandcastle/index.html?src=3D%20Tiles%20Photogrammetry.html&label=3D%20Tiles

In 1.48:
https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=3D%20Tiles%20Photogrammetry.html

@hpinkos
Copy link
Contributor

hpinkos commented Aug 27, 2018

Did you check out the behavior in master? @mramato made some improvements to viewer.flyTo for the 1.49 release #6895

@mramato
Copy link
Contributor

mramato commented Aug 27, 2018

Still happens in master, but this is definitely terrain related since that's the only difference in the demo between 1.47 and 1.48

@mramato
Copy link
Contributor

mramato commented Aug 27, 2018

That being said, I'm not sure why my change wouldn't have fixed this.

@hpinkos
Copy link
Contributor

hpinkos commented Aug 28, 2018

Your change to viewer.flyTo was only for flying to imagery. I forgot it wasn't for everything

@mramato
Copy link
Contributor

mramato commented Aug 28, 2018

Good point. But now I'm not sure why this bugs exists at all, unless the bounding sphere for the tileset is not taking the transform into account?

@hpinkos
Copy link
Contributor

hpinkos commented Aug 28, 2018

I'm sure it's whatever code we have that pushes the camera up above terrain when the terrain loads in that's causing the problem

@mramato
Copy link
Contributor

mramato commented Aug 28, 2018

Possibly, I thought it only pushed if the camera was actively under terrain though, but that's a good place to start.

@hpinkos
Copy link
Contributor

hpinkos commented Aug 28, 2018

It's not a problem with assets.agi.com terrain

var viewer = new Cesium.Viewer('cesiumContainer', {
    terrainProvider: new Cesium.CesiumTerrainProvider({
        url : 'https://assets.agi.com/stk-terrain/v1/tilesets/world/tiles',
        requestVertexNormals : true
    })
});

var tileset = new Cesium.Cesium3DTileset({
    url: Cesium.IonResource.fromAssetId(5712)
});

viewer.scene.primitives.add(tileset);
viewer.zoomTo(tileset);

@mramato
Copy link
Contributor

mramato commented Aug 31, 2018

@hpinkos I can reproduce this problem is all kinds of cases, not just the example. I think the fact that it doesn't happen with assets.agi.com is a red herring do to the difference between terrain data. For example, I have a tileset in New Zealand that this happens with.

@mramato
Copy link
Contributor

mramato commented Aug 31, 2018

It only happens the first time you zoom, zooming a second time works fine. I believe this may be some sort of race condition we recently introduced:

var viewer = new Cesium.Viewer('cesiumContainer', {
    terrainProvider: Cesium.createWorldTerrain()
});

var tileset = new Cesium.Cesium3DTileset({
    url: Cesium.IonResource.fromAssetId(5712)
});

viewer.scene.primitives.add(tileset);
viewer.zoomTo(tileset);

Sandcastle.addToolbarButton('Zoom again', function() {
    viewer.zoomTo(tileset);
});

@mramato
Copy link
Contributor

mramato commented Aug 31, 2018

I marked this next release because it's started showing up often for me.

@hpinkos
Copy link
Contributor

hpinkos commented Aug 31, 2018

I think the fact that it doesn't happen with assets.agi.com is a red herring. I believe this may be some sort of race condition we recently introduced

Well, the differences in data are at least exposing an existing problem. It's our adjustHeightForTerrain function that's moving the camera, and that code hasn't changed in a long time. If you comment out this line the problem doesn't happen: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Camera.js#L961

The globe renders with a lower resolution tile that has a greater height in a particular area and adjustHeightForTerrain moves the camera up at the end of the frame. Then a few frames later the higher resolution tiles comes in but the camera has already been moved up. I don't know that I'd call it a race condition, but maybe it is because it depends on the timing of when higher resolution terrain tiles are loaded in.

@mramato
Copy link
Contributor

mramato commented Aug 31, 2018

Okay, I think we're getting close to the root cause here. I don't think adjustHeightForTerrain should get called unless terrain is actually finished loading. (At least that was the original intent)

mramato added a commit that referenced this issue Aug 31, 2018
Fixes #6962

I'm not sure if we introduced a regression somewhere or if we just never
noticed, but the desired camera behavior is that it not force the view to
be above terrain if there is still terrain loading.  I also confirmed
that #4075 is still fixed even with this change.
@mramato
Copy link
Contributor

mramato commented Aug 31, 2018

Submitted #6989

@mramato
Copy link
Contributor

mramato commented Aug 31, 2018

And thanks @hpinkos for doing most of the hard work in narrowing this down.

jbo023 pushed a commit to virtualcitySYSTEMS/cesium that referenced this issue Sep 4, 2018
Fixes CesiumGS#6962

I'm not sure if we introduced a regression somewhere or if we just never
noticed, but the desired camera behavior is that it not force the view to
be above terrain if there is still terrain loading.  I also confirmed
that CesiumGS#4075 is still fixed even with this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants