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

Don't clamp KML or GeoJSON billboards and labels if clampToGround is false. #4459

Merged
merged 3 commits into from
Oct 20, 2016

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Oct 19, 2016

Closes #4415

@tfili can you review this. Was this an oversight from when we added clamping, or did we specifically have it always clamp billboards and labels? I feel like the clampToGround option should be consistent throughout KmlDataSource.

@mramato
Copy link
Contributor Author

mramato commented Oct 19, 2016

@tfili GeoJsonDataSource was doing the same thing, so I updated the code there too. However, I'm not 100% this is what we want, lets discuss tomorrow.

@mramato mramato changed the title Don't clamp KML billboards and labels if clampToGround is false. Don't clamp KML or GeoJSON billboards and labels if clampToGround is false. Oct 20, 2016
@mramato
Copy link
Contributor Author

mramato commented Oct 20, 2016

To expand on this a little more, the root of the problem is that clamping to ground is currently prohibitively slow on some browsers or with lots of data. It's also buggy (which we definitely want to fix this week). However, even if we fix the bugs, I'm not sure performance is there yet and as I said in my originally post, it's weird to me that the clampToGround option was being ignored for billboards and labels to begin with.

So I think I want this PR to get merged, but I'm open to discussion or other people's thoughts as to what the default behavior should be.

@tfili
Copy link
Contributor

tfili commented Oct 20, 2016

This looks good. Thanks @mramato.

@tfili tfili merged commit bc67845 into master Oct 20, 2016
@tfili tfili deleted the no-clamp branch October 20, 2016 15:26
@denverpierce
Copy link
Contributor

Wasn't this intentional? CHANGES made it seem like it was supposed to be this way:

Point and Model features will always respect altitudeMode

Points with a height will be drawn at that height; otherwise, they will be clamped to the ground.

@mramato
Copy link
Contributor Author

mramato commented Oct 20, 2016

Yes it was; (apparently I wanted it that way even though @tfili disagreed). Because of some issues that have been reported and performance work that still needs to be done on clamping, I think this is the better (and more consistent) strategy for the time being. Our goal is still terrain on by default, at which point the clampToGround options on datasources will most likely get deprecated (and always be true); but we're just not there yet.

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.

4 participants