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

Billboards on terrain - some problems #4686

Open
duvifn opened this issue Nov 27, 2016 · 4 comments
Open

Billboards on terrain - some problems #4686

duvifn opened this issue Nov 27, 2016 · 4 comments

Comments

@duvifn
Copy link
Contributor

duvifn commented Nov 27, 2016

Current implementation of billboard on terrain is well written and I have learned a lot from reading it.
However I found some problems while working on #4622:

  1. I think the billboard height should be computed also if the level of the scene getting lower (zoom-out). Currently the height is computed only in one direction. ( if (tile.level > data.level) in this link).
    However, sometimes lower levels have higher terrain at a given point so the billboard disappears.

  2. In the following code the isChildAvailable is invoked on the grandparent of the child, instead of on tile

var parentTile = tile.parent;
if ((defined(tileDataAvailable) && !tileDataAvailable) ||
    (defined(parentTile) && defined(parentTile.data) && defined(parentTile.data.terrainData) &&
    !parentTile.data.terrainData.isChildAvailable(parentTile.x, parentTile.y, child.x, child.y))) {
              data.removeFunc();
     }

(link).

  1. If the tile is upsampled the data.terrainData.isChildAvailable in the lines above would always return false (see this line). So, in such case the billboard wouldn't be updated after some zooming.

I found that this is the reason to this behavior (the billboard should be clamped to the ground):

billboard_on_terrain_hovering

  1. I'm not sure at this point but I think the billboard height must be recomputed if the original data of the tile was a result of an upsampling process, and the tile was later loaded.

  2. Please consider depth offset in the BillboardCollectionVS.glsl. In previous versions there were the following lines:

#ifdef CLAMPED_TO_GROUND
    // move slightly closer to camera to avoid depth issues.
    positionEC.z *= 0.995;
    

It would be great if, in addition to eyeOffset paramter in billboard and label, there will be a depthOffset which will be expressed as a fraction of the camera distance to the vertex (0-1).

  1. Currently if the terrain provider is replaced by another one, clamped billboards would disappear.
    Maybe that because the QuadtreePrimitive.prototype.invalidateAllTiles treat only levelZeroTiles's customData but I didn't enter into it
@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 28, 2016

Thanks for all the input, @duvifn!

@bagnell can you please review these?

@mramato
Copy link
Contributor

mramato commented Jul 26, 2018

I think we might have addressed all of these issues at one time or another and with @hpinkos' work in this are might make this entire issue OBE.

@hpinkos any thoughts here?

@hpinkos
Copy link
Contributor

hpinkos commented Jul 26, 2018

@mramato 2, 3, and 4 are all things we still potentially need to address. 2 in particular seems wrong to me but I'm not sure what the right thing to do is. @bagnell can you take a look at this?

1 is probably something we're not going to address. We made a decision to clamp to the highest resolution LOD and not reclamp if lower resolution tiles are loaded in when you zoom out from an area.

5 is OBE

6 was fixed at some point

@mramato
Copy link
Contributor

mramato commented Jul 26, 2018

Thanks, I tagged this as high priority since we'll want to address this as part of our terrain by default work.

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

No branches or pull requests

4 participants