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

Use Magnum's builtin text rendering instead of ImGui; expose it in Python #1853

Merged
merged 4 commits into from
Sep 8, 2022

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Sep 2, 2022

Motivation and Context

As we discussed in #1842, this does the following:

  • Drops ImGui in favor of Magnum's own text rendering. All ImGui was ever used for was a single text HUD, so not even 1% of its functionality. IIRC, originally it was chosen because Magnum's own text rendering "seemed hard", but I don't think that's the case -- the amount of code is basically the same for both. But what we gain compared to ImGui is that:
    • The viewer doesn't need to constantly redraw to function anymore, which may be especially useful for resource-efficient web / mobile use cases. ImGui needed that in order to handle events and do other things that we didn't make use of.
    • We can easily do text rendering in Python now. With ImGui it'd be about having to rewrite PyImGui with a Magnum backend, or using a completely different solution altogether.
  • To keep at least some of the warm fuzzy feeling, I reused ProggyClean.ttf that ImGui embeds.
  • Besides that, the Magnum::Text library is also now exposed to Python:
  • Besides that, this also contains the complete GltfSceneConverter plugin, which is now capable of saving most scenes. But since it doesn't contain the related magnum-sceneconverter changes yet, it's not really useful to anybody, actually. And for the batch renderer use case it also still needs custom scene field support and sampler property export to be finished.

How Has This Been Tested

Here's how it looks now. Probably not pixel-perfect to what was there before, but I'd say pretty close. Coincidentally, in this particular case both ImGui and Magnum use the exact same font renderer. (I'd personally make the text significantly smaller, it's obnoxiously big, let me know if you want that.)

image

If anybody else should be added as a reviewer, please add them ;)

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 2, 2022
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Running the viewer after installing on Fedora 36 I get:

PluginManager::Manager::Manager(): none of the plugin search paths in {../lib/magnum/fonts, magnum/fonts} exists and pluginDirectory was not set, skipping plugin discovery
PluginManager::Manager::load(): plugin TrueTypeFont is not static and was not found in 
Cannot open font file

There's really no need to pull in that whole dependency for just a bit
of text. At the very least, to keep the feeling, I reused the ImGui
default font.

Originally ImGui got used because Magnum::Text "seemed complicated", but
I don't really think that's the case :P
@mosra
Copy link
Collaborator Author

mosra commented Sep 2, 2022

Fixed, sorry! Forgot that you all use a build with bundled Magnum 😅

Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Works for me! Thanks for slicing out another dependency.
The text is rather larger now, let's shrink it down a bit.

Unfortunately, due to Python implicitly isolating loaded native modules,
this will only work if the following is done:

    import ctypes, sys
    flags = sys.getdlopenflags()
    sys.setdlopenflags(flags | ctypes.RTLD_GLOBAL)
    import magnum
    # Now there's no StbTrueTypeFont yet
    import habitat_sim
    # Now there is

An alternative solution would be to force newer CMake to be used and
link the StbTrueTypeFont plugin to the magnum bindings module *from
outside* :/
@mosra
Copy link
Collaborator Author

mosra commented Sep 7, 2022

@aclegg3 is it better now? I realized too late that the original setting was DPI-dependent and what looked alright on my 4K display with 1.6875x scaling was probably way too huge on non-HiDPI screens.


Also I added a commit that links the font plugin for Python bindings, but unfortunately due to the dreaded problem with native Python modules being isolated from each other, it will only work if the following is done:

import ctypes, sys
flags = sys.getdlopenflags()
sys.setdlopenflags(flags | ctypes.RTLD_GLOBAL)

import magnum
# Now there's no StbTrueTypeFont yet

import habitat_sim
# Now there is, but only thanks to RTLD_GLOBAL

An alternative solution would be to force CMake 3.13+ to be used and link the font plugin to the magnum bindings module from outside, similarly to how Erik Wijmans did it for the Basis compressor here. Then the plugin would be available right after import magnum.

This is yet another case of the exact same problem stemming from static libraries getting duplicated across multiple shared modules. I'll try to find a manual workaround at least -- something that needs an explicit piece of code but doesn't require fiddling with the CMake build or with dlopen flags.

@aclegg3
Copy link
Contributor

aclegg3 commented Sep 7, 2022

@aclegg3 is it better now?

Thanks, much better now!

Also I added a commit that links the font plugin for Python bindings, but unfortunately due to the dreaded problem with native Python modules being isolated from each other, it will only work if the following is done:
...

A bit messy, but functional. We can add this chunk to the viewer python with a comment to this effect.

An alternative solution would be to force CMake 3.13+ to be used ...

We already require CMake 3.14.0, so this doesn't sound like a blocker to me.

@mosra
Copy link
Collaborator Author

mosra commented Sep 7, 2022

3.14? Oh, haha, didn't know that. Good then, will change it to link the plugin directly to the magnum module. 3.12 it is instead, apparently. Keeping it like this (suggesting the dlopen flags workaround), will implement a proper solution in a followup PR.

@Skylion007
Copy link
Contributor

@mosra I'd much rather upgrade it to 3.14. It's a pretty old-CMake and cmake is pretty easy to update use pip install.

@Skylion007
Copy link
Contributor

@mosra Happy to bump the cmake version for 3.14. I just checked and the latest OpenEXR is specifically cmake 3.12 OR NEWER, so let's just bump the cmake version. There are a couple places we should do this, mainly the CricleCI and pyproject.toml.

I'm even fine bumping up to 3.20 as we already require that for our emsdk build.

@mosra mosra merged commit 58a6907 into main Sep 8, 2022
@mosra mosra deleted the magnum-text branch September 8, 2022 14:39
@mosra
Copy link
Collaborator Author

mosra commented Sep 8, 2022

"Pretty old" is relative ;) Personally I'm not bumping the minimal required version unless there's a significant new feature to use. And the "external linking" that's allowed since 3.13 feels like a hack in my opinion, and I'll provide a better way to handle static plugin dependencies for bindings and CLI utilities in a followup PR. Merging this as-is.

What on Emscripten needs 3.20, btw?

@Skylion007
Copy link
Contributor

Skylion007 commented Sep 8, 2022

# static Bullet libs, and Bullet static libs required CMake 3.13+.
from the CI comments.

@mosra

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants