mosra / magnum-integration

Integration libraries for the Magnum C++11 graphics engine
https://magnum.graphics/
Other
99 stars 45 forks source link

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

Closed pezcode closed 2 years ago

pezcode commented 2 years ago

Closes https://github.com/mosra/magnum-integration/issues/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 commented 2 years ago

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)

codecov[bot] commented 2 years ago

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%) :arrow_down:

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 commented 2 years ago

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.

pezcode commented 2 years ago

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 commented 2 years ago

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

pezcode commented 2 years ago

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.

pezcode commented 2 years ago

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 commented 2 years ago

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: https://github.com/mosra/magnum-integration/blob/26e7ca90aa0b4b4c406c8384f18f57c0c474098a/package/ci/unix-desktop.sh#L51-L64

pezcode commented 2 years ago

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 commented 2 years ago

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

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