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 issue #4131 #4157

Conversation

AndreyOrlov
Copy link
Contributor

No description provided.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 31, 2016

Thanks for the fix, @AndreyOrlov. Can you update CHANGES.md?

Also, sorry for the slow response; most of the team was at SIGGRAPH last week.

@tfili can you review and merge this? Is it possible to write a reasonable unit test?

For #4131.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 31, 2016

Everyone - we have a CLA.

@tfili
Copy link
Contributor

tfili commented Aug 1, 2016

The change looks good. @AndreyOrlov could you merge in master. Also, can you add a unit test. Having a tesselated LineString with a LineStyle that has a color should do the trick. Then just check that there is a corridor and that the material property is correct.

Thanks

@@ -1150,7 +1150,7 @@ define([
entity.corridor = corridor;
corridor.positions = coordinates;
if (defined(polyline)) {
corridor.material = defined(polyline.material) ? polyline.material.color : Color.WHITE;
corridor.material = defined(polyline.material) ? polyline.material.color.getValue() : Color.WHITE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getValue requires time as its first parameter, if we are 100% sure that .material.color will be a constant value (maybe it's impossible to make it time varying with kml?) then we should still use Iso860.MINIMUM_VALUE here. @tfili can you check if this is indeed always constant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, colors in KML are always constant (excluding the highlight style in StyleMaps, which isn't supported in Cesium). I would just change it to use Iso860.MINIMUM_VALUE as @mramato suggested.

@AndreyOrlov
Copy link
Contributor Author

@tfili, @mramato I added unit test and Iso860.MINIMUM_VALUE.

@mramato
Copy link
Contributor

mramato commented Aug 2, 2016

@tfili feel free to merge if you are happy.

@tfili
Copy link
Contributor

tfili commented Aug 2, 2016

@AndreyOrlov Looks good. Need you to merge in master again. There appear to be another set of conflicts. I'll merge once it's updated.

@AndreyOrlov
Copy link
Contributor Author

@tfili Done.

@tfili
Copy link
Contributor

tfili commented Aug 2, 2016

Thanks @AndreyOrlov

@tfili tfili merged commit 70be258 into CesiumGS:master Aug 2, 2016
@AndreyOrlov AndreyOrlov deleted the issue4131-kml-corridor-clamp-to-ground-error branch August 2, 2016 21:30
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