Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Clock stop event #7066

Merged
merged 6 commits into from
Oct 23, 2018
Merged

Clock stop event #7066

merged 6 commits into from
Oct 23, 2018

Conversation

dwastberg
Copy link
Contributor

Add an onStop Event to the Clock which is called when the clock reaches its stopTime.

@cesium-concierge
Copy link

Thank you so much for the pull request @dwastberg! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ❌ Missing CLA.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

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.

🌍 🌎 🌏

@dwastberg
Copy link
Contributor Author

CLA has been sent

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 25, 2018

Thanks again for the contribution, @dwastberg. We received your CLA.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 25, 2018

Thanks @dwastberg! This PR looks fine to me. Can you please explain a scenario where this event is useful?

@dwastberg
Copy link
Contributor Author

I'm using the clock to control a looping simulation and at the end of each loop I want to clean up some objects placed in the scene and make some changes to my GUI.

clock.onStop.addEventListener(onStopSpy);
clock.tick();
expect(onStopSpy).toHaveBeenCalled();

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this whitespace

@hpinkos
Copy link
Contributor

hpinkos commented Sep 26, 2018

Thanks @dwastberg! Just that one comment. And please add a note under the Additions section of CHANGES.md

@dwastberg
Copy link
Contributor Author

Just wondering if there is anything else needed to merger this?

@hpinkos
Copy link
Contributor

hpinkos commented Oct 23, 2018

Thanks for the reminder @dwastberg! We don't get notifications when you add more commits, so it's a good idea to make a comment to let us know when a PR is ready for another look.

This all looks good to me.
Does anyone else have feedback? If not, I'll merge this by the end of the day

@hpinkos
Copy link
Contributor

hpinkos commented Oct 23, 2018

Thanks again @dwastberg!

@hpinkos hpinkos merged commit a0a8ad4 into CesiumGS:master Oct 23, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants