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

Add polylines on 3D Tiles Sandcastle Example #7522

Merged
merged 9 commits into from
Feb 1, 2019
Merged

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Jan 29, 2019

Shows polylines on a BIM model:
image

And on Photogrammetry:
image

@cesium-concierge
Copy link

Thanks for the pull request @hpinkos!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 30, 2019

  • Please add to CHANGES.md
  • BIM model looks great
  • (1) are we sure about the photogrammetry model? The orignial request was to use a higher-resolution one to show the benefit when zoomed in close how the polyline is "pixel perfect." (2) if that is unrealistic, are we sure this is a good location for the line given the building's facade geometry?

image

@hpinkos
Copy link
Contributor Author

hpinkos commented Jan 30, 2019

@pjcozzi do you have a photogrammetry tileset in mind? This is the only one we have in other Sandcastle examples.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 31, 2019

No, I was hoping you or @ggetz could explore.

@hpinkos
Copy link
Contributor Author

hpinkos commented Jan 31, 2019

In the interest of getting this merged before the release, I removed the photogrammetry example. I'll open an issue to add something better for the next release.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 31, 2019

I removed the photogrammetry example. I'll open an issue to add something better for the next release.

I dunno, this seems like the most prominent use for this polyline feature. Why not just move the polyline on the current model and write an issue to create a higher quality example that shows the full value compared to the sub-sampling example that already exists?

@hpinkos
Copy link
Contributor Author

hpinkos commented Jan 31, 2019

@pjcozzi Something like this?

image

If not, please give me a better idea what you're looking for. Should I draw a line over the cars to better show that it follows contours? The polylines don't look good on foliage for the same reason you didn't like it on the side of the building.

@ggetz
Copy link
Contributor

ggetz commented Feb 1, 2019

@hpinkos Perhaps along one of the flatter vertical sides of the buildings? Or zoom in on the roof and draw lines like you would be measuring equipment there?

@hpinkos
Copy link
Contributor Author

hpinkos commented Feb 1, 2019

@ggetz the problem with going along edges is you get lots of things like this because the walls of the building are wavy:
image

@hpinkos
Copy link
Contributor Author

hpinkos commented Feb 1, 2019

@ggetz ready

@ggetz
Copy link
Contributor

ggetz commented Feb 1, 2019

Thanks @hpinkos!

Regarding

write an issue to create a higher quality example that shows the full value compared to the sub-sampling example that already exists?

Let's write an issue to include a higher quality photogrammetry model for sandcastle examples, and mention this example could benefit from using it, along with the sub-sampling example.

@hpinkos
Copy link
Contributor Author

hpinkos commented Feb 1, 2019

Done #7532

@ggetz
Copy link
Contributor

ggetz commented Feb 1, 2019

Thank @hpinkos !

@ggetz
Copy link
Contributor

ggetz commented Feb 1, 2019

Can you add a note to CHANGES.md? We did that with the model wheel animation sandcastle example this release.

@hpinkos
Copy link
Contributor Author

hpinkos commented Feb 1, 2019

Yep, sorry I missed that @ggetz
This is ready now

@ggetz
Copy link
Contributor

ggetz commented Feb 1, 2019

Thanks again @hpinkos !

@ggetz ggetz merged commit ef26844 into master Feb 1, 2019
@ggetz ggetz deleted the sandcastle-poly-3dtiles branch February 1, 2019 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants