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

Added credits for DataSources and Model #8173

Merged
merged 6 commits into from
Sep 20, 2019
Merged

Conversation

tfili
Copy link
Contributor

@tfili tfili commented Sep 18, 2019

  • Added a credit options for CZML, KML and GeoJson data sources as well as for Model.
  • Added ion credits to those classes as well that can't be removed from the instance.
  • We now allow scene.postRender callbacks to add credits. This was required for DataSource credits.
  • Added tests for all the classes.

@cesium-concierge
Copy link

Thanks for the pull request @tfili!

  • ✔️ 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.

Reviewers, don't forget to make sure that:

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

@tfili
Copy link
Contributor Author

tfili commented Sep 18, 2019

@lilleyse I just wanted to make sure what I did in Scene is ok. I don't see any ill effects from it.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2019

I think hooking this in in Viewer is too high level. DataSourceDisplay requires a Scene, right? I would add a new beginFrame event to Scene that DataSourceDisplay listens to. This event would fire sometime between when creditDisplay.beginFrame and creditDisplay.endFrame are called in Scene.render. DataSourceDisplay would listen to that event and call the updateCredits function when that is raised. Does that make sense?

@tfili
Copy link
Contributor Author

tfili commented Sep 19, 2019

I think hooking this in in Viewer is too high level. DataSourceDisplay requires a Scene, right?

Agreed.

I would add a new beginFrame event to Scene that DataSourceDisplay listens to.

Is there a need for another event? There is already [begin|end]Update and a [pre|post]Render events. Is there any ill effects from moving them out to where I have them (moved out of local function render into Scene.render).

@hpinkos
Copy link
Contributor

hpinkos commented Sep 19, 2019

@tfili yeah, looking at the code, you'll either need to add another event, or move where creditDisplay.startFrame gets called.

@tfili
Copy link
Contributor Author

tfili commented Sep 19, 2019

This should be good. To test

  • Start cesium server
  • Run ion locally as a beta user
  • Add a Model, CZML, GeoJSON and a KML and give them all attributions
  • Click the View in Sandcastle link for each of them
  • Verify you see your attributions

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

CHANGES.md?

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

Works great @tfili! Update CHANGES and I'll merge this in

@tfili
Copy link
Contributor Author

tfili commented Sep 20, 2019

@hpinkos Done!

@hpinkos hpinkos merged commit 5a41994 into master Sep 20, 2019
@hpinkos hpinkos deleted the datasource-model-credits branch September 20, 2019 15:24
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.

3 participants