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

Clamping billboards to terrain - inaccurate positioning #4598

Closed
duvifn opened this issue Nov 3, 2016 · 3 comments · Fixed by #4622
Closed

Clamping billboards to terrain - inaccurate positioning #4598

duvifn opened this issue Nov 3, 2016 · 3 comments · Fixed by #4622

Comments

@duvifn
Copy link
Contributor

duvifn commented Nov 3, 2016

Hi,
Recently I encountered a problem with clamping billborads to terrain.
Clamped billboards are appearing with an offset of few meters (not a fixed offset).

In order to see this problem you can enter the following sandcastle example.
When you click toggle terrain button you will see the billboard (on the terrain). Click it again (turn the terrain off) and you will see it with some offset.

Sandcastle:

http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Hello%20World.html&label=Showcases&gist=9a739769ae5f781db3ad3eed90c7cdb8

(I had to add the billboard again in the code since billboards are disappearing when terrain is toggled. Is it a known problem or should I open another issue for it?).

1
2

I looked at the code and found a point that could cause this problem, but since I am not experienced in this field I want you to confirm it.

Cesium uses geocentric normal when it calculates the (wrong) updated position.
You can see it here

However when I use geodetic surface normal in this calculation the result is acccuarte.

The following sandcastle example illustrates the difference between the two methods.
Run it and wait until tiles are fully loaded.
Then click 'Place Billboards On Terrain' button.

Three billboards should appear on the map:
Yellow billboard position is calculated with the current method.
Red billboard position is calculated with the GeodeticSurfaceNormal.
The green billboard is just a conversion of the original cartographic, with current terrain height, to cartesian.

The green billboard (cartographic to cartesian) and the red (GeodeticSurfaceNormal) should appear at the same position, so I made a pixel offset to the green billboard.

You can pick a billboard to see by which method it was created.

Sandcastle

http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Hello%20World.html&label=Showcases&gist=8caeac6aa77715993dff4bf20addc806

3

You may wonder why the third result's height (green- cartographich to cartesian) is not exactly the same as the geodeticSurfaceNormal (red) height.
A quick look at the code in Globe.getHeight shows that it suffers from the same problem - it uses geocentric normal rather than surface normal.
As a result, the intersection point from which it extracts the height is different and hence the wrong result.

You can see the fixed code here ,
and here .
I can do a pull request for that if you think it's correct...

@hpinkos
Copy link
Contributor

hpinkos commented Nov 3, 2016

Wow, thanks @duvifn for digging into this and providing the detailed explanation and the code examples!
I think using a geodetic is the right thing to do here, but I'm not as familiar with this part of the code base.

@bagnell does this solution seem right to you?

@duvifn
Copy link
Contributor Author

duvifn commented Nov 7, 2016

A quick look at the code in Globe.getHeight shows that it suffers from the same problem - it uses geocentric normal rather than surface normal.
As a result, the intersection point from which it extracts the height is different and hence the wrong result.

Probably this is the reason to #3411

@hpinkos
Copy link
Contributor

hpinkos commented Nov 7, 2016

Thanks @duvifn! I talked to @bagnell offline and he agreed that this change sounded correct. If you wouldn't mind putting together a pull request we would be happy to review it. You can see our contributing documentation here: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/CONTRIBUTING.md

Thanks again!

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