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

ImGuiIntegration: add tests, support ImDrawCmd::VtxOffset #90

Closed
wants to merge 5 commits into from

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Jan 12, 2022

Closes #82.

I'm not 100% sure if the version/extension checks are correct this way. Info was taken from https://doc.magnum.graphics/magnum/classMagnum_1_1GL_1_1Mesh.html#aaa49afa2c76cc12b8402418a6e31923f.

This also needs some more testing on GLES/WebGL, hence a draft PR.

@pezcode
Copy link
Contributor Author

pezcode commented Jan 14, 2022

I added a small test window to visually check for correct handling of VtxOffset, taken from here.

If all goes well, it should look something like this with no obvious glitches:

chrome_2022-01-14_22-00-33

(screenshot taken on Chrome 97, with WebGL Draft Extensions enabled)

@pezcode pezcode marked this pull request as ready for review January 14, 2022 21:12
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #90 (ac74cf6) into master (26e7ca9) will decrease coverage by 0.04%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   77.20%   77.16%   -0.05%     
==========================================
  Files          21       21              
  Lines         939      946       +7     
==========================================
+ Hits          725      730       +5     
- Misses        214      216       +2     
Impacted Files Coverage Δ
src/Magnum/ImGuiIntegration/Context.h 100.00% <ø> (ø)
src/Magnum/ImGuiIntegration/Context.cpp 89.30% <80.00%> (-0.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26e7ca9...ac74cf6. Read the comment docs.

@pezcode
Copy link
Contributor Author

pezcode commented Jan 19, 2022

Forgot to mention: this unconditionally requires imgui 1.71. Since that version is considered obsolete (ie. no official backward compatibility in the API), I'd say that's acceptable. If you'd prefer to support older versions, we can #ifdef around this as well.

@mosra mosra added this to the 2022.0a milestone Feb 3, 2022
@mosra mosra added this to TODO in GUI via automation Feb 3, 2022
@pezcode
Copy link
Contributor Author

pezcode commented Feb 5, 2022

I added integration tests for renderFrame() to make sure it correctly handles scissor rect, texture IDs, index offset and vertex offset (if supported). Hopefully makes your Vulkan porting easier in the future, too 🌋 The visual test got removed because it's now unnecessary.

All the scaffolding was taken from the Magnum Shader tests. Some special-casing (iOS?) may not be needed, I wasn't too sure about the interactions there.

@pezcode
Copy link
Contributor Author

pezcode commented Feb 5, 2022

Not sure what's happening to Github, it just completely skipped running CI 💩

@pezcode pezcode changed the title ImGuiIntegration: add support for ImDrawCmd::VtxOffset ImGuiIntegration: add tests, support ImDrawCmd::VtxOffset Feb 5, 2022
@pezcode pezcode force-pushed the imgui-vtxoffset branch 2 times, most recently from 06b0194 to c0a987b Compare February 6, 2022 11:40
@pezcode
Copy link
Contributor Author

pezcode commented Feb 6, 2022

The linux-gles tests seem to be the only ones that run ctest without -E GLTest. Can we enable the GL tests in other CIs without too much breaking? I assume they're disabled for a good reason, but running the ImGui drawFrame() tests on a few different platforms would be useful.

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Thank you! Tests look great 👍

Can we enable the GL tests in other CIs without too much breaking?

The GL tests run against SwiftShader, which is the only software GL implementation I was able to get working there, and it's GLES-only. I could enable it on macOS as well as Magnum builds got that recently already, on Windows I don't have any existing SwiftShader build to copy from yet, so there it will take some more effort.

After I manage to tag a release I plan to drop support for Ubuntu 16.04, and then I could try adding a Mesa llvmpipe build, to have an ability to test the full desktop GL. But that's still quite far ahead.

src/Magnum/ImGuiIntegration/Test/CMakeLists.txt Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Test/CMakeLists.txt Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Test/CMakeLists.txt Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Test/CMakeLists.txt Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Test/CMakeLists.txt Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Test/ContextGLTest.cpp Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Test/ContextGLTest.cpp Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Context.h Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Context.h Outdated Show resolved Hide resolved
src/Magnum/ImGuiIntegration/Context.h Outdated Show resolved Hide resolved
@pezcode
Copy link
Contributor Author

pezcode commented Feb 6, 2022

Thanks for the detailed review ☺️

I think I fixed everything, except for replacing TgaImporter with PngImporter. The effort of adding and building magnum-plugins on all CIs just for the ImGuiIntegration GL tests didn't seem worth it considering the test images are 2.5KB combined. But if you really have a strong preference for PNGs, I can add it.

@mosra
Copy link
Owner

mosra commented Feb 6, 2022

Since it's a UI library, I anticipate the test image corpus to grow over time, especially if/when we switch to our own text rendering, or to Magnum's own theme, for example. With small images TGAs are acceptable, but when they grow larger it stops being practical due to the huge file sizes. (Not to mention the lack of software support makes dealing with them kinda annoying, especially here in the browser.)

adding and building magnum-plugins on all CIs

Just on the ES build, in unix-desktop-gles.sh, where the GL tests are run ;) If you pick StbImageImporter (and not the full PngImporter with the external dependencies), apart from the time it takes to clone the repo & run cmake, the build time would be comparable to build time of the TgaImporter. Nothing else from the plugins repo will get built, I made it so exactly this use case is as efficient as it can be.

You can reuse the relevant piece from unix-desktop.sh, just drop the DART-related stuff:

# Magnum Plugins
git clone --depth 1 https://github.com/mosra/magnum-plugins.git
cd magnum-plugins
mkdir build && cd build
cmake .. \
-DCMAKE_CXX_FLAGS="$CMAKE_CXX_FLAGS" \
-DCMAKE_INSTALL_PREFIX=$HOME/deps \
-DCMAKE_INSTALL_RPATH=$HOME/deps/lib \
-DCMAKE_BUILD_TYPE=$CONFIGURATION \
-DWITH_ASSIMPIMPORTER=$WITH_DART \
-DWITH_STBIMAGEIMPORTER=$WITH_DART \
-G Ninja
ninja install
cd ../..

Using rather basic integration tests by drawing a few shapes and then
comparing the framebuffer content against known correct output. I hope
the thresholds don't blow up in my face on WebGL.
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

🎉

@pezcode
Copy link
Contributor Author

pezcode commented Feb 7, 2022

Just realized the TgaImporter is still being built, as well as AnyImageImporter on CIs that don't run the GL tests. If you're not in a rush, I'll fix it sometime this week. 🙇

@mosra
Copy link
Owner

mosra commented Feb 8, 2022

Merged as 26e7ca9...7931348, thanks for the reminder -- I changed the commits to enable the relevant plugins only for the ES CI builds.

As of b29aeee there's also a GLES3 macOS build, I'll eventually add Windows variants once Magnum has them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
GUI
  
Done
Development

Successfully merging this pull request may close these issues.

ImGui integration does not support drawing lots of elements (ImGuiBackendFlags_RendererHasVtxOffset)
2 participants