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 full UTF-8 support in Labels #7280

Merged
merged 8 commits into from
Oct 25, 2019
Merged

Added full UTF-8 support in Labels #7280

merged 8 commits into from
Oct 25, 2019

Conversation

andreborud
Copy link
Contributor

Added it in my private project and thought other might like it as well.
I have added a third-party library called GraphemeSplitter which I have slightly modified to be able to import it with define().
The other change is to LabelCollection where it now splits double characters like emojis correctly.

@cesium-concierge
Copy link

Thank you so much for the pull request @andreborud! 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 to third party files were made.
    • Looks like a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version.

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.

🌍 🌎 🌏

@andreborud
Copy link
Contributor Author

CLA was sent a few minutes before the pull request.
And CONTRIBUTORS.md has the change in the first commit.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 22, 2018

Thanks @andreborud, we received your CLA.

We'll find someone to review this, but given this is a lot of third-party code, are you sure this is the best approach compared to something like a callback where the third-party library could be used to modify the string?

Also, do you have any ideas on the performance or memory usage? Some Cesium apps update labels all the time and we would not want to impact performance.

Finally, if this is merged, we would ask you to add a Sandcastle code example and unit tests. See https://github.com/AnalyticalGraphicsInc/cesium/blob/master/CONTRIBUTING.md#pull-request-guidelines

@andreborud
Copy link
Contributor Author

Thanks @pjcozzi.

I'm realising "Added support for emojis in Labels" might not be the best description for what this pull request adds. We had two problems that got solved simultaneously one was that we needed to use national flags which we used billboards for which reduced the performance for us, but using emoji national flags in a label instead reduced the number of entities we had to draw.

The other problem was that cesium didn't support many non-english characters which we needed. So maybe this pull request should be called "Added full UTF-8 support for labels".

Can't say I'm certain this is the best approach and I haven't made any extensive performance/memory testing.
Could it be a better idea to make this optional for labels or to only apply it if any of the characters in the label has a higher character number than 128 as an example?

@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.


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

🌍 🌎 🌏

@mramato
Copy link
Contributor

mramato commented Jan 2, 2019

Sorry for the delay here. I'm interested in getting this reviewed and merged, since as you mentioned it's more about UTF-8 support) but I need to take a look at your library and better understand the issue all around. I'll try to get to it before the next release.

@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 4, 2019

@andreborud please merge in master when you have a chance. @mramato will hopefully be able to look at this for the next release. Thanks!

@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

6 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @andreborud!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@andreborud andreborud changed the title Added support for emojis in Labels Added full UTF-8 support in Labels Oct 4, 2019
@andreborud
Copy link
Contributor Author

Sorry for the delay. Master branch merged.

@mramato
Copy link
Contributor

mramato commented Oct 25, 2019

@andreborud I apologize again for it taking so long to get this merged. I think "support for emojis" is definitely underselling the improved support for non-latin character sets that this PR provides. I'll tweak CHANGES in master, but otherwise this looks good to me. Thanks!

I was slightly worried that this would affect label performance, but since it only happens at label generation time, it has negligible impact in the small amount of testing I did. Plus if performance does turn out to be an issue, it would be trivial to make this opt-in.

@mramato mramato merged commit 67cdd49 into CesiumGS:master Oct 25, 2019
mramato added a commit that referenced this pull request Oct 25, 2019
@andreborud
Copy link
Contributor Author

Happy to be of service. :)

CHANGES.md Show resolved Hide resolved
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