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

Enable non-ascii chars for MovableText #1374

Merged
merged 2 commits into from
Jul 15, 2019

Conversation

rhaschke
Copy link
Contributor

As pointed out in ros-visualization/interactive_markers#40, rviz doesn't support non-ASCII chars in rendered text. This was due to two shortcomings:

  1. The std::string, which is utf8-encoded on Linux nowadays, wasn't interpreted as an utf8-encoded string, but as an ASCII string.
  2. To render non-ASCII chars, the font needs to load corresponding textures. By default, it looks like only 0-127 are loaded, which makes sense to limit memory usage.

I extended the range arbitrarily to 0-999, solving the mentioned issue:
rviz-utf8

However, that's not an optimal solution at all, because it loads many textures that aren't probably used by most applications. On the other hand, still lots of chars are missing.
What would be needed, is a mechanism to explicitly specify the required codepoint range, either

  • dynamically, extending the range based on required chars and reloading the font if necessary
  • or statically, using e.g. some ROS parameter to list the code point range

While the first option would be the most convenient, I have no idea how to reload a font resource.

@rhaschke rhaschke requested a review from wjwwood May 10, 2019 10:47
@ghost ghost assigned rhaschke May 10, 2019
@simonschmeisser
Copy link
Contributor

Do you know how much additional memory we are talking about? 1000 is indeed a small subset of the 1,112,064 code points

@rhaschke
Copy link
Contributor Author

Do you know how much additional memory we are talking about?

No. not at all.

1000 is indeed a small subset of the 1,112,064 code points

But already way better than 127 😉

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

I am by far not an expert on non-ascii string (unicode, etc.), but I did review the code changes and didn't see any issues. I would just encourage you to run the "acceptance tests" in the testing folder to make sure ascii labels show properly still.

@simonschmeisser
Copy link
Contributor

I guess a superior solution would be to read out which code point ranges the current font supports and load only these ( I suspect those not to be necessarily continuous ...), but it already looks like a good improvement

@rhaschke rhaschke merged commit fa00ca4 into ros-visualization:melodic-devel Jul 15, 2019
@rhaschke rhaschke deleted the utf8-text branch July 15, 2019 19:46
@rhaschke rhaschke mentioned this pull request Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants