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

Credit support for 3D Tiles tileset.json #7454

Merged
merged 3 commits into from
Jan 18, 2019
Merged

Credit support for 3D Tiles tileset.json #7454

merged 3 commits into from
Jan 18, 2019

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Jan 2, 2019

Allows credits to be defined as part of tileset.json under extras.cesium.credits

Here's an example of what that might look like:

{
  "asset": {
    "extras": {
      "cesium": {
        "credits": [
          { 
            "html": "this is my credit", 
            "showOnScreen": false 
          }, { 
            "html": "<span>this one uses html</span>", 
            "showOnScreen": true 
          }
        ]
      }
    }
  }
  ...
}

cc @mramato

@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.

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

🌍 🌎 🌏

@lilleyse
Copy link
Contributor

lilleyse commented Jan 2, 2019

If it matters, the ion extras are under asset.extras rather than top-level extras. Should this also be under asset.extras for consistency?

@hpinkos
Copy link
Contributor Author

hpinkos commented Jan 3, 2019

Updated

@mramato
Copy link
Contributor

mramato commented Jan 4, 2019

@lilleyse while I know extras is a catch all, is there some centralized document we should be maintaining a list of all extras used by the Cesium ecosystem? Seems like formalizing at least our own usage is important.

@emackey
Copy link
Contributor

emackey commented Jan 4, 2019

Should STK and other non-Cesium viewers be displaying these credits to users as well? Do you need cesium in the hierarchy, or is asset.extras.credits good enough? Of course for STK we wouldn't be displaying HTML credits, it would be plain text.

Does Cesium need to sanitize the HTML for untrusted 3D Tiles sources?

FWIW, glTF 2.0 has ended up with a similar construct in its own asset object. Sketchfab's glTF downloads are already including information that looks like this:

  "asset": {
    "extras": {
      "author": "manilov.ap (https://sketchfab.com/manilov.ap)",
      "license": "CC-BY-4.0 (http://creativecommons.org/licenses/by/4.0/)",
      "source": "https://sketchfab.com/models/908985d8ec544638bcd661bc315597ad",
      "title": "Sr71"
    },
    "generator": "Sketchfab-3.20.7",
    "version": "2.0"
  },

@hpinkos
Copy link
Contributor Author

hpinkos commented Jan 4, 2019

@emackey Credit already sanitizes the html https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/Credit.js#L102

@mramato
Copy link
Contributor

mramato commented Jan 4, 2019

@emackey I think the idea is that we scope these items to cesium or ion initially and if they prove useful eventually we'll promote them to something more standard. I don't think we want to start dumping random properties in at the top level because that makes conflicts too easy.

@emackey
Copy link
Contributor

emackey commented Jan 4, 2019

Yeah I'm just thinking out loud here. If there are credits that are required to be shown, there should be some cross-platform mechanism for communicating that. Some people say glTF didn't do a good enough job of nailing this down before the 2.0 spec shipped. But these are all just suggestions to think about.

@mramato
Copy link
Contributor

mramato commented Jan 18, 2019

I swear I thought I merged this. Thanks @hpinkos!

@mramato mramato merged commit c430e8f into master Jan 18, 2019
@mramato mramato deleted the tile-credits branch January 18, 2019 15:20
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.

5 participants