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 billboard and label clamping #4493

Merged
merged 3 commits into from
Oct 24, 2016
Merged

Fix billboard and label clamping #4493

merged 3 commits into from
Oct 24, 2016

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Oct 21, 2016

Turns out that billboard and label clamping were fundamentally broken because the QuadtreePrimitive was processing the tile queue in the wrong order. It was always pulling new tiles from the back of the array rather than the front, which meant that data would get processed in the wrong order causing old tiles to take precedence over newer tiles.

Additionally, there was a bad if block in Label.js which caused the initial position of the individual label billboards to not be properly set when clamping was on, instead we should always set the positions before calling _updateClamping (if needed).

Fixes #4396 and #4062

I already talked to @bagnell about this and he agreed it was a straight up bug. I would love to add unit tests for these, but I'm not sure what the best strategy would be.

There are still several other clamping related issues that I'm looking into; but I wanted to keep these PRs small for easier review.

Turns out that billboard and label clamping were fundamentally broken
because the `QuadtreePrimitive` was processing the tile queue in the wrong
order.  It was always pulling new tiles from the back of the array rather
than the front, which meant that data would get processed in the wrong
order causing old tiles to take precedence over newer tiles.

Addtiionally, there was a bad if block in `Label.js` which caused the
initial position of the individual label billboards to not be properly set
when clamping was on, instead we should always set the positions before
calling `_updateClamping` (if needed).

Fixes #4396 and #4062
@hpinkos
Copy link
Contributor

hpinkos commented Oct 21, 2016

Am i doing something wrong? #4026 doesn't seem to be fixed. The billboards look all over the place to me.

var viewer = new Cesium.Viewer('cesiumContainer', {
    baseLayerPicker: false,
    terrainProvider: new Cesium.CesiumTerrainProvider({
        url : 'https://assets.agi.com/stk-terrain/world',
        requestWaterMask : true,
        requestVertexNormals : true
    })
});
var options = {
    camera : viewer.scene.camera,
    canvas : viewer.scene.canvas,
    clampToGround: true
};
viewer.dataSources.add(Cesium.KmlDataSource.load('../../SampleData/kml/facilities/facilities.kml', options));

@hpinkos
Copy link
Contributor

hpinkos commented Oct 21, 2016

image

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2016

Works for me, are you waiting long enough, it can be slow?

@denverpierce
Copy link
Contributor

I'm not seeing the floaters anymore, but clamping perf appears a lot worse on this, because billboard and labels are dropping below the terrain before jumping back up.

I tried to get a gif of it, but it ended up taking so long to represent it, the file ends up larger than 10mb.

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2016

I'm not seeing the floaters anymore, but clamping perf appears a lot worse on this, because billboard and labels are dropping below the terrain before jumping back up.

I tried to get a gif of it, but it ended up taking so long to represent it, the file ends up larger than 10mb.

Yes, performance appears a lot worse because it is now more correct and doing thing in the proper order. The next step is to fix the actual performance issues but that will happen in future PRs.

There's also still some clamp related bugs that need to be addressed independently, but this is definitely an important incremental improvements.

@hpinkos
Copy link
Contributor

hpinkos commented Oct 21, 2016

@denverpierce what seems to be happening for me is that clamping works for a little bit, then as i zoom in and out and click around to target different billboards it seems to stop working after a little bit.

@pjcozzi can you review and merge this?

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2016

I have another change that I'm going to PR into this branch because it may be a bit controversial.

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2016

I opened #4497, can people please try that out and let me know what you think.

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2016

Since #4497 has been retargeted, this is ready unless there are other comments.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 21, 2016

Will look at this shortly.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 21, 2016

For #4062, the labels didn't clamp to terrain for me:
image

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2016

Did you turn terrain on after you loaded the KML? If so they won't clamp because that's fixed with #4497

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2016

Actually, I think changing terrain is yet another unrelated clamping bug, I'm looking at that separately now.

@denverpierce
Copy link
Contributor

Works for me, but when you switch terrain, depth testing comes on, and due to #4090 KML billboards are centered, so they occlude a bit.

Facilities isn't a great example, but here's how it looks when you turn on terrain with clamping in this branch:
ss

@mramato
Copy link
Contributor Author

mramato commented Oct 24, 2016

Works for me, but when you switch terrain, depth testing comes on, and due to #4090 KML billboards are centered, so they occlude a bit.

Thanks for testing @denverpierce. I actually think this is a bug in the facilities.kml file itself, but I have to check. Also, did you turn terrain on after the fact? I think there is a separate bug where terrain needs to be turned on before the data is added but I'm not sure.

@bagnell
Copy link
Contributor

bagnell commented Oct 24, 2016

This fixes #4396 and the misplaced glyphs of a label, but it does not fix #4062 completely.

@bagnell bagnell merged commit c4449e2 into master Oct 24, 2016
@bagnell bagnell deleted the clamp-champ branch October 24, 2016 20:44
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.

5 participants