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

Polylines on 3D Tiles #7437

Merged
merged 13 commits into from
Jan 2, 2019
Merged

Polylines on 3D Tiles #7437

merged 13 commits into from
Jan 2, 2019

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Dec 22, 2018

Follows up on #7434 and adds support for polylines on 3D Tiles.

polylines

Demo I've been using for testing: localhost link

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

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

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

Copy link
Contributor

@likangning93 likangning93 left a comment

Choose a reason for hiding this comment

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

Just a couple notes on specs, but otherwise nothing pops out to me.

(btw this is awesome)

var visualizer = new PolylineVisualizer(scene, objects);

var polyline = new PolylineGraphics();
polyline.positions = new ConstantProperty([Cartesian3.fromDegrees(0.0, 0.0), Cartesian3.fromDegrees(0.0, 1.0)]);
Copy link
Contributor

Choose a reason for hiding this comment

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

As @OmarShehata discovered a little while back, the sizes of geometries can actually impact spec runtime pretty severely. Do the ClassificationType specs significantly increase runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw an improvement when changing 1.0 to 0.000001. Master is ~3.5 seconds, this branch is ~5.0, with that change I see ~4.5. It varies a lot though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't confirmed this but I have reason to believe that a spec that takes ~5 seconds on our machines will time out from time to time (like here #7249). If there's no easy way to bring this down I'd mention the spec in that issue when that gets merged so I can take a look next time I'm cutting down test times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry for not stating that my times were for the whole file not a single test.

scene.initializeFrame();
var isUpdated = visualizer.update(time);
scene.render(time);
return isUpdated;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a good way to wrap these pollToPromise blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I cleaned it up in PolylineVisualizerSpec and GeometryVisualizerSpec.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 28, 2018

🎉

In addition to the CZML example, please add/update a Sandcastle example using the CesiumJS API.

@lilleyse lilleyse changed the base branch from polylines-3d-tiles to classification-fixes January 2, 2019 20:44
@bagnell
Copy link
Contributor

bagnell commented Jan 2, 2019

This might be an issue that we don't fix. It'll possibly also be an issue with #7434.
image

@bagnell
Copy link
Contributor

bagnell commented Jan 2, 2019

Here is a better shot:
image

Source/Scene/GroundPolylinePrimitive.js Outdated Show resolved Hide resolved
Source/Scene/GroundPolylinePrimitive.js Show resolved Hide resolved
@lilleyse lilleyse force-pushed the polylines-on-3d-tiles branch from 7fec168 to 56b63d1 Compare January 2, 2019 22:24
@lilleyse lilleyse force-pushed the polylines-on-3d-tiles branch from 5d5188b to 3729e70 Compare January 2, 2019 22:53
@lilleyse
Copy link
Contributor Author

lilleyse commented Jan 2, 2019

In addition to the CZML example, please add/update a Sandcastle example using the CesiumJS API.

I updated the Classification Types demo (the yellow line on the road).

classification

@lilleyse
Copy link
Contributor Author

lilleyse commented Jan 2, 2019

All the comments should be addressed now. I opened an issue for the slicing through problem: #7453. It will require a bit of thinking to get right so I think it should get pushed to some other PR.

@bagnell bagnell merged commit 29e9146 into classification-fixes Jan 2, 2019
@bagnell bagnell deleted the polylines-on-3d-tiles branch January 2, 2019 23:35
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.

6 participants